-
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
Phase-2 Global Trigger Emulator #41808
Conversation
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41808/35693 ERROR: Build errors found during clang-tidy run.
|
f572c36
to
10a7b95
Compare
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41808/35695 ERROR: Build errors found during clang-tidy run.
|
10a7b95
to
00ac291
Compare
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41808/35696 ERROR: Build errors found during clang-tidy run.
|
00ac291
to
d2ea389
Compare
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41808/35753 ERROR: Build errors found during clang-tidy run.
|
This requires #41492 |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-59f559/33830/summary.html Comparison SummarySummary:
|
+l1
|
+Upgrade |
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. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
@makortel @fwyzard At this point I want to ask both of you if you have any advice on the question of a menu parser. I talked about this a bit here: #41808 (comment) and here: #41808 (comment). The complication is that for phase-2, whenever one wants to run the GT now, one has to load a series of modules corresponding to different paths or conditions, create (or load) paths for each of them, and have them attached to the process alongside two other GT components. I.e. to run the GT in a configuration, one must simultaneously specify what the menu is, there is no separation anymore on the questions of "Is the GT emulator running?" and "What is the GT emulator menu?". There is no XML file it reads from. It's all python modules now. So there are two questions I want to try and sort out:
(@qvyz Just tagging you on this, because we had discussed it a little) |
Hi @aloeliger |
Probably. I'll open an issue. |
def collectAlgorithmPaths(process) -> "tuple[cms.Path]": | ||
str_paths = set() | ||
for algorithm in algorithms: | ||
algo_paths = re.sub(r'[()]'," " , algorithm.expression.value()).split() | ||
for algo in algo_paths: | ||
if algo in process.pathNames() : | ||
str_paths.add(algo) | ||
paths = set() | ||
|
||
for str_path in str_paths: | ||
paths.add(getattr(process, str_path)) | ||
|
||
return tuple(paths) |
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.
Unfortunately this approach can potentially return the paths
in a different order on each invocation.
This does happen in production, and results in jobs with a different process hash.
The framework then considers them different configurations and does not fully merge the resulting files; for example, they appear to belong to different runs (even though the run number is the same).
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.
@fwyzard I see, so in principle if the order is deterministic there shouldn't be a problem, right? So wouldn't a simple sorted(str_paths)
then do the trick?
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.
Yes, that should fix the reproducibility problem.
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.
@fwyzard Would you mind either checking whether this indeed solves the issue or pointing me to how I can reproduce it myself before submitting a PR?
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'm not sure.
You could try running the cmsDriver
command used in production a few times, potentially on different machines (like lxplus8
and lxplus9
) and check
- that sometimes you get different ordering with the current code
- that you always get the same order with the fix
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.
Would it be feasible to insert them in the process
using the same order ?
Or it's not meaningful anyway ?
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 suppose it could be ordered by seed number in the menu, but I don't think that is meaningful enough in this context that we truly need to order them that way as paths as well.
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.
Would it be feasible to insert them in the
process
using the same order ? Or it's not meaningful anyway ?
Given that an algorithm can be a combination of multiple paths and in principle a path can be used in multiple algorithms (although it's unclear whether we should deem the latter case bad practice), I'm not entirely sure how meaningful any path order actually is.
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.
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.
@rovere simply used the samples from the official Spring24 production; a test would be to re-produce the datasets.
PR description:
This PR includes the first version of the Phase-2 Global Trigger emulator that implements a bit-wise compatible emulation of the GT firmware. The PR includes the HLT-TDR simplified L1 menu (16 seeds), documented in this twiki: https://twiki.cern.ch/twiki/bin/viewauth/CMS/PhaseIIL1TriggerMenuTools#How_to_use_the_GT_Emulator
As the Phase-2 Global Trigger emulator relies on various upstream emulators, this PR can be merged once:
PR validation:
The expected rates have been presented at the Annual Review (https://indico.cern.ch/event/1257943/#7-algorithms-and-physics-perfo).
-Jaana on behalf of the Phase-2 Global Trigger team