Use mayConsumes in VolumeBasedMagneticFieldESProducerFromDB #28120

Dr15Jones · 2019-10-07T14:28:06Z

PR description:

The module decides which geometry to read from the database by reading the value stored in the MagFieldConfig EventSetup object. The decision cannot be made at module configuration time, only at run time. Therefore mayConsumes must be used.

Unit tests were added to guarantee the module's behavior is the same after this change.

PR validation:

Ran a slightly modified version of MagneticField/Engine/test/testMagneticFieldDB.py where the comparison field map file was copied from afs and read locally. This test showed no changes.
Created a host of new unit tests which check the field value at (0,0,0) for all different configurations of VolumeBasedMagneticFieldESProducerFromDB. The comparison values were obtained from running the tests on the orginal version of the module. All the new unit tests pass.

This also included a test for RunInfoTestESProducer.

This is used to test VolumeBasedMagneticFieldESProducerFromDB.

The module decides which geometry to read from the database by reading the value stored in the MagFieldConfig EventSetup object. The decision cannot be made at module configuration time, only at run time. Therefore mayConsumes must be used. Unit tests were added to guarantee the module's behavior is the same after this change.

cmsbuild · 2019-10-07T14:28:37Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-10-07T14:35:03Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28120/12156

This PR adds an extra 24KB to repository

Dr15Jones · 2019-10-07T14:35:38Z

please test

cmsbuild · 2019-10-07T14:35:39Z

A new Pull Request was created by @Dr15Jones (Chris Jones) for master.

It involves the following packages:

CondFormats/MFObjects
CondTools/RunInfo
MagneticField/GeomBuilder

@perrotta, @slava77, @christopheralanwest, @tocheng, @cmsbuild, @franzoni, @tlampen, @ggovi, @pohsun can you please review it and eventually sign? Thanks.
@tocheng, @namapane, @mmusich, @seemasharmafnal this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

cmsbuild · 2019-10-07T14:36:03Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/2818/console Started: 2019/10/07 16:36

namapane · 2019-10-07T15:46:33Z

I'm completely lost in the logic behind these modifications. Since we are also in the middle of migrating the same files to DD4hep, including some planned cleanup of eg. test .py, can you please wait before proceeding with this PR?

Dr15Jones · 2019-10-07T16:14:50Z

@namapane we are working towards requiring all ESProducers to call consumes. We are very close except for a few modules which need a functional mayConsumes (like the present implementation of VolumeBasedMagneticFieldESProducerFromDB).

If your upcoming changes can all work with calling consumes, then great. If the algorithm can not know what it needs to get from the EventSetup system at configuration time, we can work with you to setup the proper mayConsume calls.

cmsbuild · 2019-10-07T16:17:23Z

+1
Tested at: 5106105
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f5809f/2818/summary.html

cmsbuild · 2019-10-07T16:17:26Z

Comparison job queued.

namapane · 2019-10-07T16:49:22Z

