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

Fix parameters in PPS RawToDigiConverter, and clean up a few related configs #41785

Closed

Conversation

perrotta
Copy link
Contributor

@perrotta perrotta commented May 28, 2023

PR description:

(This replaces #41784 that I somehow messed up)

Fix a few parameters in PPS RawToDigiConverter:

  • Make "useOlderT2TestFile" tracked
  • Make "useOlderT2TestFile", "printErrorSummary", "printUnknownFrameSummary" bool parameters, instead of unsigned int
  • Propagate those modified parameters in the corresponding python configs

While doing so I also:

  • Made constant a few class members of RawToDigiConverter that should have been so
  • Moved to a safer syntax, with no explicit type declaration, in the touched configs when possible

This is a follow up of the #41777 (comment) posted when merging PR #41777

PR validation:

It builds and a few matrix workflows run

@oljemark @forthommel and https://github.com/orgs/cms-sw/teams/ctpps-dpg-l2 please check that this actually corresponds to what you need (nothing should change in the final results with respect to the code currently merged in the master release). I did not try the totemt2_dqm_test because I miss some input: could you please give it a try and report if you get any error from it?

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41785/35662

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @perrotta (Andrea Perrotta) for master.

It involves the following packages:

  • DQM/CTPPS (dqm)
  • EventFilter/CTPPSRawToDigi (reconstruction)

@micsucmed, @nothingface0, @emanueleusai, @clacaputo, @cmsbuild, @pmandrik, @syuvivida, @tjavaid, @mandrenguyen, @rvenditti can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @missirol, @forthommel, @fabferro, @grzanka this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@oljemark
Copy link
Contributor

Hello, @perrotta . Yesterday I was able to run the fixes you requested (without the code improvements you introduce here) in branch CTPPS/cmssw:fo_nt2_mapfix , which also contained some bugfixes to be able to read newly produced T2 test files from this Friday, so now I extracted only part you asked for to branch master...CTPPS:cmssw:fo_nt2_bugfix2 , for which at least the New T2 unit test in RecoPPS/Local runs successfully. I'll try out your code next.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see some noncapitalized "false" here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @oljemark , fixed now

@perrotta perrotta force-pushed the fixParamInCtppsRawToDigiConverter branch from 36bbd74 to a459714 Compare May 28, 2023 10:15
@perrotta
Copy link
Contributor Author

Hello, @perrotta . Yesterday I was able to run the fixes you requested (without the code improvements you introduce here) in branch CTPPS/cmssw:fo_nt2_mapfix , which also contained some bugfixes to be able to read newly produced T2 test files from this Friday, so now I extracted only part you asked for to branch master...CTPPS:cmssw:fo_nt2_bugfix2 , for which at least the New T2 unit test in RecoPPS/Local runs successfully. I'll try out your code next.

Please fill free to take whatever you want from this PR and use with your own: the "official" @cms-sw/ctpps-dpg-l2 code will supersede this fix when ready, if you think so

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41785/35663

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

Pull request #41785 was updated. @micsucmed, @nothingface0, @emanueleusai, @clacaputo, @cmsbuild, @pmandrik, @syuvivida, @tjavaid, @mandrenguyen, @rvenditti can you please check and sign again.

# if non-zero, prints a per-VFAT error summary at the end of the job
printErrorSummary = cms.untracked.uint32(0),
# prints a per-VFAT error summary at the end of the job
printErrorSummary = cms.untracked.bool(false),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see some here too.

@perrotta perrotta force-pushed the fixParamInCtppsRawToDigiConverter branch from a459714 to c38b64e Compare May 28, 2023 10:44
@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41785/35664

  • This PR adds an extra 24KB to repository

@clacaputo
Copy link
Contributor

+reconstruction

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

@perrotta
Copy link
Contributor Author

hold

@cmsbuild
Copy link
Contributor

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

@perrotta
Copy link
Contributor Author

@oljemark this is now fully signed. Please let us know if it can be merged as such, or if you have another PR ready to be submitted that can supersede this one

oljemark added a commit to CTPPS/cmssw that referenced this pull request May 31, 2023
oljemark added a commit to CTPPS/cmssw that referenced this pull request May 31, 2023
@grzanka
Copy link
Contributor

grzanka commented May 31, 2023

Hi, @perrotta . When I did cms-checkdeps for my own branch fo_nt2_mapfix2 and now yours too I got several unit test failures in DQM/Integration and one in CalibPPS/TimingCalibration , but when I tried in the from-CMSSW_13_2_X_2023-05-28-2300 branch I also got these failures, so it doesn't seem to come from your code, most being unreachable ROOT files?

I think that is an unreachable ROOT file. When running CalibPPS/TimingCalibration/testHptdcPcl test on lxplus, cmsRun tries to access /store/data/Run2022B/AlCaPPS/RAW/v1/000/355/207/00000/c23440f4-49c0-44aa-b8f6-f40598fb4705.root which is on TAPE. The tests fails.

On the other hand, the cmsbot on Jenkins tries to access that file via /eos/cms/store/user/cmsbuild/store/data/Run2022B/AlCaPPS/RAW/v1/000/355/207/00000/c23440f4-49c0-44aa-b8f6-f40598fb4705.root (note /eos/cms/store/user/cmsbuild/ in front) which is available on EOS and test passes.

@oljemark
Copy link
Contributor

oljemark commented Jun 1, 2023

Hi, @perrotta . I've gotten several DQM plot requests and found some additional functionality was needed in the DataFormats/TotemReco, so I'm going with the PR still in CTPPS, #75, with some work still to be done to get it ready by tomorrow.

@perrotta
Copy link
Contributor Author

Already included in (and therefore superseded by) #41859

@perrotta perrotta closed this Jun 10, 2023
@perrotta perrotta deleted the fixParamInCtppsRawToDigiConverter branch June 10, 2023 08:29
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