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

Several updates to SiPixel LA PCL workflow #36538

Merged
merged 7 commits into from Dec 21, 2021

Conversation

mmusich
Copy link
Contributor

@mmusich mmusich commented Dec 17, 2021

PR description:

Miscellanea updates and bug-fixes to the SiPixel LA PCL workflow that are built on top of the replay at dmwm/T0#4635:

  • made the processing much faster by calling SiPixelTemplate::pushfile in dqmBeginRun instead of the analyze method;
  • add basic accepted tracks monitoring;
  • adding plot titles;
  • re-organize folders to have subfolders: (Top) / (BPix) / etc. ..;
  • actually do write to DB the values for the "new" modules when they are fit;
  • change the default cut value on the fit probability for writing out the payload from 0.5 to 0.1;
  • fix a bug that was causing populating the lists of "new" modules and modules attached to a given "sector" multiple times (this was triggering several warnings in the harvesting step when populating the DB about multiple puts, see CondFormats: fix warning message about "skipping this put" #36535);

PR validation:

Run:

 cmsDriver.py testReAlCa -s ALCA:PromptCalibProdSiPixelLorentzAngle --conditions 121X_dataRun3_Express_TIER0_REPLAY_Run2_v1 --scenario pp --data --era Run2_2018 --datatier ALCARECO --eventcontent ALCARECO --processName=ReAlCa -n 100 --dasquery='file dataset=/StreamExpress/Tier0_REPLAY_2021-SiPixelCalSingleMuon-Express-v1/ALCARECO' --customise_commands='process.ALCARECOCalSignleMuonFilterForSiPixelLorentzAngle.TriggerResultsTag = cms.InputTag ( "TriggerResults","","HLT" ) ; process.ALCARECOCalSignleMuonFilterForSiPixelLorentzAngle.HLTPaths = ["*"]' --nThreads=4 

followed by:

 cmsDriver.py stepHarvest -s ALCAHARVEST:SiPixelLA --conditions 121X_dataRun3_Express_TIER0_REPLAY_Run2_v1 --scenario pp --data --era Run2_2018 --filein file:PromptCalibProdSiPixelLorentzAngle.root -n -1

the results are uploaded to a private DQM GUI at https://tinyurl.com/y4dm6s98:

  • necessitates tunneling (ssh -NL 8060:localhost:8060 <USER>@lxplus723.cern.ch).

An example of the result is link to the companion PR to dmwm: dmwm/deployment#1118

if this PR is a backport please specify the original PR and why you need to backport that PR:

Not a backport, but might be backported

@mmusich
Copy link
Contributor Author

mmusich commented Dec 17, 2021

test parameters:

  • workflow = 1001.2

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36538/27443

  • This PR adds an extra 24KB to repository

  • There are other open Pull requests which might conflict with changes you have proposed:

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @mmusich (Marco Musich) for master.

It involves the following packages:

  • CalibTracker/SiPixelLorentzAngle (alca)

@cmsbuild, @malbouis, @tvami, @yuanchao, @francescobrivio can you please review it and eventually sign? Thanks.
@tocheng, @OzAmram, @ferencek, @mmusich, @dkotlins, @tvami this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@mmusich
Copy link
Contributor Author

mmusich commented Dec 17, 2021

While running the debugging I noticed that the module SiPixelLorentzAnglePCLWorker was extremely slow.
This was due to calling SiPixelTemplate::pushfile in the analyze method at each event.
Moving it to dqmBeginRun in the last commit (47a4146) there's a significant speed-up of the amount of time passed in SiPixelLorentzAnglePCLWorker.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36538/27452

  • This PR adds an extra 28KB to repository

  • There are other open Pull requests which might conflict with changes you have proposed:

@cmsbuild
Copy link
Contributor

Pull request #36538 was updated. @cmsbuild, @malbouis, @tvami, @yuanchao, @francescobrivio can you please check and sign again.

@mmusich
Copy link
Contributor Author

mmusich commented Dec 17, 2021

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

Pull request #36538 was updated. @cmsbuild, @malbouis, @tvami, @yuanchao, @francescobrivio can you please check and sign again.

@mmusich
Copy link
Contributor Author

mmusich commented Dec 18, 2021

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f2270c/21387/summary.html
COMMIT: c54e06e
CMSSW: CMSSW_12_3_X_2021-12-18-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36538/21387/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /pool/condor/dir_226508/jenkins/workspace/compare-root-files-short-matrix/data/PR-f2270c/1001.2_RunZeroBias2017F+RunZeroBias2017F+TIER0EXPRUN2+ALCAEXPRUN2+ALCAHARVDSIPIXELCAL+ALCAHARVDSIPIXELCALLA

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 15 differences found in the comparisons
  • DQMHistoTests: Total files compared: 43
  • DQMHistoTests: Total histograms compared: 3461559
  • DQMHistoTests: Total failures: 22
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3461514
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 3.348 KiB( 42 files compared)
  • DQMHistoSizes: changed ( 1001.0 ): 3.352 KiB AlCaReco/SiPixelLorentzAngle
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 181 log files, 42 edm output root files, 43 DQM output files
  • TriggerResults: no differences found

@tvami
Copy link
Contributor

tvami commented Dec 18, 2021

+alca

  • tests pass, diffs in MsgLogger only
  • code changes are in line with the PR description
  • I think we should do a backport to 12_1_X, @mmusich

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

@mmusich
Copy link
Contributor Author

mmusich commented Dec 19, 2021

@tvami

I think we should do a backport to 12_1_X

sure, though we might want to wait for the rest of the developments suggested at #36538 (comment) to land to avoid multiple backports.
Also, since in my understanding 12_2_0 should be available in a few weeks (beginning of 2022 IIUC), if the intention is to make the updates available for further Tier-0 replays, wouldn't suffice to backport to 12_2_X instead?

@mmusich mmusich closed this Dec 19, 2021
@mmusich mmusich reopened this Dec 19, 2021
@tvami
Copy link
Contributor

tvami commented Dec 19, 2021

wouldn't suffice to backport to 12_2_X instead?

Yes, you are right, that's indeed sufficient

@mmusich
Copy link
Contributor Author

mmusich commented Dec 21, 2021

@cms-sw/orp-l2 is there anything holding up this PR?

@perrotta
Copy link
Contributor

@mmusich , as there is no need to affix any sort of endl to the LogMessages [1], would you mind cleaning the code in SiPixelLorentzAnglePCLHarvester.cc from them? (They are likely remnants of when those messages were cout's)

[1] https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideMessageLogger

@mmusich
Copy link
Contributor Author

mmusich commented Dec 21, 2021

@perrotta

as there is no need to affix any sort of endl to the LogMessages [1], would you mind cleaning the code in SiPixelLorentzAnglePCLHarvester.cc from them? (They are likely remnants of when those messages were cout's)

As far as I can tell there's no functional difference in having them or not, so I take your comment is purely aesthetical (matter of taste).
In any case, can it be done in a follow-up PR?
This one delivers a rather major performance improvement and there is more to come soon.

@perrotta
Copy link
Contributor

As far as I can tell there's no functional difference in having them or not, so I take your comment is purely aesthetical (matter of taste). In any case, can it be done in a follow-up PR? This one delivers a rather major performance improvement and there is more to come soon.

Yes, aestetical (even not just "matter of taste", since clean code is always easier to read and maintain): "would you mind" in my message was exactly referring to that.
I agree that this PR can be also merged as such, and I also don't urge to have those updates done.

@perrotta
Copy link
Contributor

+1

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.

None yet

4 participants