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

Run2-gex125 Attempt to make 2017 scenario compatible with dd4hep #37394

Merged
merged 2 commits into from Apr 11, 2022

Conversation

bsunanda
Copy link
Contributor

@bsunanda bsunanda commented Mar 29, 2022

PR description:

Attempt to make 2017 scenario compatible with dd4hep. There is still an issue in DTGeometryESModule which was observed earlier for dd4hep version of Phase2 and resolved by Phat. The relevant files for Geometry are in Geometry/HcalCommonData/python/GeometryDD4HepExtended2017... and test codes are in Geometry/HcalCommonData/test/python for steps 1,2,3 derived from 10024.0

PR validation:

Tested using the runTheMatrix test workflows

Details about the test procedure are available below in

Definitely all these scenarios should be finally made testable in the IBs, with either a unit test or a dedicated workflow.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37394/29047

  • This PR adds an extra 60KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @bsunanda (Sunanda Banerjee) for master.

It involves the following packages:

  • Geometry/CMSCommonData (geometry, upgrade)
  • Geometry/EcalCommonData (geometry)
  • Geometry/HcalCommonData (geometry)
  • Geometry/HcalSimData (geometry)
  • Geometry/MuonCommonData (geometry)
  • Geometry/TrackerCommonData (geometry)

@civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @AdrianoDee, @srimanob can you please review it and eventually sign? Thanks.
@vargasa, @rchatter, @JanFSchulte, @VinInn, @ghugo83, @ptcox, @thomreis, @simonepigazzini, @mmusich, @argiro, @mtosi, @fabiocos, @slomeo, @venturia this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@bsunanda
Copy link
Contributor Author

@cmsbuild Please test

@bsunanda
Copy link
Contributor Author

@srimanob How did you cure the discrepancy for number of ID's for DT's

@srimanob
Copy link
Contributor

Hi @bsunanda

I follow Run-3 config by using
'Geometry/DTGeometryBuilder/data/dtSpecsFilter/2021/v1/dtSpecsFilter.xml',
instead of
'Geometry/DTGeometryBuilder/data/dtSpecsFilter.xml',

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37394/29048

  • This PR adds an extra 60KB to repository

@cmsbuild
Copy link
Contributor

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

@bsunanda
Copy link
Contributor Author

@cmsbuild Please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cefc60/23507/summary.html
COMMIT: 553d4b3
CMSSW: CMSSW_12_4_X_2022-03-29-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/37394/23507/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: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3585896
  • DQMHistoTests: Total failures: 8
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3585866
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 200 log files, 45 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@cvuosalo
Copy link
Contributor

Why is eefixed.xml being changed? It looks like a small change in the geometry, from toruses to tube segments.

@bsunanda
Copy link
Contributor Author

bsunanda commented Apr 1, 2022 via email

@cvuosalo
Copy link
Contributor

cvuosalo commented Apr 4, 2022

I started a document to record geometry changes required for the Runs 1-2 DD4hep migration: https://docs.google.com/document/d/13PgtGTpk92shgtwWPVy8rCx0s7mmXEjH7kz1veYC7dI/edit?usp=sharing

@cvuosalo
Copy link
Contributor

cvuosalo commented Apr 4, 2022

+1

@srimanob
Copy link
Contributor

srimanob commented Apr 7, 2022

+Upgrade

This PR is am attempt to make DD4hep for Run-2 (2017). It will not effect the existing workflow.

Should we open the git issue which may just have a short description and then point to the gdoc? So that we will not forget the remaining, at least from the list of remaining items on git.

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 7, 2022

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

@perrotta
Copy link
Contributor

perrotta commented Apr 7, 2022

Tested using the runTheMatrix test workflows

@bsunanda could you please specify how was it tested? Are those workflows the one regularly run during the IB tests (and therefore possible future issues or incompatibilities with other updates can be spotted by them during the usual IB checks)?

@srimanob
Copy link
Contributor

srimanob commented Apr 7, 2022

@perrotta
I think this was what we start the discussion during the O&C week on the strategy. Do we need to have something run-able but not validate not test, or we need something to be tested regularly. I think we don't have a clear plan of what to be done. I signed it as considered that it is a starting point to have work XML for DD4hep Run-2.

If the goal or strategy are still unclear, I think we can find a chance for following up discussion. One possibility is tomorrow SIM meeting. But I think the goal/strategy should come from top-level. This was also stated by @civanch during the meeting.

@civanch
Copy link
Contributor

civanch commented Apr 7, 2022

Hi all, yes the discussion should go but we cannot come to conclusion what is needed to CMS in a short term. We are in a slow process to prepare variants of Run-2 geometry, which can be processed by DD4hep.

So, I would merge this PR.

@perrotta
Copy link
Contributor

perrotta commented Apr 7, 2022

@bsunanda @srimanob @civanch my first question was simple, and answering it shouldn't delay the merging of this PR.

Tested using the runTheMatrix test workflows

  • Could you please specify how was it tested?

While for the second question ("Are those workflows the ones regularly run during the IB tests (and therefore possible future issues or incompatibilities with other updates can be spotted by them during the usual IB checks?"), I imaging from the previous answers of yours that those " runTheMatrix test workflows" were not the ones run regularly on IB tests. In any case, also answering this question shouldn't take that much...

@cvuosalo
Copy link
Contributor

cvuosalo commented Apr 7, 2022

@perrotta I would guess that this PR was tested with the test scripts included in the PR. A Run 2 DD4hep workflow that runs correctly to completion is probably some time away. This PR may be a first step in that direction.

@bsunanda
Copy link
Contributor Author

I tested using the cfg's stored in Geometry/HcalCommonData. There are still issues - they are related to some HCAL code which we see for Phase2 dd4hep trials as well. We shall handle this separately. For DDD this works fine till the end. This is a good starting point for transition of Run2 scenario. There will be many steps to go.

@perrotta
Copy link
Contributor

I tested using the cfg's stored in Geometry/HcalCommonData. There are still issues - they are related to some HCAL code which we see for Phase2 dd4hep trials as well. We shall handle this separately. For DDD this works fine till the end. This is a good starting point for transition of Run2 scenario. There will be many steps to go.

Thank you @bsunanda

Whilch is the test that succeeds, and the one which fails? What is the failing message, to be left here as a reference?
Would it be possible to make those config to be run as unit tests, so that they can be tested automatically at every IB and spot possible issues as soon as they show up?

@bsunanda
Copy link
Contributor Author

It fails in the L1 step related to HCAL - this is most likely related to accessing calibration constant. It is a bit complex and I shall look into this issue later on. I do not want to make it a unit test till the issue is resolved. For DDD, there is no issue - only happend for dd4hep. So it is most likely linked with the specpar analysis The cfg's 10024.py are the ones used to take care of steps 1, 2, 3 for 2017 scenario. Please let this PR be merged and then I shall start investigating the issue

@bsunanda
Copy link
Contributor Author

The tests performed started with 2 workflows: 10024.0 and 10024.911 and replacing the geometries using process.load('Geometry.HcalCommonData.GeometryDD4HepExtended2017_cff')
process.load('Geometry.HcalCommonData.GeometryDD4HepExtended2017Reco_cff')
The job failed in step2 for the dd4hep side with the message:

An exception of category 'Conditions mismatch' occurred while
[0] Processing Event run: 1 lumi: 1 event: 1 stream: 0
[1] Running path 'HLTAnalyzerEndpath'
[2] Prefetching for module L1TRawToDigi/'hltGtStage2Digis'
[3] Prefetching for module RawDataCollectorByLabel/'rawDataCollector'
[4] Prefetching for module SiStripDigiToRawModule/'SiStripDigiToRaw'
[5] Calling method for module MixingModule/'mix'
Exception Message:
Requested conditions of type HcalQIETypes for cell (0x45104401) (HE -17,1,1) got
conditions for cell (0x45386c0b) (HE 27,11,3)

@perrotta
Copy link
Contributor

The tests performed started with 2 workflows: 10024.0 and 10024.911 and replacing the geometries using process.load('Geometry.HcalCommonData.GeometryDD4HepExtended2017_cff') process.load('Geometry.HcalCommonData.GeometryDD4HepExtended2017Reco_cff') The job failed in step2 for the dd4hep side with the message:

An exception of category 'Conditions mismatch' occurred while [0] Processing Event run: 1 lumi: 1 event: 1 stream: 0 [1] Running path 'HLTAnalyzerEndpath' [2] Prefetching for module L1TRawToDigi/'hltGtStage2Digis' [3] Prefetching for module RawDataCollectorByLabel/'rawDataCollector' [4] Prefetching for module SiStripDigiToRawModule/'SiStripDigiToRaw' [5] Calling method for module MixingModule/'mix' Exception Message: Requested conditions of type HcalQIETypes for cell (0x45104401) (HE -17,1,1) got conditions for cell (0x45386c0b) (HE 27,11,3)

Thank you @bsunanda
Let accept this PR to let you proceed. However, these scenarios must be made testable in the IBs. The two workflows: 10024.0 and 10024.911 modified as you reported in #37394 (comment) should better become two new workflows being regularly run in the IB tests (of course, by keeping the DD4Hep workflow disabled until the exception that you are talking about cannot be fixed).

@perrotta
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 08c576f into cms-sw:master Apr 11, 2022
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