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

Add L1 Upgrade Workflow: 0.78 #43271

Merged
merged 5 commits into from
Nov 22, 2023
Merged

Conversation

aloeliger
Copy link
Contributor

PR description:

This PR creates an L1 workflow that includes the recently added Phase 2 L1 GT Emulator (and related cmsDriver step from #43210) for testing of L1 upgrade sequences and PRs. The workflow should run a modified version of the Digi and DigiTrigger steps with the GT emulator inserted, and it should be available on 2026 workflows. @srimanob This should provide the L1 testing workflow we have been discussing, please let me know if you would like/were envisioning something else.

L1 would also like to explore whether this workflow, or a similar workflow idea can be used for validation of proposed to changes to L1, especially from the physics effects/menu effects perspective.

I don't know if this is the optimal solution for the testing that has been requested from L1, or if it will fully suit other L1 uses for it, so I would like some outside input on whether this is a good solution for being able to test the L1 steps going forward versus say, something like modifying the in place Digi/DigiTrigger steps, and having these L1 steps be in place "by default". Also, this runs redundant steps from the L1 point of view... Anything past step2 is not L1, so as far as testing L1 exclusively, seems like wasted work, but I am not sure how I might change that, or if it is within my ability to do so, let alone whether it is a good idea.

I don't know if there is a policy on naming/numbering these workflows. I didn't see anything relevant in the README.md, so I more or less chose .1001 at random. If this needs to be changed, no issue, it is not used in anything else that we plan to commit at the moment.

@mcepeda & @artlbv I figure you might like to be informed about this, my hope is that eventually this can serve as the basis of a way to do more automatic L1 software validation and testing. Perhaps we can discuss this at some point.

PR validation:

This PR has been tested by running two added workflows via the commands:

  • runTheMatrix.py --what upgrade -l 20834.1001 for an older and more used geometry
  • runTheMatrix.py --what upgrade -l 26834.1001 for a more recent geometry

Both workflows manage to run through step2, with the Digi or DigiTrigger steps, but, in full disclosure, step3 fails on the RAW2DIGI step, complaining about no FEDRawData collection.

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

This PR is not a backport, and likely will not need backporting.

Should run for Digi or DigiTrigger steps, and on 2026 keys. Will include the Phase 2 GT emulator
@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43271/37674

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @aloeliger (Andrew Loeliger) for master.

It involves the following packages:

  • Configuration/PyReleaseValidation (upgrade, pdmv)

@miquork, @AdrianoDee, @srimanob, @sunilUIET, @cmsbuild can you please review it and eventually sign? Thanks.
@missirol, @makortel, @fabiocos, @slomeo, @Martin-Grunewald 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

@srimanob
Copy link
Contributor

Hi @aloeliger
Thanks very much for PR. Just to understand a bit, does the HLT in the release can use L1P2GT out of box? For example, will it work if you run
DIGI:pdigi_valid,L1,L1TrackTrigger,L1P2GT,DIGI2RAW,HLT:@relval2026
instead of
DIGI:pdigi_valid,L1,L1TrackTrigger,L1,DIGI2RAW,HLT:@relval2026

Few comments for now:

  • I think you need only DigiTrigger. We don't use Digi step in Phase-2
  • For the workflow number, I would prefer to assign number closed to .76 (Phase-2 HLT, and very close workflow to this modification). Maybe .78
  • Please update also README

@aloeliger
Copy link
Contributor Author

Hi @aloeliger Thanks very much for PR. Just to understand a bit, does the HLT in the release can use L1P2GT out of box? For example, will it work if you run DIGI:pdigi_valid,L1,L1TrackTrigger,L1P2GT,DIGI2RAW,HLT:@relval2026 instead of DIGI:pdigi_valid,L1,L1TrackTrigger,L1,DIGI2RAW,HLT:@relval2026

@srimanob Technically it will not crash because HLT has been simulating/providing their own non genuine GT inputs for a while now, but HLT hasn't attempted (to my knowledge) to officially move any inputs over to the GT emulator in preparation for this. i.e. I think you could run those steps, but I don't think you would be getting what you want, which is HLT run off of a GT emulator input for it's first step. I haven't actually tested this with the HLT step in place, but I guess I could add it if desired and see what we get. I think HLT experts would need to comment on what their plans are to provide upgrade changes to inputs for complete trigger workflows.

Few comments for now:

  • I think you need only DigiTrigger. We don't use Digi step in Phase-2
  • For the workflow number, I would prefer to assign number closed to .76 (Phase-2 HLT, and very close workflow to this modification). Maybe .78
  • Please update also README

I have removed the Digi and implemented the other changes.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43271/37680

@cmsbuild
Copy link
Contributor

Pull request #43271 was updated. @AdrianoDee, @srimanob, @cmsbuild, @sunilUIET, @miquork can you please check and sign again.

@srimanob
Copy link
Contributor

Could you plese try to run the complete sequence, even HLT result is wrong:
DIGI:pdigi_valid,L1,L1TrackTrigger,L1P2GT,DIGI2RAW,HLT:@relval2026

This will allow us to run the full workflow, including RECO and also DQM. It will open a chance to develop Phase-2 L1T DQM. I don't think we will compare HLT at this point, but (with this .78 workflow) it will also allow us to test if updated HLT code can run on L1P2GT.

Thanks very much.

@aloeliger aloeliger changed the title Add L1 Upgrade Workflow: 0.1001 Add L1 Upgrade Workflow: 0.78 Nov 14, 2023
@aloeliger
Copy link
Contributor Author

@srimanob Trying to add the HLT step into this by itself just errors out, but adding it and DIGI2RAW raw step gets all steps functioning. I have added both into the workflow.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43271/37691

@srimanob
Copy link
Contributor

@srimanob Trying to add the HLT step into this by itself just errors out, but adding it and DIGI2RAW raw step gets all steps functioning. I have added both into the workflow.

@aloeliger
Right, they need to come together. I just trigger the test of new workflow, let's see the result.
@artlbv
To confirm, currently, FEVTDEBUGHLT contain all what you need right?

Thx.

@srimanob
Copy link
Contributor

FYI @SohamBhattacharya @rovere
This PR introduce a new workflow with L1P2GT in sequence.

@artlbv
Copy link
Contributor

artlbv commented Nov 15, 2023

@srimanob

To confirm, currently, FEVTDEBUGHLT contain all what you need right?

Well, for the menu studies we never used the FEVTDEBUG except for doing the edmDump for checking the new collections were produced (even though it seems that not everything is present there).

I think our HLT experts @SohamBhattacharya and @rovere were making more use of that, though of course not using all the collections we have in the "full step-1 L1 menu".

Given that we are working on Nano for the Phase-2 L1 Menu ntuples (https://github.com/cms-l1-dpg/Phase2-L1Nano) I think we could actually check whether we could produce these from the FEVTDEBUG. Would this be a good test?

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b34f90/35835/summary.html
COMMIT: 854b496
CMSSW: CMSSW_14_0_X_2023-11-14-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/43271/35835/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 184 lines from the logs
  • Reco comparison results: 136 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3363070
  • DQMHistoTests: Total failures: 1792
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3361256
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 214 log files, 167 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@srimanob
Copy link
Contributor

Hi @aloeliger
The PR runs fine. I think it is good to go. One last question, should the new workflow be tested when new Phase-2 L1T PRs come?

@srimanob
Copy link
Contributor

+Upgrade

New workflow with L1P2GT runs fine.

@aloeliger
Copy link
Contributor Author

Hi @aloeliger
The PR runs fine. I think it is good to go. One last question, should the new workflow be tested when new Phase-2 L1T PRs come?

@srimanob Yes. There aren't any plots to check, but at the least it will make sure everything still runs correctly, which I think is not a bad thing to test for.

@srimanob
Copy link
Contributor

Kindly ping @cms-sw/pdmv-l2 as this PR will be useful for future Phase-2 L1T PR. Thanks.

@srimanob
Copy link
Contributor

Ping again @cms-sw/pdmv-l2
Thanks.

@sunilUIET
Copy link
Contributor

+pdmv

@cmsbuild
Copy link
Contributor

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

@antoniovilela
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 314acbb into cms-sw:master Nov 22, 2023
12 checks passed
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.

6 participants