Fix when neither "data" nor "MC" is specified in the cmsDriver. #34452

srimanob · 2021-07-13T07:07:56Z

PR description:

This PR is the first draft to fix the issue reported in
#34443

It is #34302 (which is currently reverted) with additional fix to make DQM/validation test on IB work. Detail is in #34443

PR validation:

Run successfully with
scram b runtests_TestDQMOfflineConfiguration_110

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

Not a backport, and no need a backport at the moment.

smuzaffar · 2021-07-13T07:11:19Z

test parameters:

addpkg = DQMOffline/Configuration

cmsbuild · 2021-07-13T07:14:54Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34452/23889

This PR adds an extra 40KB to repository

cmsbuild · 2021-07-13T07:15:13Z

A new Pull Request was created by @srimanob (Phat Srimanobhas) for master.

It involves the following packages:

Configuration/Applications (operations)

@cmsbuild, @silviodonato, @qliphy, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks.
@makortel, @Martin-Grunewald, @fabiocos this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy, @perrotta you are the release manager for this.

cms-bot commands are listed here

smuzaffar · 2021-07-13T07:36:35Z

please test

perrotta · 2021-07-13T07:40:44Z

Configuration/Applications/python/cmsDriverOptions.py

+        # This is a temporary solution to make validation-only sequence work in IB. One should define workflow properly.
+        if options.era and 'Phase2' in options.era:
+            options.isMC=True
+        if 'miniAODValidation' in options.step:


It was counter-intuitive to me that all miniAODValidation must be considered as "MC": of course, we produce miniAOD also from real data.
Then I realized that the miniAODValidationSequence only includes a genParticlesValidation: therefore, the fix here makes sense... but the name of the sequence probably not!
@hatakeyamak @jfernan2 : is there any (hystorical) reason for it?

I've never seen that we use miniAODValidationSequence with data before. In UL, PdmV uses DQM:@rerecoCommon or DQM:@standardDQM+@ExtraHLT

I agree that this makes confusion on what it for MC/Data or both

[1, 2016 UL]
https://cmsweb.cern.ch/couchdb/reqmgr_config_cache/5ad31c9be7cfbf89c62a938d7fa94dfa/configFile

[2, B-Parking]
https://cmsweb.cern.ch/couchdb/reqmgr_config_cache/be67f91f945866144130b3b61e2e2994/configFile

It was counter-intuitive to me that all miniAODValidation must be considered as "MC"

