-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 globalreco_tracking sequence for Phase2 #27529
Conversation
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27529/10899
|
A new Pull Request was created by @fabiocos (Fabio Cossutti) for master. It involves the following packages: Configuration/StandardSequences @cmsbuild, @franzoni, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test workflow 20434.1 |
The tests are being triggered in jenkins. |
Comparison job queued. |
@@ -140,6 +140,11 @@ | |||
from Configuration.Eras.Modifier_phase2_timing_layer_bar_cff import phase2_timing_layer_bar | |||
(phase2_timing_layer_tile | phase2_timing_layer_bar).toReplaceWith(globalreco,_phase2_timing_layer_globalreco) | |||
|
|||
_phase2_timing_layer_globalreco_tracking = globalreco_tracking.copy() | |||
_phase2_timing_layer_globalreco_tracking += fastTimingGlobalReco | |||
(phase2_timing_layer_tile | phase2_timing_layer_bar).toReplaceWith(globalreco_tracking,_phase2_timing_layer_globalreco_tracking) |
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.
shouldn't the piece above with .toReplaceWith(globalreco,_phase2_timing_layer_globalreco)
be removed?
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.
also, it would be better to modify globalreco_tracking
right after its definition instead of after some other sequences
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.
@slava77 I see your point, as it is now fastTimingGlobalReco
appears twice inside teh globalreco sequence. As far as I can see this module should be independent on the other parts of globalreco, so it should work
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.
@slava77 I agree with both of your comments
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.
from https://github.com/cms-sw/cmssw/pull/27529/files#r303926710
also, it would be better to modify
globalreco_tracking
right after its definition instead of after some other sequences
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.
to be more clear, I'm suggesting to
- move the currently proposed L138,139,140-143 to L122
- remove everything else from L137 to L144
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.
I think you meant 141 - 143, 140 at this point should just go together with 137
assign upgrade |
New categories assigned: upgrade @kpedro88 you have been requested to review this Pull request/Issue and eventually sign? Thanks |
Comparison is ready @slava77 comparisons for the following workflows were not done due to missing matrix map:
Comparison Summary:
|
Comparison job queued. |
@kpedro88 I would like to merge this as soon as the result of the test is out (it does not crash any more, I want to see the comparison). It is one of the last pieces I want to have for 11_0_0_pre4 |
+upgrade |
@slava77 probably you are correct, unfortunately at the time of the initial deployment we were in a hurry and things were not optimal... |
Comparison is ready @slava77 comparisons for the following workflows were not done due to missing matrix map:
Comparison Summary:
|
+operations this update allows the globalreco_tracking sequence to be used also for phase2 scenarios with MTD, without changing the behaviour of the globalreco sequence (only the calling order of the modules is modified) |
+1 |
@mrodozov is the bot stuck on this PR? |
@fabiocos would you try again +operations and +1 please |
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 be automatically merged. |
+operations |
+1 |
@mrodozov although the labels were already correctly set to signed... |
yes it did right after I commented it changed them :/ |
PR description:
Minimal update to the globalreco_tracking sequence for Phase2 in order to include MTD. This is needed by the new test wf 20434.1, so far systematically failing because the sequence is not running the needed module:
See issue #27507 for details.
PR validation:
Test workflow 20434.1 runs to completion without apparent issues.