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

[DD4Hep] make sure that expectations for retrieving a numeric value are met #32422

Closed
wants to merge 2 commits into from

Conversation

ianna
Copy link
Contributor

@ianna ianna commented Dec 8, 2020

PR description:

Force a request to get a numeric value for a non-evaluated numeric value to fail.

This will allow us to trace all cases where XML may need an update without debugging the C++ code.
See #32414

PR validation:

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

Before submitting your pull requests, make sure you followed this checklist:

@ianna
Copy link
Contributor Author

ianna commented Dec 8, 2020

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 8, 2020

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32422/20288

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 8, 2020

A new Pull Request was created by @ianna (Ianna Osborne) for master.

It involves the following packages:

DetectorDescription/DDCMS

@civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth can you please review it and eventually sign? Thanks.
@fabiocos, @slomeo, @vargasa this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 8, 2020

-1

CMSSW: CMSSW_11_3_X_2020-12-08-1100
SCRAM_ARCH: slc7_amd64_gcc900
You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c0a02f/11436/summary.html

I found follow errors while testing this PR

Failed tests: RelVals

  • RelVals:

When I ran the RelVals I found an error in the following workflows:
11624.911 step2

runTheMatrix-results/11624.911_TTbar_13+2021_DD4hep+TTbar_13TeV_TuneCUETP8M1_GenSim+Digi+Reco+HARVEST+ALCA/step2_TTbar_13+2021_DD4hep+TTbar_13TeV_TuneCUETP8M1_GenSim+Digi+Reco+HARVEST+ALCA.log

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 9, 2020

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32422/20297

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 9, 2020

Pull request #32422 was updated. @civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild can you please check and sign again.

@ianna
Copy link
Contributor Author

ianna commented Dec 9, 2020

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 9, 2020

-1

CMSSW: CMSSW_11_3_X_2020-12-08-2300/slc7_amd64_gcc900
You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c0a02f/11452/summary.html

I found follow errors while testing this PR

Failed tests: RelVals

  • RelVals:

When I ran the RelVals I found an error in the following workflows:
11624.911 step1

runTheMatrix-results/11624.911_TTbar_13+2021_DD4hep+TTbar_13TeV_TuneCUETP8M1_GenSim+Digi+Reco+HARVEST+ALCA/step1_TTbar_13+2021_DD4hep+TTbar_13TeV_TuneCUETP8M1_GenSim+Digi+Reco+HARVEST+ALCA.log

@cvuosalo
Copy link
Contributor

cvuosalo commented Dec 9, 2020

@ghugo83 Did you see the error in this PR test?
"Request for a non-existent numeric parameter PixelROCRows Please, check that it is defined in XML and has an eval attribute set." It's related to TrackerGeometricDetESModule/trackerNumberingGeometry. Are you working on this?

@ianna
Copy link
Contributor Author

ianna commented Dec 10, 2020

Legacy DD returns zero by design. It looks like current Tracker code needs this feature to construct a generic GeomDet. It's possible that the other sub-detectors exploit it as well. Closing the PR for now.

@ianna ianna closed this Dec 10, 2020
@ghugo83
Copy link
Contributor

ghugo83 commented Dec 10, 2020

@ghugo83 Did you see the error in this PR test?
"Request for a non-existent numeric parameter PixelROCRows Please, check that it is defined in XML and has an eval attribute set." It's related to TrackerGeometricDetESModule/trackerNumberingGeometry. Are you working on this?

Well, seeing this first with this message ;)
So I have looked at it, and there are plenty of numerical values in the XMLs (both Phase 1 and Phase 2) with are not set with eval = true. This had correct behavior with old DD, but also with DD4hep until recently (trackerRadLength/Xi, numROCRows, were for sure OK for example with DD4hep workflow).
So there must have been a very recent change in FilteredView behavior, that now makes these 'eval=true' needed in the XMLs ? If so, I will just add it to all the XMLs (but would rather not use versions as numerous files are concerned).

@ianna
Copy link
Contributor Author

ianna commented Dec 10, 2020

@ghugo83 Did you see the error in this PR test?
"Request for a non-existent numeric parameter PixelROCRows Please, check that it is defined in XML and has an eval attribute set." It's related to TrackerGeometricDetESModule/trackerNumberingGeometry. Are you working on this?

Well, seeing this first with this message ;)
So I have looked at it, and there are plenty of numerical values in the XMLs (both Phase 1 and Phase 2) with are not set with eval = true. This had correct behavior with old DD, but also with DD4hep until recently (trackerRadLength/Xi, numROCRows, were for sure OK for example with DD4hep workflow).
So there must have been a very recent change in FilteredView behavior, that now makes these 'eval=true' needed in the XMLs ? If so, I will just add it to all the XMLs (but would rather not use versions as numerous files are concerned).

Yes, it was introduced in the two recent PRs we've discussed at the last meeting that have fixed the thread safety issue:
#32249

@ianna
Copy link
Contributor Author

ianna commented Dec 10, 2020

@ghugo83 - here is the issue that is still open: #32414

@ghugo83
Copy link
Contributor

ghugo83 commented Dec 14, 2020

@ianna Thanks

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