Updating the TICL offline NanoAOD - #51725
Conversation
|
cms-bot internal usage |
|
A new Pull Request was created by @Moanwar for master. It involves the following packages:
@Moanwar, @battibass, @cmsbuild, @ftorrresd, @jfernan2, @mandrenguyen, @srimanob can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
test parameters:
|
|
@cmsbuild please test |
does it make sense to add this also in the HLT nano? |
Sure, that’s indeed the plan. I started with the offline workflow and will propagate these changes to HLT as well. The HLT NanoAOD already contains some of these branches, such as GEN information and general tracks, but I’ll make the corresponding updates there too. |
|
-1 Failed Tests: RelVals-AMD_MI300X DAS Queries: The DAS query tests failed, see the summary page for details. Failed RelVals-AMD_MI300X
Comparison SummarySummary:
NVIDIA_H100 Comparison SummarySummary:
NVIDIA_L4 Comparison SummarySummary:
NVIDIA_L40S Comparison SummarySummary:
NVIDIA_T4 Comparison SummarySummary:
|
|
Well, the error is not related to this PR, but I’m wondering, for my understanding, why the GPU test is enabled. Maybe I missed something, and it will be enabled by default now? @smuzaffar |
I suspect it's coming from cms-sw/cms-bot#2825, but I am not sure if any of the packages touched here qualifies? |
Okay, I see. Yeah, I wasn’t aware of that, thanks! All the packages here should be GPU-unrelated. Maybe @smuzaffar could cross-check? |
|
@Moanwar @mmusich , bot decided to enable GPU tests as it found direct cuda/rocm/alpaka dependency in one of the packages touch by this PR e.g I see in Jenkins logs and of course there is alpaka dependency https://github.com/cms-sw/cmssw/blob/master/RecoHGCal/TICL/plugins/BuildFile.xml#L44 |
In my opinion this is a bit too open. Shouldn't the bot check before if the plugin touched is actually an alpaka one or not? |
this is too fine graind detail which bot does not know at the time it has to decide it GPU tests should be enabled or not |
Thanks a lot @smuzaffar for checking. I see the point, but this would still lead to longer test times when the changes don’t actually need to be tested on the GPU, and could potentially trigger unrelated errors as well. Do you think there’s a way to adapt this? |
|
ignore tests-rejected with ib-failure
|
|
+1 |
|
@cms-sw/xpog-l2 , can you please have a look ? |
|
@cms-sw/xpog-l2 , can you please have a look and sign ? |
|
enable nano |
|
please test |
|
+1 Size: This PR adds an extra 20KB to repository Comparison SummarySummary:
Nano size comparison Summary:
AMD_MI300X Comparison SummarySummary:
AMD_W7900 Comparison SummarySummary:
NANO Comparison SummarySummary:
NVIDIA_H100 Comparison SummarySummary:
NVIDIA_L4 Comparison SummarySummary:
NVIDIA_L40S Comparison SummarySummary:
NVIDIA_T4 Comparison SummarySummary:
Max Memory Comparisons exceeding threshold NVIDIA_T4@cms-sw/core-l2 , I found 1 workflow step(s) with memory usage exceeding the error threshold: Expand to see workflows ...
|
|
+1 |
cmsbuild
added
fully-signed
xpog-approved
and removed
pending-signatures
xpog-pending
labels
|
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. @ftenchini, @mandrenguyen, @sextonkennedy (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
| lambdaErr=Var("lambdaError()", "float", doc="track lambda error"), | ||
| qoverpErr=Var("qoverpError()", "float", doc="q/p error"), | ||
| ), | ||
| externalVariables = cms.PSet( |
There was a problem hiding this comment.
when trying to port this in the HLT nano (as discussed at #51725 (comment)), @JanGerritSchulz made me realize that the encoding of this information is done perhaps in a memory-inefficient way. Because if I understand correctly, every track gets a bunch of muon-specific values. Most of which will be set to invalid -1. Instead, one could add the information to the muon and add the track index on top. This would effectively add the same information, no?
There was a problem hiding this comment.
Okay, so you propose saving the muon info separately instead, and having the same flags and track index stored for each muon, right?
I’m not sure yet how much memory this would actually save. I did it this way so that the muon info could be accessed directly from the track, and having -1 as the track index is also useful information to have. However, this could still be handled separately oustide.
I can run some tests to see how efficient this would be and whether it would complicate things. I’ll check and let you know.
There was a problem hiding this comment.
Considering there are perhaps few hundreds (even thousands) of tracks per event and only some (<10) muons,I think that adding the track index to the few muon per event should save quite a bit of space.
There was a problem hiding this comment.
Probably, yes. I will test the change and follow up with a PR.
7 participants
This PR adds more information to the offline TICL NanoAOD, with features aimed at improving TICL performance studies, especially for general tracks and GSF tracks for PF interpretation.
It also adds:
The PR has been tested with the relevant workflows and runs smoothly for 34506.213 and 34706.213.