Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add file for the test-suite of MG5aMC and a ci related to it #865

Closed
wants to merge 15 commits into from

Conversation

oliviermattelaer
Copy link
Member

Ok this is a proof of principle where we can add test within the madgraph framework and how to add them within the CID/CD.

Here I'm testing p p > t t~ within SIMD mode

@oliviermattelaer oliviermattelaer requested a review from a team as a code owner June 7, 2024 10:12
@oliviermattelaer oliviermattelaer requested a review from roiser June 7, 2024 10:12
@oliviermattelaer
Copy link
Member Author

Good news/Bad news:

  • I have added four tests
  • two of them are successful (that's the good news)
  • two of them are not (one of them might be related to the coupling ordering (let's hope), the second is related to a compilation issue, that I will investigate).

So yes we need more test :-) But I would propose to merge this anyway:-)

@oliviermattelaer
Copy link
Member Author

Hi @roiser,

I have updated the branch to allow the generation of the test (both the madgraph file and the cicd one ) from very simple file
(that include the metadata in the file).

When you add the reference file, you need to run
python create_acceptance_from_file.py
and both file will be updated

Can you add a one such file / test and give me a feedback?

@oliviermattelaer
Copy link
Member Author

Let me also add @valassi to this PR (low priority) since I have close the related one directly targeting the master branch.

@oliviermattelaer
Copy link
Member Author

@Andrea if you want to fix #886, I think the best is to merge this first.
This is actually adding some CI and fixing one bug detected by the CI, looking at the fix this seems closely related to your complain in #886, so I would suggest to merge this first and then I would have something clean to fix #886

@valassi
Copy link
Member

valassi commented Jul 24, 2024

Hi @oliviermattelaer thanks for asking the review :-)

Three comments

  • One, this is against master_june24: I would first look and approve Merge of master into master_june24 and channelid fixes/reimplementation #882 (channelid fixes into master), because master_june24 as-is is completely unusable for me
  • Two, this is mixing two things, some CI changes and some makefile clean changes. Why do you need the makefile changes? I'd rather look at them separately (and at first sight they may well break some of my stuff)
  • Three, for the CI itself, why did you put all the stuff inside the plugin? I'd rather keep everything in .github workflows if this is a CI-only test essentially. Or is this meant also as a manual test? (Later on with a submodule or separate repo it could go in the .github of that.) (And later on we should probably restructure/rename our CI tests, even something by name SH AV OM could do, we have three different things now, just brainstorming)

@oliviermattelaer
Copy link
Member Author

oliviermattelaer commented Jul 24, 2024 via email

@valassi
Copy link
Member

valassi commented Jul 29, 2024

Hi,

Thanks Olivier :-)

The makefile change was because one of the new CI was crashing. So I decide to do it right away. (ans looks like this is now "crucial" for your since the associated commit for the mg5amc branch has already been merged into gpucpp_june24) making important to merge this master_june24 to resolve that missmatch.

Ok I will look at that then. But you seem to be removing a lot of 'make clean' stuff I need, so it look sproblematic. Maybe it is not, I need to test that.

  • Three, for the CI itself, why did you put all the stuff inside the plugin? I'd rather keep everything in .github workflows if this is a CI-only test essentially. Or is this meant also as a manual test? (Later on with a submodule or separate repo it could go in the .github of that.)
    So yes the python script creates two files: 1) the file that MG5aMC needs to run test within the normal MG5aMC framework (so that all test can be reproduce locally with the normal search/regular expression/debug mode/... than standard MG5aMC suite. 2) This also creates the CI that simply runs the above scripts But this is also design to be as lightweight as possible (to allow anyone to add a test (and the associated CI) in less than 3 mins. Putting them inside .github does not sound appropriate for the #1 goal.

Ok thanks I see. Then ok to have that partly in test (a part is needed in .github anyway as you did), I will have a look

  • (And later on we should probably restructure/rename our CI tests, even something by name SH AV OM could do, we have three different things now, just brainstorming) This (=OM/SH/AV) has no interest to me to be exposed in the name/... git blame is there for that if/when such information is relevant.

I don't care about the name SH/AV/OM being exposed here, this was just an example or the first proposal that crossed my mind. I just want a clear naming convention and a way to distinguish them. We have three different CIs superposed. We can even call them CI1, CI2 and CI3 (or maybe CI0, CI1, CI2 - I guess that 'CI0' by SH that works on generated code will disappear eventually, while the other two CIs that generate code on the fly will stay). Is it ok for you to call them CI0, CI1, CI2? Or any better idea for names?

@oliviermattelaer
Copy link
Member Author

For the name of the tests.
The first question is which name do you refer to?
The name of the CI file (i.e. the name of the file in the .github)
The name of the test in the CI item?
The name of the test in the tree of test of mad?

I do not care too much about any of those to be honest. My point of view is the more flexible we are, the best it is (because we want that adding test SHOULD be easy for us and for future developper).

Therefore, If you add a prefix it should be human readable and related to what is tested:
like "same_seed_check" for all your test fortran versus cpp.
"statistical_check" for all those new test where we check that reference value is correct (statistically).

So to focus on this PR alone here and not on the general topic.
I'm ok to add a "meaningfull" prefix to wathever file/item/... that you want.
Tell me what kind of prefix you want and on which item, and I will adapt the PR accordingly.

Cheers,

Olivier

@valassi
Copy link
Member

valassi commented Sep 2, 2024

Hi @oliviermattelaer, now that the new CI in #979 and #980 has been merged, is this PR still relevant?

In any case if it is relevant we should first merge the june24 stuff (and update this PR accordingly). But I just wanted to know if it can be closed instead.

Thanks
Andrea

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants