-
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
Add protection if non-existent geometry is used #47587
base: master
Are you sure you want to change the base?
Conversation
cms-bot internal usage |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47587/44071 |
A new Pull Request was created by @srimanob 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 |
@cmsbuild please test |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47587/44076 |
Pull request #47587 was updated. @antoniovilela, @cmsbuild, @davidlange6, @fabiocos, @mandrenguyen, @rappoccio can you please check and sign again. |
@cmsbuild please test |
-1 Failed Tests: UnitTests RelVals RelVals-INPUT Unit TestsI found 2 errors in the following unit tests: ---> test test_MC_22_crosscheck had ERRORS ---> test test_MC_23_crosscheck had ERRORS RelVals
Expand to see more relval errors ...RelVals-INPUT
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47587/44081 |
Pull request #47587 was updated. @antoniovilela, @cmsbuild, @davidlange6, @fabiocos, @mandrenguyen, @rappoccio can you please check and sign again. |
@cmsbuild please test |
+1 Size: This PR adds an extra 48KB to repository Comparison SummarySummary:
|
@@ -817,6 +817,8 @@ def addStandardSequences(self): | |||
try: | |||
if len(self.stepMap): | |||
self.loadAndRemember(self.GeometryCFF) | |||
if (self.GeometryCFF == 'Configuration/StandardSequences/GeometryRecoDB_cff'): | |||
print("Warning: Default GeometryRecoDB_cff is used. It may not work with your process.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cms-sw/alca-l2
Do you think this warning might cause confusion? It will appear when a DB condition is used or when no geometry is specified. The goal is to ensure users are aware of the Global Tag (GT) and Geometry they are using.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @srimanob
As a minimal line of warning that just certifies that there is something haswhich has to be checked this may work.
But I think that a user who reads the warning message and does not know about this PR may wonder why the default GeometryRecoDB_cff was used, and what's wrong with it: I would add a line that specifies why this happened and what should be done to fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Phat, Andrea,
what is exactly the pitfall that you are trying to warn the user about here?
Isn't taking the reco geometry from DB the recommended way (somehow guaranteed by using the right Global Tag) in the vast majority of the cases (and eventually all of them when the phase-2 geometry is put in DB)?
I think the number of false positive here would be very high.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If someone try to run Phase-2 without specifying geometry, maybe?
I open to any suggestion. That is why I raise the question here. If we agree that warning may cause more confusion, then I can remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
Warning: The default GeometryRecoDB_cff is being used. If you are using a DB geometry, you can ignore this warning. However, if no geometry is specified in your cmsDriver command, you may need to verify your configuration.
Of I can put if
just to trigger this warning only when no geometry is specify. Maybe that can be done.
PR description:
This PR changes how cmsDriver handles non-existent geometry.
PR validation:
Try to dump config files in many ways, and check.
If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:
No need of backport.