@Dr15Jones, I do understand the idea behind mayConsumes; but I'm puzzled by the fact that moving to mayConsume requires so many changes in the code, and I am not sure I understand all their implications. The migration to DD4hep would be a clone of the producer with the same functional logic, so it will also require mayConsumes.
Also the "unit tests" do not make sense for me, at least at a first look, see comments to the code. If you insist in releasing these unit tests please move them to a separate folder. I would prefer these not to be released at all since (a) they make up for a very poor test of the actual configuration [I can provide details if needed]; (b) the new DD4hep producer will require complete tests anyhow; (c) we will probably have to declare some non-standard currents obsolete which will break some of these tests (which I don't want to have to maintain myself given (a) and (b))

cmsbuild · 2019-10-07T17:55:55Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f5809f/2818/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2961052
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2960710
DQMHistoTests: Total skipped: 341
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
Checked 147 log files, 16 edm output root files, 34 DQM output files

slava77 · 2019-10-14T17:19:16Z

@namapane
please clarify if you are OK with this PR.
Thank you.

namapane · 2019-10-14T18:11:27Z

Yes, everything fine for me. Thanks for checking.

ggovi · 2019-10-15T15:40:06Z

+1

slava77 · 2019-10-16T08:08:02Z

+1

for #28120 9d98541

code changes are in line with the PR description and the follow up review. Changes in the reco category are only in MagneticField/GeomBuilder/plugins/VolumeBasedMagneticFieldESProducerFromDB.cc
jenkins tests pass and comparisons with the baseline show no (relevant) differences

fabiocos · 2019-10-16T13:31:12Z

@christopheralanwest @pohsun could you please have a look at this PR?

fabiocos · 2019-10-17T20:59:32Z

@christopheralanwest @pohsun any remark?

fabiocos · 2019-10-18T16:07:14Z

@christopheralanwest @pohsun @tocheng @tlampen could you please check this PR? I would like to finally move it forward, and CondFormats/MFobjects is directly under AlCa supervision

pohsun · 2019-10-19T10:05:22Z

+1

cmsbuild · 2019-10-19T10:05:52Z

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

fabiocos · 2019-10-20T07:20:34Z

code-checks

cmsbuild · 2019-10-20T07:21:06Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-10-20T07:28:20Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28120/12344

This PR adds an extra 28KB to repository
Found files with invalid states:
- MagneticField/GeomBuilder/test/testMagneticFieldDB_parameterized_cfg.results:
  - Added: 5106105
  - Deleted: 24ae889
- MagneticField/GeomBuilder/test/python/testMagneticFieldDB_valueOverride_cfg.py:
  - Added: 5106105
  - Deleted: 24ae889
- MagneticField/GeomBuilder/test/python/testMagneticFieldDB_parameterized_cfg.py:
  - Added: 5106105
  - Deleted: 24ae889
- MagneticField/GeomBuilder/test/testMagneticFieldDB_cfg.results:
  - Added: 5106105
  - Deleted: 24ae889
- MagneticField/GeomBuilder/test/python/testMagneticFieldDB_cfg.py:
  - Added: 5106105
  - Deleted: 24ae889
- MagneticField/GeomBuilder/test/testMagneticFieldDB.sh:
  - Added: 5106105
  - Deleted: 24ae889
- MagneticField/GeomBuilder/test/testMagneticFieldDB_valueOverride_cfg.results:
  - Added: 5106105
  - Deleted: 24ae889

fabiocos · 2019-10-20T07:29:48Z

+1

Dr15Jones added 3 commits October 7, 2019 09:01

Added RunInfoTestESProducer used for testing

7d2432c

This also included a test for RunInfoTestESProducer.

Added MagFieldConfigTestESProducer

e7c3558

This is used to test VolumeBasedMagneticFieldESProducerFromDB.

cmsbuild added this to the CMSSW_11_0_X milestone Oct 7, 2019

cmsbuild added alca-pending code-checks-pending comparison-pending db-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Oct 7, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 7, 2019

cmsbuild added tests-started and removed tests-pending labels Oct 7, 2019

Dr15Jones mentioned this pull request Oct 7, 2019

Migrate ESProducers used in standard workflows to EventSetup consumes #26748

Closed

cmsbuild added tests-approved and removed tests-started labels Oct 7, 2019

cmsbuild added db-approved and removed db-pending labels Oct 15, 2019

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Oct 16, 2019

cmsbuild mentioned this pull request Oct 16, 2019

updated architecture to mach cc* too #28189

Merged

cmsbuild added alca-approved fully-signed and removed alca-pending pending-signatures labels Oct 19, 2019

cmsbuild removed the code-checks-approved label Oct 20, 2019

cmsbuild added the code-checks-pending label Oct 20, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 20, 2019

cmsbuild added orp-approved and removed orp-pending labels Oct 20, 2019

cmsbuild merged commit bee8b7a into cms-sw:master Oct 20, 2019

Dr15Jones deleted the mayConsumeVolumeBasedMagneticFieldESProducerFromDB branch October 22, 2019 23:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use mayConsumes in VolumeBasedMagneticFieldESProducerFromDB #28120

Use mayConsumes in VolumeBasedMagneticFieldESProducerFromDB #28120

Dr15Jones commented Oct 7, 2019

cmsbuild commented Oct 7, 2019

cmsbuild commented Oct 7, 2019

Dr15Jones commented Oct 7, 2019

cmsbuild commented Oct 7, 2019

cmsbuild commented Oct 7, 2019 •

edited

namapane commented Oct 7, 2019

Dr15Jones commented Oct 7, 2019 •

edited

cmsbuild commented Oct 7, 2019

cmsbuild commented Oct 7, 2019

namapane commented Oct 7, 2019

cmsbuild commented Oct 7, 2019

slava77 commented Oct 14, 2019

namapane commented Oct 14, 2019

ggovi commented Oct 15, 2019

slava77 commented Oct 16, 2019

fabiocos commented Oct 16, 2019

fabiocos commented Oct 17, 2019

fabiocos commented Oct 18, 2019

pohsun commented Oct 19, 2019

cmsbuild commented Oct 19, 2019

fabiocos commented Oct 20, 2019

cmsbuild commented Oct 20, 2019

cmsbuild commented Oct 20, 2019

fabiocos commented Oct 20, 2019

Use mayConsumes in VolumeBasedMagneticFieldESProducerFromDB #28120

Use mayConsumes in VolumeBasedMagneticFieldESProducerFromDB #28120

Conversation

Dr15Jones commented Oct 7, 2019

PR description:

PR validation:

cmsbuild commented Oct 7, 2019

cmsbuild commented Oct 7, 2019

Dr15Jones commented Oct 7, 2019

cmsbuild commented Oct 7, 2019

cmsbuild commented Oct 7, 2019 • edited

namapane commented Oct 7, 2019

Dr15Jones commented Oct 7, 2019 • edited

cmsbuild commented Oct 7, 2019

cmsbuild commented Oct 7, 2019

namapane commented Oct 7, 2019

cmsbuild commented Oct 7, 2019

slava77 commented Oct 14, 2019

namapane commented Oct 14, 2019

ggovi commented Oct 15, 2019

slava77 commented Oct 16, 2019

fabiocos commented Oct 16, 2019

fabiocos commented Oct 17, 2019

fabiocos commented Oct 18, 2019

pohsun commented Oct 19, 2019

cmsbuild commented Oct 19, 2019

fabiocos commented Oct 20, 2019

cmsbuild commented Oct 20, 2019

cmsbuild commented Oct 20, 2019

fabiocos commented Oct 20, 2019

cmsbuild commented Oct 7, 2019 •

edited

Dr15Jones commented Oct 7, 2019 •

edited