[DD4hep] Add support for Assembly tags in XML and for creating DB payload #34344

cvuosalo · 2021-07-06T05:15:51Z

Up to now, our DD4hep code has supported Assemblies being used in algorithms but did not support explicit Assembly definitions in XML. This PR provides support for Assembly definitions in XML.

This PR is required for PR #34343 to be able to run.

Secondly, it also provides support for the tool from PR #34111 for creating the DD4hep ideal geometry description DB payload. This PR is required for #34111 to work.

The new Assembly XML syntax is simple. In the Solids section, Assemblies are defined merely with a name:

 <Assembly name="assembly1"/>

Then in the Logical Part section, an Assembly is treated like any other Logical Part. Its material should be "Air", but the material is never actually used. The PosPart placement of Assemblies is just like that of other Logical Parts.

PR validation:

This PR has been put through the following tests:

DD4hep simulation reading from XML (wf 11634.911)
DD4hep simulation reading the ideal geometry description(with Assemblies) from an Sqlite DB (modified wf 11634.911)
DB payload creation tool (PR 34111) reading the ideal geometry description (stress test, only done for testing purposes)
Unit tests

No backport will be done.

cvuosalo · 2021-07-06T05:17:44Z

urgent

cvuosalo · 2021-07-06T05:18:06Z

This PR is aimed for 12_0_0_pre4.

cmsbuild · 2021-07-06T05:23:59Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34344/23716

This PR adds an extra 36KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File DetectorDescription/DDCMS/data/testLogicalParts.xml modified in PR(s): [DD4hep] Tool for creation of DD4hep geometry DB payloads and for migration validation #34111
- File DetectorDescription/DDCMS/data/testPosParts.xml modified in PR(s): [DD4hep] Tool for creation of DD4hep geometry DB payloads and for migration validation #34111
- File DetectorDescription/DDCMS/data/testSolids.xml modified in PR(s): [DD4hep] Tool for creation of DD4hep geometry DB payloads and for migration validation #34111
- File DetectorDescription/DDCMS/interface/DDDetector.h modified in PR(s): [DD4hep] Tool for creation of DD4hep geometry DB payloads and for migration validation #34111
- File DetectorDescription/DDCMS/interface/DDNamespace.h modified in PR(s): [DD4hep] Tool for creation of DD4hep geometry DB payloads and for migration validation #34111
- File DetectorDescription/DDCMS/interface/DDParsingContext.h modified in PR(s): [DD4hep] start on geometry XML payload producer #33548, [DD4hep] Tool for creation of DD4hep geometry DB payloads and for migration validation #34111
- File DetectorDescription/DDCMS/interface/DDSolidShapes.h modified in PR(s): [DD4hep] Tool for creation of DD4hep geometry DB payloads and for migration validation #34111
- File DetectorDescription/DDCMS/plugins/DDDetectorESProducer.cc modified in PR(s): [DD4hep] Tool for creation of DD4hep geometry DB payloads and for migration validation #34111
- File DetectorDescription/DDCMS/plugins/dd4hep/DDDefinitions2Objects.cc modified in PR(s): [DD4hep] start on geometry XML payload producer #33548, [DD4hep] Tool for creation of DD4hep geometry DB payloads and for migration validation #34111, Bianca pinolini ecal reco phase2 commit unimib #34212
- File DetectorDescription/DDCMS/src/DDDetector.cc modified in PR(s): [DD4hep] Tool for creation of DD4hep geometry DB payloads and for migration validation #34111
- File DetectorDescription/DDCMS/src/DDFilteredView.cc modified in PR(s): Bianca pinolini ecal reco phase2 commit unimib #34212
- File DetectorDescription/DDCMS/src/DDNamespace.cc modified in PR(s): [DD4hep] Tool for creation of DD4hep geometry DB payloads and for migration validation #34111
- File DetectorDescription/DDCMS/test/DDSolidLegacyShapes.cppunit.cc modified in PR(s): [DD4hep] Tool for creation of DD4hep geometry DB payloads and for migration validation #34111

cmsbuild · 2021-07-06T05:24:24Z

A new Pull Request was created by @cvuosalo (Carl Vuosalo) for master.

It involves the following packages:

DetectorDescription/DDCMS

@civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild 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

cvuosalo · 2021-07-06T05:25:51Z

@cmsbuild please test

cmsbuild · 2021-07-06T08:48:11Z

-1

Failed Tests: UnitTests
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-417079/16480/summary.html
COMMIT: 1e620ab
CMSSW: CMSSW_12_0_X_2021-07-05-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/34344/16480/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 test_PixelBaryCentreTool had ERRORS

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: 2785924
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2785901
DQMHistoTests: Total skipped: 22
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

civanch · 2021-07-06T11:44:04Z

Failed unit test related to Run-2 not to Run-3, it should be unchanged I guess.

qliphy · 2021-07-06T12:04:17Z

The unit failure is already in IB and reported by #34223
It is related to #33583 and being fixed. Thus it is not related to this PR.
See also #34259 (comment)

civanch · 2021-07-06T14:18:58Z

+1

cmsbuild · 2021-07-06T14:19:21Z

This pull request is fully signed and it will be integrated in one of the next master IBs (but tests are reportedly failing). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

civanch · 2021-07-06T17:40:13Z

@cvuosalo , @ianna , is it possible to merge this PR just now, create an issue and improve the code in the next PR? We need it for tomorrow morning.

cvuosalo · 2021-07-06T17:49:56Z

Issue #34367 created to document and track concerns raised by @ianna. These concerns will be addressed on a high priority basis in one or more upcoming PRs.

civanch · 2021-07-07T07:09:51Z

+1

let us merge this for pre4, the code will be improved in the next PR after pre4.

cmsbuild · 2021-07-07T07:10:12Z

This pull request is fully signed and it will be integrated in one of the next master IBs (but tests are reportedly failing). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

ianna · 2021-07-07T07:14:54Z

+1

let us merge this for pre4, the code will be improved in the next PR after pre4.

@civanch - I do not see why we have to rush this in and lower our standards.

civanch · 2021-07-07T07:23:08Z

If this PR will not be merged for pre4 then validation campaign cannot be proposed and will be shifted to the last opened pre-release.

ianna · 2021-07-07T07:26:33Z

If this PR will not be merged for pre4 then validation campaign cannot be proposed and will be shifted to the last opened pre-release.

IMHO, it is better to validate a final solution than a temporary work-around.

civanch · 2021-07-07T07:32:19Z

@ianna , I agree but using work around we may earlier face other problem which today is hidden.

ianna · 2021-07-07T07:45:37Z

@ianna , I agree but using work around we may earlier face other problem which today is hidden.

We will also face new issues introduced with this work-around that it there to stay - @cvuosalo argues that this is the solution. I respectfully disagree with that statement. The goal of migrating to DD4hep to improve our code is being compromised.

cvuosalo · 2021-07-07T08:01:59Z

@ianna @civanch "@cvuosalo argues that this is the solution. " No, I think improvement and revision is likely.
I favor an incremental approach. It is useful to get a software project up and running and then improve from there.

ianna · 2021-07-07T09:48:03Z

@ianna @civanch "@cvuosalo argues that this is the solution. " No, I think improvement and revision is likely.
I favor an incremental approach. It is useful to get a software project up and running and then improve from there.

@cvuosalo - an incremental approach emphasises the virtue of taking small steps toward the goal. At each increment, the key phases of the software development life cycle, including functional specification, design, implementation, and testing, are reiterated. Even though "an incremental approach" sounds good, this PR ain't it.

cvuosalo · 2021-07-08T11:31:11Z

@cmsbuild please test

cvuosalo · 2021-07-08T11:35:09Z

The unit test that failed was not related to this PR. It was fixed in #34369. I started the tests again, just to clear the "test-rejected" marker. The first tests already gave a good result for the changes in this PR.

cmsbuild · 2021-07-08T14:52:29Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-417079/16598/summary.html
COMMIT: 1e620ab
CMSSW: CMSSW_12_0_X_2021-07-07-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/34344/16598/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 5163 differences found in the comparisons
DQMHistoTests: Total files compared: 38
DQMHistoTests: Total histograms compared: 2786260
DQMHistoTests: Total failures: 7
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2786231
DQMHistoTests: Total skipped: 22
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

silviodonato · 2021-07-08T17:38:24Z

+1
about the comparison, please note that the previous PR tests (#34344 (comment)) were ok. Both of them are based on the same commit 1e620ab
Moreover about

Reco comparison results: 5163 differences found in the comparisons

these are fake failures which appeared already in other PRs and Slava has proposed a fix right now #34409.

Add Assembly support

1e620ab

cmsbuild added this to the CMSSW_12_0_X milestone Jul 6, 2021

cmsbuild added code-checks-pending geometry-pending orp-pending pending-signatures tests-pending labels Jul 6, 2021

cvuosalo mentioned this pull request Jul 6, 2021

[DD4hep] Add DD4hep Run 3 workflow that reads geometry from DB #34343

Merged

cmsbuild added the urgent label Jul 6, 2021

cvuosalo changed the title ~~[DD4hep] Add support for Assembly tags in XML and creating DB payload~~ [DD4hep] Add support for Assembly tags in XML and for creating DB payload Jul 6, 2021

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

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

cmsbuild mentioned this pull request Jul 6, 2021

[DD4hep] Tool for creation of DD4hep geometry DB payloads and for migration validation #34111

Merged

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

cmsbuild added fully-signed and removed geometry-pending pending-signatures labels Jul 6, 2021

cvuosalo mentioned this pull request Jul 6, 2021

[High Priority][DD4hep] Improve design of DDCMS handling of assemblies and parsing context #34367

Closed

cmsbuild added fully-signed geometry-approved and removed geometry-rejected pending-signatures labels Jul 7, 2021

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

cmsbuild added tests-approved and removed tests-started labels Jul 8, 2021

cmsbuild added orp-approved and removed orp-pending labels Jul 8, 2021

cmsbuild merged commit db39755 into cms-sw:master Jul 8, 2021

This was referenced Jul 8, 2021

[DQM] Fix to compareDQMOutput since python3 #34411

Merged

Update to python 3.9.6 cms-sw/cmsdist#7111

Merged

qliphy mentioned this pull request Jul 9, 2021

introduce uploadConditions.py unit test #34379

Merged

[DD4hep] Add support for Assembly tags in XML and for creating DB payload #34344

[DD4hep] Add support for Assembly tags in XML and for creating DB payload #34344

Uh oh!

Conversation

cvuosalo commented Jul 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR validation:

Uh oh!

cvuosalo commented Jul 6, 2021

Uh oh!

cvuosalo commented Jul 6, 2021

Uh oh!

cmsbuild commented Jul 6, 2021

Uh oh!

cmsbuild commented Jul 6, 2021

Uh oh!

cvuosalo commented Jul 6, 2021

Uh oh!

cmsbuild commented Jul 6, 2021

Unit Tests

Comparison Summary

Uh oh!

civanch commented Jul 6, 2021

Uh oh!

qliphy commented Jul 6, 2021

Uh oh!

civanch commented Jul 6, 2021

Uh oh!

cmsbuild commented Jul 6, 2021

Uh oh!

civanch commented Jul 6, 2021

Uh oh!

cvuosalo commented Jul 6, 2021

Uh oh!

civanch commented Jul 7, 2021

Uh oh!

cmsbuild commented Jul 7, 2021

Uh oh!

ianna commented Jul 7, 2021

Uh oh!

civanch commented Jul 7, 2021

Uh oh!

ianna commented Jul 7, 2021

Uh oh!

civanch commented Jul 7, 2021

Uh oh!

ianna commented Jul 7, 2021

Uh oh!

cvuosalo commented Jul 7, 2021

Uh oh!

ianna commented Jul 7, 2021

Uh oh!

cvuosalo commented Jul 8, 2021

Uh oh!

cvuosalo commented Jul 8, 2021

Uh oh!

cmsbuild commented Jul 8, 2021

Comparison Summary

Uh oh!

silviodonato commented Jul 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cvuosalo commented Jul 6, 2021 •

edited

Loading