Skip to content
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

Sequence-based simplified HLT phase2 menu. #44025

Merged
merged 5 commits into from
Mar 11, 2024

Conversation

rovere
Copy link
Contributor

@rovere rovere commented Feb 20, 2024

PR description:

This (large) PR is a rewrite of the Simplified Phase2 HLT Menu using sequences and abandoning the cms.ConditionalTasks almost completely. The notable exceptions are the Muons paths for which more expert help is needed to perform this transition.

The timing variant of the menu has been updated to include the recent move to L1P2GT and converted to using sequences as well.

Some of the pset configurations have been included in the PR even if they contain 0 changes. This is due to how the tool edmConfigSplit internally dumps those psets. Rather than chasing all of them one by one, I decided to let it do its job.

Unused modules have been removed. If needed, they could be extracted from git.

PR validation:

Run before/after this PR on some 100 events and the results are identical.

All paths have been tested one-by-one to verify that they could run alone, w/o any other paths present.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 20, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44025/38955

  • This PR adds an extra 400KB to repository

  • Found files with invalid states:

    • HLTrigger/Configuration/python/HLT_75e33/modules/simSiStripDigis_cfi.py:
    • HLTrigger/Configuration/python/fullDumpPhase2_HLT_OriginalNoTimingGeneric.py:
    • HLTrigger/Configuration/python/HLT_75e33/modules/simSiPixelDigis_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/modules/simEcalUnsuppressedDigis_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/modules/simHFNoseUnsuppressedDigis_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/psets/options_cfi.py:
    • 24834.0_TTbar_14TeV+2026D98/genericPhase2SinglePath.py:
    • HLTrigger/Configuration/python/HLT_75e33/psets/maxEvents_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/modules/randomEngineStateProducer_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/psets/MEtoEDMConverterRECO_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/modules/simAPVsaturation_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/modules/simHGCalUnsuppressedDigis_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/modules/simHcalUnsuppressedDigis_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/services/DependencyGraph_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/tasks/patAlgosToolsTask_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/psets/MEtoEDMConverterFEVT_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/modules/simCastorDigis_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/paths/endjob_step_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/psets/MEtoEDMConverterAOD_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/sequences/HLTDiphoton3024IsoCaloIdUnseededSequence_cfi.py:

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44025/38956

  • This PR adds an extra 396KB to repository

  • Found files with invalid states:

    • HLTrigger/Configuration/python/HLT_75e33/source_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/modules/simSiStripDigis_cfi.py:
    • HLTrigger/Configuration/python/fullDumpPhase2_HLT_OriginalNoTimingGeneric.py:
    • HLTrigger/Configuration/python/HLT_75e33/modules/simSiPixelDigis_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/modules/simEcalUnsuppressedDigis_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/modules/simHFNoseUnsuppressedDigis_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/psets/options_cfi.py:
    • 24834.0_TTbar_14TeV+2026D98/genericPhase2SinglePath.py:
    • HLTrigger/Configuration/python/HLT_75e33/psets/maxEvents_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/modules/randomEngineStateProducer_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/psets/MEtoEDMConverterRECO_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/modules/simAPVsaturation_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/modules/simHGCalUnsuppressedDigis_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/modules/simHcalUnsuppressedDigis_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/services/DependencyGraph_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/tasks/patAlgosToolsTask_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/psets/MEtoEDMConverterFEVT_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/modules/simCastorDigis_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/paths/endjob_step_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/psets/MEtoEDMConverterAOD_cfi.py:
    • HLTrigger/Configuration/python/HLT_75e33/sequences/HLTDiphoton3024IsoCaloIdUnseededSequence_cfi.py:

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @rovere for master.

It involves the following packages:

  • 24834.0_TTbar_14TeV+2026D98/genericPhase2SinglePath.py (****)
  • HLTrigger/Configuration (hlt)

The following packages do not have a category, yet:

24834.0_TTbar_14TeV+2026D98/genericPhase2SinglePath.py
Please create a PR for https://github.com/cms-sw/cms-bot/blob/master/categories_map.py to assign category

@cmsbuild, @mmusich, @Martin-Grunewald can you please review it and eventually sign? Thanks.
@SohamBhattacharya, @wang0jin, @thomreis, @Martin-Grunewald, @silviodonato, @argiro, @rchatter, @ReyerBand, @missirol this is something you requested to watch as well.
@rappoccio, @sextonkennedy, @antoniovilela you are the release manager for this.

cms-bot commands are listed here

@rovere
Copy link
Contributor Author

rovere commented Feb 20, 2024

@cmsbuild please test

@rovere
Copy link
Contributor Author

rovere commented Feb 20, 2024

I'm not sure why the bot is convinced that we need a new package. The file he is complaining about is part of the git history (and was needed while developing the PR), but it is not part of the final set of changes of this PR.
Ideas?

@Martin-Grunewald
Copy link
Contributor

Remove and force-push?

@mmusich
Copy link
Contributor

mmusich commented Feb 20, 2024

Ideas?

Do we need the whole history? Otherwise squash the commits to 1.

@rovere
Copy link
Contributor Author

rovere commented Feb 20, 2024

Ciao @mmusich @Martin-Grunewald I added to the git history for a reason and would rather have it in.
The history has been already rewritten via several rebases. I would be in favour of not squashing it into 1 single commit.
Besides the bot being confused, is there any other drawback in leaving it in?

@Martin-Grunewald
Copy link
Contributor

Hmm, the bot should not create a 24834.0_TTbar_14TeV+2026D98/ even if empty.
Maybe ask @smuzaffar on what to do here so that the bot gets rid of the new-package-pending status?