May I ask why? (I'm mostly just curious) I mean, we tend to use the word "validation" for MC and "DQM" for data.

It was counter-intuitive to me that all miniAODValidation must be considered as "MC"

May I ask why? (I'm mostly just curious) I mean, we tend to use the word "validation" for MC and "DQM" for data.

Ah, ok: that was the key I missed

@makortel @perrotta
If that is the key, the issue may solve easily. However, I see that we also use them together, i.e.
VALIDATION:@phase2Validation+@miniAODValidation,DQM:@phase2+@miniAODDQM in (*)

Can I simply say that if we have VALIDATION step, it will always be MC workflow?
And if we have only DQM, it will be data workflow?

(*)
https://cmsweb.cern.ch/couchdb/reqmgr_config_cache/ab0d87d02b073cc5e7ae3f16e1350bbb/configFile

Then I assume we should be OK if DQM-only is used with MC because CMSSW should treat it like data in this way.

I mean, we tend to use the word "validation" for MC and "DQM" for data.

I believe this is not true in a practical and general way, as you can see from:

https://github.com/cms-sw/cmssw/blob/master/Configuration/PyReleaseValidation/python/relval_steps.py

where there is a mixture of VALIDATION,DQM and VALIDATION/DQM over data and MC too

What it is true is that the DQM Unit Tests performed on:
https://github.com/cms-sw/cmssw/blob/master/DQMOffline/Configuration/test/runtests.sh#L23
are using a single (real) data input file, to simplify them, so we could switch these tests to isData=True to make them run with Phat's PR

In these tests, since a module with a given label can have different configurations depending on options such as Era,
Scenario, Data vs. MC etc, if multiple configurations for the same name were found, they are listed separately
here and denoted using subscripts. That is why the changes in this PR are failing somehow.

I don't know if the changes in this PR have implications in the standard workflows though

I mean, we tend to use the word "validation" for MC and "DQM" for data.

I believe this is not true in a practical and general way, as you can see from:

Right, sorry for being imprecise. What I really meant (and what would have been sufficient) is that what we tend to call "validation" in the context of "standard sequences" requires MC.

What it is true is that the DQM Unit Tests performed on:
https://github.com/cms-sw/cmssw/blob/master/DQMOffline/Configuration/test/runtests.sh#L23
are using a single (real) data input file, to simplify them, so we could switch these tests to isData=True to make them run with Phat's PR

This may not true. Somehow the test also picks DQM-only for Phase-2 workflow which does not exist (or I miss it somehow). CMSSW already guess that it is data-workflow, but still, fail. See in https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7e50b2/16755/unitTests/failed.html

The input file for the tests is always the same, and it is an EGamma 2018 RAW data file downloaded at the beginning of the script to avoid delays from AAA:
--infile file:///data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_0_X_2021-07-12-2300/tmp/004D960A-EA4C-E811-A908-FA163ED1F481.root

cmsbuild · 2021-07-13T11:35:47Z

-1

Failed Tests: UnitTests
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7e50b2/16736/summary.html
COMMIT: d20e7b5
CMSSW: CMSSW_12_0_X_2021-07-12-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/34452/16736/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 TestDQMOfflineConfiguration_160 had ERRORS
---> test TestDQMOfflineConfiguration_20 had ERRORS
---> test TestDQMOfflineConfiguration_200 had ERRORS
---> test TestDQMOfflineConfiguration_190 had ERRORS
and more ...

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 38
DQMHistoTests: Total histograms compared: 2787742
DQMHistoTests: Total failures: 882
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2786860
DQMHistoTests: Total skipped: 0
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 37 files compared)
Checked 160 log files, 37 edm output root files, 38 DQM output files
TriggerResults: no differences found

srimanob · 2021-07-13T11:59:34Z

I still see the error of the unit test. Maybe the best/proper way is to make test script pick up the proper config, either MC or data. VALIDATION only sequence is a bit tricky as it does not fall into any rule of guess as MC workflow.

cmsbuild · 2021-07-13T13:52:25Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34452/23908

This PR adds an extra 16KB to repository

jfernan2 · 2021-07-14T07:29:44Z

+1

srimanob · 2021-07-14T08:29:51Z

@cms-sw/xpog-l2

I've checked the Mini
https://github.com/cms-sw/cmssw/blob/master/Configuration/Applications/python/ConfigBuilder.py#L1676-L1682

and Nano sequences,
https://github.com/cms-sw/cmssw/blob/master/Configuration/Applications/python/ConfigBuilder.py#L1707

Basically, they are the same. They check first if isData is True or not. If not, MC (split between Fast/Full in Mini) customization will be used. So the code is already in the good shape already.

srimanob · 2021-07-27T06:18:59Z

Can this PR be merged? I don't see any point to hold it now. There is nothing change for XPOG, except cmsDriver can dump proper config when neither --data nor --mc is specified for production workflow.

mariadalfonso · 2021-07-27T07:47:54Z

@gouskos did you checked ?

srimanob · 2021-07-29T17:09:09Z

I would propose we merge this as we don't have (yet) a new proposal to make MC/Data guess from xpog. This PR does not change the way we guess, just make the setting right for data part.

I see no point to hold it.

gouskos · 2021-08-03T13:04:58Z

I would propose we merge this as we don't have (yet) a new proposal to make MC/Data guess from xpog. This PR does not change the way we guess, just make the setting right for data part.

I see no point to hold it.

Hi @srimanob I am not done checking possible issues.
Let's hold it for a couple of days

gouskos · 2021-08-09T10:00:41Z

+xpog
Hi @srimanob - to the best I can tell this should be fine

srimanob · 2021-08-09T11:04:18Z

Thanks @gouskos

By the way, I don't see the PR switches xpog status from pending to approved. FYI @qliphy @perrotta

qliphy · 2021-08-09T11:42:15Z

+operations

cmsbuild · 2021-08-09T11:42:39Z

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

qliphy · 2021-08-09T11:46:10Z

+1

Norraphat added 2 commits July 13, 2021 09:01

first implementation

5d5d7f0

first implementation

d20e7b5

cmsbuild added this to the CMSSW_12_0_X milestone Jul 13, 2021

cmsbuild added code-checks-pending operations-pending orp-pending pending-signatures tests-pending labels Jul 13, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 13, 2021

cmsbuild added tests-started and removed tests-pending labels Jul 13, 2021

perrotta reviewed Jul 13, 2021

View reviewed changes

srimanob mentioned this pull request Jul 13, 2021

Confusion in sequence when neither "--data" nor "--mc" is used #34443

Closed

cmsbuild added tests-rejected and removed tests-started labels Jul 13, 2021

update isMC checking guesing step

07661d5

cmsbuild added code-checks-pending tests-pending and removed code-checks-approved tests-rejected labels Jul 13, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 13, 2021

cmsbuild added dqm-pending xpog-pending labels Jul 14, 2021

cmsbuild added dqm-approved and removed dqm-pending labels Jul 14, 2021

cmsbuild mentioned this pull request Jul 17, 2021

Remove six python2->3 compatibility helper usage #34534

Merged

srimanob changed the title ~~[Draft] Fix when neither "data" nor "MC" is specified in the cmsDriver.~~ Fix when neither "data" nor "MC" is specified in the cmsDriver. Jul 20, 2021

smuzaffar modified the milestones: CMSSW_12_0_X, CMSSW_12_1_X Jul 30, 2021

cmsbuild added xpog-approved and removed xpog-pending labels Aug 9, 2021

cmsbuild added fully-signed operations-approved and removed operations-pending pending-signatures labels Aug 9, 2021

cmsbuild added orp-approved and removed orp-pending labels Aug 9, 2021

cmsbuild merged commit 18c8673 into cms-sw:master Aug 9, 2021

This was referenced Aug 11, 2021

[GCC11] Fix "this pointer is null" in class FWRecoGeometryESProducer of Fireworks/Geometry #34824

Merged

[GCC11] Add missing #include for std::size_t definition #34825

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix when neither "data" nor "MC" is specified in the cmsDriver. #34452

Fix when neither "data" nor "MC" is specified in the cmsDriver. #34452

srimanob commented Jul 13, 2021

smuzaffar commented Jul 13, 2021

cmsbuild commented Jul 13, 2021

cmsbuild commented Jul 13, 2021

smuzaffar commented Jul 13, 2021

perrotta Jul 13, 2021 •

edited

Loading

srimanob Jul 13, 2021

makortel Jul 13, 2021 •

edited

Loading

perrotta Jul 13, 2021

srimanob Jul 13, 2021

srimanob Jul 13, 2021

jfernan2 Jul 13, 2021

makortel Jul 13, 2021

srimanob Jul 13, 2021

jfernan2 Jul 13, 2021

cmsbuild commented Jul 13, 2021

srimanob commented Jul 13, 2021

cmsbuild commented Jul 13, 2021

jfernan2 commented Jul 14, 2021

srimanob commented Jul 14, 2021

srimanob commented Jul 27, 2021

mariadalfonso commented Jul 27, 2021

srimanob commented Jul 29, 2021

gouskos commented Aug 3, 2021

gouskos commented Aug 9, 2021 •

edited by qliphy

Loading

srimanob commented Aug 9, 2021

qliphy commented Aug 9, 2021

cmsbuild commented Aug 9, 2021

qliphy commented Aug 9, 2021

Fix when neither "data" nor "MC" is specified in the cmsDriver. #34452

Fix when neither "data" nor "MC" is specified in the cmsDriver. #34452

Conversation

srimanob commented Jul 13, 2021

PR description:

PR validation:

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

smuzaffar commented Jul 13, 2021

cmsbuild commented Jul 13, 2021

cmsbuild commented Jul 13, 2021

smuzaffar commented Jul 13, 2021

perrotta Jul 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makortel Jul 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Jul 13, 2021

Unit Tests

Comparison Summary

srimanob commented Jul 13, 2021

cmsbuild commented Jul 13, 2021

jfernan2 commented Jul 14, 2021

srimanob commented Jul 14, 2021

srimanob commented Jul 27, 2021

mariadalfonso commented Jul 27, 2021

srimanob commented Jul 29, 2021

gouskos commented Aug 3, 2021

gouskos commented Aug 9, 2021 • edited by qliphy Loading

srimanob commented Aug 9, 2021

qliphy commented Aug 9, 2021

cmsbuild commented Aug 9, 2021

qliphy commented Aug 9, 2021

perrotta Jul 13, 2021 •

edited

Loading

makortel Jul 13, 2021 •

edited

Loading

gouskos commented Aug 9, 2021 •

edited by qliphy

Loading