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

allow to choose fptype via the run_card #788

Merged
merged 30 commits into from
Nov 15, 2023
Merged

Conversation

oliviermattelaer
Copy link
Member

Hi Andrea,

This is one example (that I likely want to improve a bit) of a case where I want to pass information trought make_opts.
In this case the run_card is triggering a re-creation of make_opts in order to include the definition of FPTYPE requested by the user.

The point I want to improve is to include the check that fptype is correct at the compilation of the Source directory (for the moment it is at the level of the P directories.

So this relate to your request: #780.

Olivier

@oliviermattelaer
Copy link
Member Author

Hi Andrea,

Could we move forward on this PR?

Olivier

Copy link
Member

@valassi valassi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @oliviermattelaer thanks.

I found one minor (but blocking) issue, that I will fix.

I am a bit puzzled howver who creates vector.inc now. All tghe comments I had added in the header have now disappeared.

I also saw a 'standalone_cudapp' directory appear in MG5AMC/mg5amcnlo in one test, I need to understand this better.

If that's ok with you, I would rather push through #793 (today) and then come back to this one (next week).

Sounds ok? Thanks Andrea

…e the checks for errors in codegen log before manipulating the generated code
…h, add some debug printouts about comparison of generated code
….yml, disable on:push triggers to avoid launching two jobs instead of one
…h, fix a bash bug and disable comparisons to the existing repo
Copy link
Member

@valassi valassi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @oliviermattelaer I fixed a minor blocker issue, an dnow it builds ok for me.

NB I have not tried to exercise this in "my" setup, and I will most likely need to make further changes.

One question however for you ("request for change"?) is about vector.inc: I do not understand why previously the vector.inc was generated with all of my comments. Also, there are vector.inc files that end up in the MG5AMC/mg5amcnlo directory after generation. So I would try to make sure that

  • we get again the comments (like "THIS FILE CANNOT CONTAIN #ifdef DIRECTIVES" etc)
  • we do not produce vector.inc files elsewhere

My vague impression is that there are issues related to the "path" (it's the same issue I had mentioned last week, you had an exception that I had turned into a warning, maybe something around that area).

Maybe this comes from my calling treatcards run? Not sure, I have no time to debug this now, maybe it rings a bell to you?

Thanks Andrea

@oliviermattelaer
Copy link
Member Author

Hi,
let me reply now, but let's discuss this next week. (I'm just stuck since my laptop is running an heavy production for next week talk).

I do not think that I have changed the handling of vector.inc in this particular PR. (I did apply changes in the handling of make_opts but none in the handling of vector.inc. So this discussion is likely better in another issue (and/or PR)

Concerning the handling of vector.inc this file is first generated by the fortran output of the code.
either if you specify "--vector_size=XXXX" or if you use the plugin syntax (madevent_simd or madevent_gpu) that force some default value without the need to specify such value.

Now when you are handling the run_card.dat a check is done if the current vector_size requested in the run_card is
the same or not compare to the value of vector.inc.
If the new value is bigger than the one within vector.inc, then the code will force recompilation of the simd/gpu library for the new value.
What I'm not fully sure if what happens for the fortran side if the value was modified. ( Did I re-create vector.inc and re-compile the fortran code or not).
So yes, this code is created at generation time and modify at launch time to follow the run_card.

For the "path" issue, this is on my todo list (but that will likely be another PR, likely targetting gpucpp with MG5aMC and not the plugin).

@valassi
Copy link
Member

valassi commented Nov 9, 2023

Thanks Olivier, lets discuss next week. I will show you what happens in practice. Probably there is some unintended side effect somewhere, lets debug it together next week if confirmed.

…erging master

git checkout c803581 ../../MG5aMC
git submodule update
Fix conflicts:
	epochX/cudacpp/CODEGEN/PLUGIN/CUDACPP_SA_OUTPUT/launch_plugin.py
	epochX/cudacpp/CODEGEN/PLUGIN/CUDACPP_SA_OUTPUT/output.py
@valassi
Copy link
Member

valassi commented Nov 10, 2023

I have merged master into this (which was a bit tricky as there were a couple conflicts to solve). Lets discuss next week anyway.

…ent vector.inc (where comments have disappeared) that I want to debug - will revert
…g vector.inc

Revert "[floating_type_interface] regenerate gg_tt.mad, but this has a different vector.inc (where comments have disappeared) that I want to debug - will revert"
This reverts commit 5b4652b
…e comments in vector.inc are kept when 'madevent treatcards' is called

Note: vector.inc is initially created via export_v4.py, which is not available to treatcards.
Then vector.inc is modified by banner.py, whose write_one_include_file is overloaded in launch_plugin.py.
…ctor.inc is now that VECSIZE_MEMMAX is 16 instead of 16384
…e runcard to make sure that VECSIZE_MEMMAX is 16384 instead of 16
…uncard.dat) use 16384 instead of 16

Note also that 'WARNING! CPPRunCard instance has no attribute path' has disappeared from the logs.
This is because vector.inc is no longer regenerated?
…tor_size=32 to ensure that vector.inc is recreated when the runcard is modified to use 16384

Indeed, the warning now appears again "WARNING! CPPRunCard instance has no attribute path" when regenerating ggtt.mad
…rs again "WARNING! CPPRunCard instance has no attribute path"
…7b8964e28 including Olivier's patch for warning #790
… thrown after fixing #790 and removing the workaround
…d cudacpp_backend at the end of runcards, as it is now already there by default
…e existing global_flag in runcards, instead of adding a new line at the end
… to avoid duplicate '#end_of_make_opts_variables' in make_opts

Also add a comment about GLOBAL_FLAG in make_opts (why is this not propagated from the runcards?)
…e as avx_level in the runcard (and fix a minor bug, replace Auto by auto)
…ke_opts changes but only a comment is removed in there)
…nerate all 15 processes

No need to rerun the tests, there are no real changes in the code with respect to upstream/master
@valassi valassi self-requested a review November 15, 2023 17:58
Copy link
Member

@valassi valassi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Olivier, thanks a lot. I fixed with a hack the vector.inc issue (I generate with comments in export_v4.py, then if updates are needed via banner.py and launch_plugin.py I simply keep the existing comments and update the rest).

I also took the opportunity to include your fix for #790.

I will resrun the CI tests and then merge

@valassi
Copy link
Member

valassi commented Nov 15, 2023

Thanks Olivier I will now merge this.

Note: I tested individual madevent applications, but I have not tested the launch workflow. SO I have not tested if the runcard actually works here.

But I merge this anyway.

@valassi valassi merged commit b9f16e9 into master Nov 15, 2023
53 checks passed
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Aug 7, 2024
…_type' to 'cudacpp_fptype' for runcard options (madgraph5#700, madgraph5#788, madgraph5#835)

(Older commit on gg_tt.mad, later cherry-picked fixing AVX conflicts, and later backported to CODEGEN too)
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.

path is not defined in CPPRunCard when treatcards is called outside launch
2 participants