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.

Sign up for GitHub

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

Jump to bottom

Remove add_input_for_banner from output.py (plugin_run_card is not needed in cudacpp) #1016

Merged
merged 9 commits into from
Oct 8, 2024
Merged

Remove add_input_for_banner from output.py (plugin_run_card is not needed in cudacpp) #1016

merged 9 commits into from
Oct 8, 2024

Conversation

valassi
Copy link
Member

@valassi valassi commented Oct 5, 2024

Hi @oliviermattelaer this is a very simple PR to remove add_input_for_banner from output.py (plugin_run_card is not needed in cudacpp)

It fixes #1015. As described there:

  • In output.py cudacpp_backend is only needed to define plugin_run_card
  • In banner.py plugin_run_card is only used in RunCard.plugin_input
  • HOWEVER in launch_plugin.py there is a specific CPPRunCard that overloads RunCard.plugin_input and DOES NOT USE plugin_run_card
  • THEREFORE plugin_run_card is not used at all in cudacpp and cudacpp_backend can be removed from output.py

Can you please review?

I need to get this out to then mak eprogress on other things for CHEP

Thanks Andrea

valassi added 4 commits October 5, 2024 10:06
@valassi
[banner] in CODEGEN output.py, comment out add_input_for_banner becau…
78f36c2 
…se plugin_run_card is not needed in cudacpp (madgraph5#1015)
@valassi
[banner] in CHANGELOG.md: madgraph5#1015 Remove add_input_for_banner …
ff45558 
…from output.py (plugin_run_card is not needed in cudacpp)
@valassi
[banner] regenerate all processes after commenting out add_input_for_…
ecf49c0 
…banner madgraph5#1015 - this triggers the removal of *.mad/bin/internal/plugin_run_card
@valassi
[banner] ** COMPLETE BANNER ** move back to CODEGEN logs from the lat…
63238de 
…est upstream/master for easier merging

git checkout upstream/master $(git ls-tree --name-only upstream/master */CODEGEN*txt)
@valassi valassi self-assigned this Oct 5, 2024
@valassi valassi changed the title Remove add_input_for_banner from output.py (plugin_run_card is not needed in cudacpp) (1 in pipeline) - Remove add_input_for_banner from output.py (plugin_run_card is not needed in cudacpp) Oct 7, 2024
@valassi
Copy link
Member Author

valassi commented Oct 7, 2024

Hi @oliviermattelaer as discussed via email.

This is N=1 in the pipeline of PRs I would like to merge. This is ready as is, should be merged to master. Please tell me if I can go ahead, thanks!

@oliviermattelaer
Copy link
Member

Ok,

I do not like this PR ... I either see this has half full or half empty PR. (meaning it either due too much or too little).

So here are two counter proposal:

  1. fully remove "plugin_run_card" mechanism from 3.6.1 and the associate "plugin_input" API, and see this as deprecated by the launch_plugin API (both API were designed for the cudacpp case so this will not break anything)

  2. keep this plugin_run_card completes as easy way to see new parameter (even if this is not used) and in that case add in it "floating_type".

I need to get this out to then mak eprogress on other things for CHEP

With all due respect, this is NOT something needed for CHEP (since this is a pure cleaning of the code which has zero impact on anything, so this can be put in super low priority). So let's discuss this at our meeting today.

Cheers,

Olivier

@valassi
Copy link
Member Author

valassi commented Oct 8, 2024

Ok,

I do not like this PR ... I either see this has half full or half empty PR. (meaning it either due too much or too little).

So here are two counter proposal:

1. fully remove "plugin_run_card" mechanism from 3.6.1 and the associate "plugin_input" API, and see this as deprecated by the launch_plugin API (both API were designed for the cudacpp case so this will not break anything)

2. keep this plugin_run_card completes as easy way to see new parameter (even if this is not used) and in that case add in it "floating_type".

Hi @oliviermattelaer thanks :-)

I would vote for option 1 then. I do not see the need for plugin_run_card since we have launch_plugin.py. I think you introduced first plugin_run_card and then launch_plugin.py, but I thought that maybe you had other plugins in mind too. If they are both for cudacpp, then I think that launch_plugin.py is enough, and we can remove plugin_run_card.

Can you do the removal of plugin_run_card from mg5amcnlo yourself, whenever you have time?

This is low priority I agree. The reason I put this first in my 'pipeline' is that the subsequent PRs also touch runcards, and I did not want to have to do things there in plugin_run_card if I knew that this was not needed anyway.

By the way I still think that this PR as is makes sense, if we eventually remove plugin_run_card. It is "too little" because it only removes it from cudacpp and not from mg5amcnlo, but inside cudacpp it should be ok? Well I can COMPLETELY remove all mentioning of plugin_runcard in cudacpp, rather than leave it commented out. But otherwise I do not see what elese needs to change in cudacpp itself?

I need to get this out to then mak eprogress on other things for CHEP

With all due respect, this is NOT something needed for CHEP (since this is a pure cleaning of the code which has zero impact on anything, so this can be put in super low priority). So let's discuss this at our meeting today.

