-
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
Quieting 76x step2 and step3 #11963
Quieting 76x step2 and step3 #11963
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -86,7 +86,7 @@ VersionedIdProducer(const edm::ParameterSet& iConfig) { | |
} | ||
|
||
if( idMD5 != calculated_md5 ) { | ||
edm::LogError("IdConfigurationNotValidated") | ||
edm::LogInfo("IdConfigurationNotValidated") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lgray If I understand correctly, with md5 check machinery not working, a noticeable feature of VIDs to be able to validate the configuration is dead. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still trying to understand it. I am somewhat annoyed if I have to keep updating it over time for no reason... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, wouldn't say that a major feature is missing. It's possible to check git hashes as well (which appear to be more stable somehow...), so there are ways around it and it's not "a significantly missing piece and the validation is dead otherwise". One that is rather nice to have though, still possible to validate by git repo states. |
||
<< "ID: " << ids_.back()->name() << "\n" | ||
<< "The expected md5: " << idMD5 << " does not match the md5\n" | ||
<< "calculated by the ID: " << calculated_md5 << " please\n" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,7 +41,7 @@ PixelVertexProducer::PixelVertexProducer(const edm::ParameterSet& conf) | |
track_pt_min = PVcomparerPSet.getParameter<double>("track_pt_min"); | ||
if (track_pt_min != ptMin_) { | ||
if (track_pt_min < ptMin_) | ||
edm::LogWarning("PixelVertexProducer") << "minimum track pT setting differs between PixelVertexProducer (" << ptMin_ << ") and PVcomparer (" << track_pt_min << ") [PVcomparer considers tracks w/ lower threshold than PixelVertexProducer does] !!!"; | ||
edm::LogInfo("PixelVertexProducer") << "minimum track pT setting differs between PixelVertexProducer (" << ptMin_ << ") and PVcomparer (" << track_pt_min << ") [PVcomparer considers tracks w/ lower threshold than PixelVertexProducer does] !!!"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess @silviodonato can comment... if this is firing we have to fix the config I guess... can you clarify in which context was it firing too often? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can check, e.g. 134.702 in step2 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it happens once per job There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @silviodonato can you have a look? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @davidlange6 are we going to make 5 billion jobs for 5 billion events? that's inefficient :-) This Warning is at construction time right? so it complains once per job at most... I guess config warning should be print once per job ... we have no way to print "once per production campaign" given that jobs do not talk with each other There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure - but why keep warnings around for years? Its not “new"
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Andrea
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. because nobody complained/noticed it earlier, and the fix is "fix the config". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it seems we now agree the printout has accomplished its goal. if things return back to the state where it indicates a real unknown problem, it would then be good to reenable by default as a warning. Meanwhile we can more easily see other warnings
|
||
else | ||
edm::LogInfo("PixelVertexProducer") << "minimum track pT setting differs between PixelVertexProducer (" << ptMin_ << ") and PVcomparer (" << track_pt_min << ") !!!"; | ||
} | ||
|
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.
Again, rather than changing the LogPrint to a LogInfo, why not change it to a LogWarning and fix the configuration ?
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.
Sounds great. as I said in the PR itself "if these are considered bugs, we can reenable them together with the bug fix..” Maybe @deguio wants to comment on the interest of the developers to fix