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 PostDQMOffline* sequences in cmsDriver.py #40293

Merged
merged 1 commit into from Dec 13, 2022

Conversation

elusian
Copy link

@elusian elusian commented Dec 12, 2022

PR description:

When adding a DQM sequence to the command line options (e.g. DQM:@miniAODDQM), cmsDriver.py is supposed to add the relative PostDQMOffline sequence as selected in autoDQM.py.
However, due to this commit in CMSSW_12_6_0_pre3, this does not happen anymore.

After the commit sequenceList and postSequenceList are the same object at the same address and filling sequenceList inhibits the fill of postSequenceList (since expandMapping sees no "@" in the array). Since the sequences are the same, no dqmofflineOnPAT_step is produced in the output.

This PR splits the assigment into two lines, which makes sequenceList and postSequenceList distinct again.

PR validation:

Created a simple config file passing DQM:@miniAODDQM to cmsDriver.py, requested PostDQMOffline* sequences are back in the process.schedule as they were in CMSSW_12_6_0_pre2

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:

Not a backport

@mmusich
Copy link
Contributor

mmusich commented Dec 12, 2022

type bug-fix

@mmusich
Copy link
Contributor

mmusich commented Dec 12, 2022

@vlimant FYI

@perrotta
Copy link
Contributor

code-checks

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40293/33357

  • This PR adds an extra 36KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • Configuration/Applications (operations)

@cmsbuild, @perrotta, @rappoccio, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks.
@makortel, @missirol, @fabiocos, @Martin-Grunewald 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

@mmusich
Copy link
Contributor

mmusich commented Dec 12, 2022

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

-1

Failed Tests: UnitTests
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6dfdd5/29576/summary.html
COMMIT: 0bd33ff
CMSSW: CMSSW_13_0_X_2022-12-12-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/40293/29576/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found errors in the following unit tests:

---> test TestConfigDP had ERRORS

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 52 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3421510
  • DQMHistoTests: Total failures: 1238
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3420250
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 76941.71699999996 KiB( 47 files compared)
  • DQMHistoSizes: changed ( 10024.0,... ): 640.710 KiB JetMET/PFCandidates
  • DQMHistoSizes: changed ( 10024.0,... ): 448.898 KiB ParticleFlow/slimmedJetValidation
  • DQMHistoSizes: changed ( 10024.0,... ): 263.359 KiB ParticleFlow/slimmedElectronValidation
  • DQMHistoSizes: changed ( 10024.0,... ): 261.354 KiB ParticleFlow/slimmedMuonValidation
  • DQMHistoSizes: changed ( 10024.0,... ): 239.348 KiB ParticleFlow/slimmedMETValidation
  • DQMHistoSizes: changed ( 10024.0,... ): 239.010 KiB Physics/Top
  • DQMHistoSizes: changed ( 10024.0,... ): 236.857 KiB JetMET/Jet
  • DQMHistoSizes: changed ( 10024.0,... ): 198.366 KiB JetMET/MET
  • DQMHistoSizes: changed ( 10024.0,... ): 37.338 KiB ParticleFlow/PFJetResValidation
  • DQMHistoSizes: changed ( 10024.0,... ): 24.309 KiB Tracking/PackedCandidate
  • DQMHistoSizes: changed ( 13234.0 ): ...
  • Checked 206 log files, 158 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@perrotta
Copy link
Contributor

  • Unit test errors are also in the IB, and clearly unrelated
  • There are twice the entries in the JetMET DQM plots, e.g for 4.53

image
Is it understood?

@elusian
Copy link
Author

elusian commented Dec 13, 2022

It seems that all JetAnalyzer plugins share that plot (https://github.com/cms-sw/cmssw/blob/master/DQMOffline/JetMET/src/JetAnalyzer.cc#L1684-L1698).
4 of those plugins are contained in the DQMOffline sequence so you see them both in reference and target, while 4 are in the PostDQMOfflineMiniAOD, which we are adding back with this PR. Twice more plugins filling the same plot leads to twice the entries.
Whether that is intended I do not know, but the difference is expected in this PR.

The comparisons are not available anymore, but I would expect #39337 to have shown the opposite change.

@perrotta
Copy link
Contributor

Thank you @elusian
Not crystal clear to me... Maybe @cms-sw/dqm-l2 can have a look and suggest us how to deal with it.

@perrotta
Copy link
Contributor

assign dqm

@perrotta
Copy link
Contributor

urgent

@cmsbuild
Copy link
Contributor

New categories assigned: dqm

@jfernan2,@ahmad3213,@micsucmed,@rvenditti,@emanueleusai,@syuvivida,@pmandrik you have been requested to review this Pull request/Issue and eventually sign? Thanks

@mmusich
Copy link
Contributor

mmusich commented Dec 13, 2022

Not crystal clear to me... Maybe https://github.com/orgs/cms-sw/teams/dqm-l2 can have a look and suggest us how to deal with it.

In my opinion, instead of assign DQM signature here, opening a gitHub issue and assigning JME POG conveners would be more appropriate. The duplication of JME plots filling looks like a bug (or at least a funny feature), but this is only tangentially related to the issue at hand.
This PR is necessary to restore the correct production of miniAOD monitoring histograms (e.g. without this there is no tracking packed candidate monitoring at all, see e.g.

DQMHistoSizes: changed ( 10024.0,... ): 24.309 KiB Tracking/PackedCandidate

at #40293 (comment)), and it's lack is already causing delays (see e.g. https://its.cern.ch/jira/browse/PDMVRELVALS-172 )

@perrotta
Copy link
Contributor

at #40293 (comment)), and it's lack is already causing delays (see e.g. https://its.cern.ch/jira/browse/PDMVRELVALS-172 )

Ciao @mmusich

I agree about the need of speeding up the integration of this fix. In any case, the integration in the release is quantizied by the time of the IBs: whatever is merged between now and 11pm will end up in the 2300 master IB. And when the results of that IB tests will become available, likely tomorrow morning, we can merge the backport ones, which are the ones you are mostly aiming at.

We have an orp meeting in a few hours from now. Hopefully all the stakeholders (@vlimant @cms-sw/dqm-l2 @cms-sw/jetmet-pog-l2) will be there to address the issue reported. In particular, I look forward earing something from Jean-Roch, whose original PR implemented the modification which is reverted here. If anyone of them is missing there, we will proceed without, as we always do in case of need for the PRs flagged as "urgent".

Therefore, please be reassured that asking for the opinion of the supposedy relevant experts will not delay the integration of this PR.

@emanueleusai
Copy link
Member

emanueleusai commented Dec 13, 2022

Hello, yes, I'd say something fishy is going on here, but it's a separate issue to be addressed by JME DQM developers.
On the other hand I recognize the urgency to have this fix in asap, so I am ok merging this as it is

@emanueleusai
Copy link
Member

+1

@perrotta
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (but tests are reportedly failing).

@perrotta
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit 9bdff61 into cms-sw:master Dec 13, 2022
vlimant referenced this pull request Dec 14, 2022
…fic cff> and using the default sequence and various custom
@vlimant
Copy link
Contributor

vlimant commented Dec 14, 2022

I don't see how anything here could create any of the plot difference you've observed, or would harm nano

@perrotta
Copy link
Contributor

I don't see how anything here could create any of the plot difference you've observed, or would harm nano

Thank you @vlimant for the confirmation!

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

6 participants