-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Update beamspot option in ConfigBuilder #47188
Update beamspot option in ConfigBuilder #47188
Conversation
cms-bot internal usage |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47188/43427
|
A new Pull Request was created by @francescobrivio for master. It involves the following packages:
@antoniovilela, @cmsbuild, @davidlange6, @fabiocos, @mandrenguyen, @rappoccio can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
Hmm well, this PR will render unusable any/all cmsDriver commands not specifying a beamspot. While that is arguable great for costly MC production, for many technical tests this just creates work to fix. Anyway, can you please suggest here a valid (default?) beamspot parameter for generic use? |
Hi Martin, here you mean all steps after GEN-SIM right? Because GS step should always specify a beamspot. In any case, I will update the PR to provide a default |
We have some (technical) TSG tests using cmsDriver which now fail, post GEN-SIM, such as with cmsDriver steps:
|
We also have some GEN-SIM steps here where we would need a generic beamspot parameter now: |
I think we should just add:
|
Or, we could just change the default in
as suggested at #47182 (comment) as option 1. (which I quite frankly prefer). |
Thanks @Martin-Grunewald and @mmusich !
This would still mean that the addOnTests need to be updated...Marco, Martin I could include it in this PR if you want, just let me know. |
Please make the change in this PR as the addOn tests are run by the PR testing bot. |
b365f57
to
3d70b17
Compare
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47188/43429
|
Pull request #47188 was updated. @Martin-Grunewald, @antoniovilela, @cmsbuild, @davidlange6, @fabiocos, @mandrenguyen, @mmusich, @rappoccio can you please check and sign again. |
Hmm, I guess |
Pull request #47188 was updated. @AdrianoDee, @Martin-Grunewald, @Moanwar, @antoniovilela, @cmsbuild, @davidlange6, @DickyChant, @fabiocos, @mandrenguyen, @miquork, @mmusich, @rappoccio, @srimanob, @subirsarkar can you please check and sign again. |
@cmsbuild, please test |
+1 Size: This PR adds an extra 92KB to repository Comparison SummarySummary:
|
+hlt
|
+Upgrade |
a kind ping @cms-sw/pdmv-l2 |
+pdmv |
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @antoniovilela, @sextonkennedy, @rappoccio, @mandrenguyen (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
Addressing issue #47182.
In this PR I'm updating the ConfigBuilder script to make sure that an exception is raised in case a cmsDriver command for MC is launched without the
--beamspot
parameter in the GEN step.Additionally I:
VtxSmearedDefaultKey
andVtxSmearedHIDefaultKey
default values to beDBrealistic
--beamspot DBrealistic
PREMIX
workflow steps that were missing the--beamspot
optionPR validation:
I have tested this PR in multiple conditions (Data,MC) x (pp,HI,cosmics) and verified that it behaves as expected with:
Additionally I verified that the command:
(where the
--beamspot
option is missing) raises the following exception:Backport:
Not a backport, but backports can be opened in case @cms-sw/pdmv-l2 experts think it could be useful.