My point was that I will produce profiling numbers for CHEP and I wanted to do it if possible from stuff merged in master. But not a problem, I will do it from my branch and then we can merge in master later on also after CHEP.

By the way there is no dev meeting today, it's next week.
https://indico.cern.ch/event/1355163/

valassi added a commit to valassi/mg5amcnlo that referenced this pull request Oct 8, 2024
@valassi
In banner.py, completely remove function plugin_input: cudacpp will u…
793b4ad 
…se launch_plugin.py instead

See madgraph5/madgraph4gpu#1015
See madgraph5/madgraph4gpu#1016
valassi added a commit to valassi/mg5amcnlo that referenced this pull request Oct 8, 2024
@valassi
In banner.py, completely remove function plugin_input: cudacpp will u…
f7959d2 
…se launch_plugin.py instead

This reverts most of commit 8d9f3f1

See madgraph5/madgraph4gpu#1015
See madgraph5/madgraph4gpu#1016
@valassi valassi mentioned this pull request Oct 8, 2024
Merged
@valassi
Copy link
Member Author

valassi commented Oct 8, 2024

I do not like this PR ... I either see this has half full or half empty PR. (meaning it either due too much or too little).

So here are two counter proposal:

1. fully remove "plugin_run_card" mechanism from 3.6.1 and the associate "plugin_input" API, and see this as deprecated by the launch_plugin API (both API were designed for the cudacpp case so this will not break anything)

I prepared a WIP PR
mg5amcnlo/mg5amcnlo#149

Is this what you had in mind?
Thanks
Andrea

@oliviermattelaer
Copy link
Member

Yes I merged the associated PR

valassi added 5 commits October 8, 2024 20:46
@valassi
[banner] update mg5amcnlo from 55a291dc8 (v3.6.0) to a7f4f9e10 (3.6.1…
b1bdaab 
… branch including plugin_input and driver.f fixes)

See mg5amcnlo/mg5amcnlo#148
Allow patching header/initialise/finalise in driver.f

See mg5amcnlo/mg5amcnlo#149
Remove plugin_input and plugin_run_card completely
@valassi
[banner] in .gitmodules, move mg5amcnlo from branch gpucpp to branch …
b3fefa4 
…3.6.1
@valassi
[banner] in CODEGEN, remove plugin_input completely as suggested by O…
a7ef501 
…livier

See mg5amcnlo/mg5amcnlo#149 (remove plugin_input and plugin_run_card completely)
@valassi
[banner] regenerate CODEGEN patch from gg_tt.mad after updating mg5am…
1ef55d6 
…cnlo (functionally equivalent, only line numbers change)

Only the following files are needed to build the patch:
- 1 in patch.common: SubProcesses/makefile
- 2 in patch.P1: driver.f, matrix1.f

./CODEGEN/generateAndCompare.sh gg_tt --mad --nopatch
git diff --no-ext-diff -R gg_tt.mad/SubProcesses/makefile > CODEGEN/PLUGIN/CUDACPP_SA_OUTPUT/MG5aMC_patches/PROD/patch.common
git diff --no-ext-diff -R gg_tt.mad/SubProcesses/P1_gg_ttx/driver.f gg_tt.mad/SubProcesses/P1_gg_ttx/matrix1.f > CODEGEN/PLUGIN/CUDACPP_SA_OUTPUT/MG5aMC_patches/PROD/patch.P1
git checkout gg_tt.mad

(Later checked that gg_tt.mad code is regenerated with no changes in c++/fortran - quite a few python changes however)
@valassi
[banner] ** COMPLETE BANNER (again) ** regenerate all processes
750af82
@valassi
Copy link
Member Author

valassi commented Oct 8, 2024

Yes I merged the associated PR

Hi @oliviermattelaer thanks :-)

I have modified this PR accordingly

  • I updated the mg5amcnlo submodule with the upstream removal of plugin_input
  • I have completely removed plugin_input from cudacpp (previously only commented out)
  • I have regenerated patch.P1
  • I have regenerated all processes

Is this ok for merging now or are there other things to fix/improve?

Thanks
Andrea

@valassi valassi mentioned this pull request Oct 8, 2024
Merged
oliviermattelaer
oliviermattelaer approved these changes Oct 8, 2024
View reviewed changes
Copy link
Member

@oliviermattelaer oliviermattelaer left a comment

Choose a reason for hiding this comment

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

Thanks, this is good to go.

Thanks,

Olivier

@valassi
Copy link
Member Author

valassi commented Oct 8, 2024

Thanks to you :-)

Merging now

@valassi valassi changed the title (1 in pipeline) - Remove add_input_for_banner from output.py (plugin_run_card is not needed in cudacpp) Remove add_input_for_banner from output.py (plugin_run_card is not needed in cudacpp) Oct 8, 2024
@valassi valassi merged commit c1f5dcf into madgraph5:master Oct 8, 2024
169 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviermattelaer oliviermattelaer oliviermattelaer approved these changes

Assignees

@valassi valassi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cudacpp_backend card should be removed from output.py (it is only needed in launch_plugin.py)
2 participants
@valassi @oliviermattelaer