@iarspider
Copy link
Contributor

@rovere This PR touches 430 files, thus causing the bot to fail. If this is intentional, I think the best course of action would be to split it this PR two or more - e.g., move 1a2126e into a separate PR and remove it from history. You can use please test with #XYZ syntax to make bot merge additional PRs on top of this one for testing.

@rovere
Copy link
Contributor Author

rovere commented Feb 20, 2024

Ciao @iarspider thanks for your reply.
Can you clarify what do you mean by "causing the bot to fail"?
I'm not willing to split a PR based on the number of files changed.

@rovere This PR touches 430 files, thus causing the bot to fail. If this is intentional, I think the best course of action would be to split it this PR two or more - e.g., move 1a2126e into a separate PR and remove it from history. You can use please test with #XYZ syntax to make bot merge additional PRs on top of this one for testing.

@iarspider
Copy link
Contributor

@rovere In order to reduce the number of GitHub API calls used per PR, the bot keeps a cache of some information regarding the PR (list of all commits, including squashed; list of files changed in each commit; reactions). That information is stored inside a comment, and thus is limited to 65k bytes. The bot will fail, i.e. it will not process a PR, if cache size is too big to be stored.

@cmsbuild
Copy link
Contributor

-1

Failed Tests: RelVals
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-008d13/37571/summary.html
COMMIT: 30478c1
CMSSW: CMSSW_14_1_X_2024-02-19-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/44025/37571/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals

----- Begin Fatal Exception 20-Feb-2024 11:36:31 CET-----------------------
An exception of category 'ProductNotFound' occurred while
   [0] Processing  Event run: 1 lumi: 1 event: 1 stream: 0
   [1] Running path 'MC_Ele5_Open_L1Seeded'
   [2] Calling method for module Phase2TrackerClusterizer/'siPhase2Clusters'
Exception Message:
Principal::getByToken: Found zero products matching all criteria
Looking for type: edm::DetSetVector<Phase2TrackerDigi>
Looking for module label: mix
Looking for productInstanceName: Tracker

   Additional Info:
      [a] If you wish to continue processing events after a ProductNotFound exception,
add "TryToContinue = cms.untracked.vstring('ProductNotFound')" to the "options" PSet in the configuration.

----- End Fatal Exception -------------------------------------------------

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 8, 2024

Pull request has been put on hold by @mmusich
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

@rovere
Copy link
Contributor Author

rovere commented Mar 8, 2024

Ciao @mmusich thanks for these further checks. For completeness, these are the changes I made to the collections (to try and have a uniform approach and naming convention, far from being finalized, yet):

  • ecalRecHit:EcalRecHitsEB $\to$ hltEcalRecHit:EcalRecHitsEB
  • ecalRecHit:EcalRecHitsEE $\to$ hltEcalRecHit:EcalRecHitsEE (useless, it's empty by definition, should be removed in the long run)
  • hfprereco $\to$ hltHfprereco
  • hfreco $\to$ hltHfreco
  • horeco $\to$ hltHoreco

I wrote a Python script that compares the entries (and their members) of these collections before/after the PR, and they are identical.

On a side note, I just realized that the new naming convention is such that the hltHfprereco is not part of the FEVTHLTDEBUG event content, since the rule is keep *_hfprereco_*_*.

Should we consider this as a shower-stopper for this PR to be merged or can we have another PR to fix this?

@mmusich
Copy link
Contributor

mmusich commented Mar 8, 2024

Thanks @rovere for the checks.

I wrote a Python script that compares the entries (and their members) of these collections before/after the PR, and they are identical.

Can you link this from here?

Should we consider this as a shower-stopper for this PR to be merged or can we have another PR to fix this?

Let's go ahead, but please open a bug-fix PR as soon as possible.

@mmusich
Copy link
Contributor

mmusich commented Mar 8, 2024

unhold

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 8, 2024

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. @rappoccio, @sextonkennedy, @antoniovilela (and backports should be raised in the release meeting by the corresponding L2)

@rovere
Copy link
Contributor Author

rovere commented Mar 8, 2024

It is bread and butter, but it helped quite a bit:

link

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 8, 2024

REMINDER @rappoccio, @sextonkennedy, @antoniovilela: This PR was tested with #44041, please check if they should be merged together

@antoniovilela
Copy link
Contributor

Let's move on with this, but for our knowledge: what is automatically generated and what is manually maintained from the 430 files?

@antoniovilela
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit faf732f into cms-sw:master Mar 11, 2024
11 checks passed
@antoniovilela
Copy link
Contributor

Let's move on with this, but for our knowledge: what is automatically generated and what is manually maintained from the 430 files?

@rovere

@mmusich
Copy link
Contributor

mmusich commented Mar 11, 2024

what is automatically generated and what is manually maintained from the 430 files?

we're not using confDB (yet) for phase-2, so this is completely manual (of course I guess @rovere used scripts to adjust and cross-check the configurations) @antoniovilela

@rovere
Copy link
Contributor Author

rovere commented Mar 12, 2024

@antoniovilela I mentioned the script used in the PR description. It is called edmConfigSplit and appropriately splits a fully expanded configuration into files and folders.
All the configurations should be checked and maintained by the corresponding developers.

@antoniovilela
Copy link
Contributor

@antoniovilela I mentioned the script used in the PR description. It is called edmConfigSplit and appropriately splits a fully expanded configuration into files and folders. All the configurations should be checked and maintained by the corresponding developers.

I guess what I wanted to make sure (just because of my current hat in ORP) is that there is no risk of affecting modules that can be used outside of the Phase 2 processing. I guess so, but it does not hurt to confirm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants