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

Added consumes to DD related ESProducers #28346

Merged
merged 2 commits into from Nov 6, 2019

Conversation

Dr15Jones
Copy link
Contributor

PR description:

Also use ESGetToken to retrieve data.

PR validation:

The unit tests succeed, even when using #28223.

Also use ESGetToken to retrieve data.
@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 5, 2019

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 5, 2019

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28346/12622

  • This PR adds an extra 12KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 5, 2019

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

It involves the following packages:

DetectorDescription/DDCMS

@civanch, @Dr15Jones, @cvuosalo, @ianna, @mdhildreth, @cmsbuild can you please review it and eventually sign? Thanks.
@vargasa 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

@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 5, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/3342/console Started: 2019/11/05 07:40

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 5, 2019

The tests are being triggered in jenkins.
Tested with other pull request(s) #28223,#28338
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/3343/console Started: 2019/11/05 07:48

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 5, 2019

-1

Tested at: 7b4fda1

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d82277/3343/summary.html

I found follow errors while testing this PR

Failed tests: UnitTests

  • Unit Tests:

I found errors in the following unit tests:

---> test DetectorDescriptionDDCMSTestDriver had ERRORS

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 5, 2019

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 5, 2019

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2939026
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2938683
  • 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

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 6, 2019

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 6, 2019

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 6, 2019

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f4cc8f/3361/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: 2939026
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2938684
  • 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

@fabiocos
Copy link
Contributor

fabiocos commented Nov 6, 2019

+1

@ianna @cvuosalo the code looks ok to me, please check and in case comment for a further iteration. As I want to get ready for a next pre-release I would like to have the development of which this code is part finalized in time

@fabiocos
Copy link
Contributor

fabiocos commented Nov 6, 2019

merge

@cmsbuild cmsbuild merged commit 139c01c into cms-sw:master Nov 6, 2019
@Dr15Jones Dr15Jones deleted the consumesDDCompactViewESProducer branch November 7, 2019 22:10
@cvuosalo
Copy link
Contributor

cvuosalo commented Dec 3, 2019

@Dr15Jones @namapane I am just seeing this PR now. I think it is related to the reason the magnetic field geometry builder stopped working. I'm guessing that the changes made in this PR to DetectorDescription/DDCMS/plugins/DDCompactViewESProducer.cc need also to be made to DetectorDescription/DDCMS/plugins/DDCompactViewMFESProducer.cc to fix the mf geometry builder.

@namapane
Copy link
Contributor

namapane commented Dec 4, 2019

@cvuosalo, there's one thing I do not understand here, why do we need an explicit ESProducer here:

process.DDCompactViewMFESProducer = cms.ESProducer("DDCompactViewMFESProducer",

while we have no explicit DDCompactViewESProducer set up in any of the .py for the non-dd4hep versions of the MF?
And also, would this DDCompactViewMFESProducer snippet have to go in the cfi included in the standard sequences or not? I also have to figure out if that is needed for the dd4hep producer from DB, which would not require a DDCompactViewMFESProducer, I suppose, since it must create iteslf the DDCompactView from the fileBlob.

@cvuosalo
Copy link
Contributor

cvuosalo commented Dec 4, 2019

@namapane I'm not sure why it is needed now. The way this program gets its products has been changed over the last several months by @Dr15Jones and @makortel so I'm not sure anymore how it should work. I can tell you that originally, before I created the DDCompactViewMFESProducer, the Python config generated errors saying that an ESProducer was needed. You could try commenting out the DDCompactViewMFESProducer to see if it's still needed. Maybe there is a better way now to write the Python config. You could ask @Dr15Jones or @ianna when she returns.

@fabiocos
Copy link
Contributor

fabiocos commented Dec 9, 2019

@cvuosalo the problem you are mentioning isn't clear to me, I integrated this PR as it looked technically straightforward and ok. Could you please open an issue and describe the problem, so as we can cleanly keep track of it?

@namapane
Copy link
Contributor

namapane commented Dec 9, 2019

@fabiocos well this is part of the plan of migrating to dd4hep. At the moment we only have a static producer for dd4hep that is not used in production; to complete the migration we have to implement the version from DB and while doing so we have to get record dependencies right. It is not clear to me how to do this but that's in my to-do list (proceeding slowly because of teaching...)

@namapane
Copy link
Contributor

I fixed the problem in #28642; as suggested by Carl DDCompactViewMFESProducer had to be updated.

@namapane namapane mentioned this pull request Dec 20, 2019
@cvuosalo
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 (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo (and backports should be raised in the release meeting by the corresponding L2)

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

5 participants