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

Improve handling of phase-2 geometries and add unit tests for Fireworks/Geometry package #45328

Merged
merged 2 commits into from
Aug 28, 2024

Conversation

mmusich
Copy link
Contributor

@mmusich mmusich commented Jun 27, 2024

PR description:

This is in response of #43097 (comment).

  • 9fe55e7 : improves the handing of phase-2 global tags and eras in dumpRecoGeometry
  • d7d4b43 : adds a testing script for dumpRecoGeometry

PR validation:

Run scram b runtests_test_dumpRecoGeometry, that reveals the issue #43097 (comment).

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

N/A

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 27, 2024

cms-bot internal usage

function die { echo $1: status $2 ; exit $2; }

# Define the base directory where the geometry files are located
GEOMETRY_DIR="$CMSSW_RELEASE_BASE/src/Configuration/Geometry/python"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smuzaffar I think this will be fine for IBs and full releases but not maybe not for PR and patches ?
Do you have a suggestion to automatically fall back to $CMSSW_BASE in those cases ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmusich , yes this is not good for PR and patch releases as it will not pack any local changes from $CMSSW_BASE/src/Configuration/Geometry. In many places in cmssw we do something like

XDIR="${CMSSW_BASE}/<path>"
if [ ! -e ${XDIR} ] ; then
  XDIR="${CMSSW_RELEASE_BASE}/<path>"
fi

We can add a utility script ( either in cmssw itself or in cms-common to be available for all releases) which can return the fullpath for a relative path e.g. find-cmssw-file relative-path and it can return either $CMSSW_BASE/relative-path or $CMSSW_RELEASE_BASE/relative-path

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45328/40745

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @mmusich for master.

It involves the following packages:

  • Fireworks/Geometry (visualization)

@Dr15Jones, @alja, @cmsbuild, @makortel can you please review it and eventually sign? Thanks.
@alja this is something you requested to watch as well.
@antoniovilela, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

if options.tag == "2026" or options.tag == "MaPSA":
prop_key = 2026
version_key = options.tag + options.version
elif options.tag == "2017": #or options.tag == "2021": (this leads to crashes in tests ?)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cms-sw/geometry-l2 if I add here the 2021 part (which makes use the era Run3) I get a failure:

----- Begin Fatal Exception 27-Jun-2024 14:17:04 CEST-----------------------
An exception of category 'NoProductResolverException' occurred while
   [0] Processing  Event run: 1 lumi: 1 event: 1 stream: 0
   [1] Running path 'p'
   [2] Prefetching for module DumpFWRecoGeometry/'dump'
   [3] Prefetching for EventSetup module FWRecoGeometryESProducer/''
   [4] Prefetching for EventSetup module CaloGeometryBuilder/''
   [5] Calling method for EventSetup module EcalPreshowerGeometryEPdd4hep/'EcalPreshowerGeometryEP'
Exception Message:
No data of type "cms::DDCompactView" with label "" in record "IdealGeometryRecord"
 Please add an ESSource or ESProducer to your job which can deliver this data.
----- End Fatal Exception -------------------------------------------------

while that's not the case when no era is used. Is something missing in the GlobalTags ?

@mmusich
Copy link
Contributor Author

mmusich commented Jun 27, 2024

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

-1

Failed Tests: UnitTests
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-050cc7/40123/summary.html
COMMIT: d7d4b43
CMSSW: CMSSW_14_1_X_2024-06-27-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/45328/40123/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found 1 errors in the following unit tests:

---> test test_dumpRecoGeometry had ERRORS

Comparison Summary

Summary:

  • You potentially added 3 lines to the logs
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3345088
  • DQMHistoTests: Total failures: 4
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3345064
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 202 log files, 165 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45328/40751

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

Pull request #45328 was updated. @Dr15Jones, @alja, @cmsbuild, @makortel can you please check and sign again.

@mmusich mmusich changed the title [RFC] improve handling of phase-2 geometries and add unit tests for Fireworks/Geometry package Improve handling of phase-2 geometries and add unit tests for Fireworks/Geometry package Aug 24, 2024
@cmsbuild
Copy link
Contributor

-1

Failed Tests: RelVals-INPUT
Size: This PR adds an extra 16KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-050cc7/41113/summary.html
COMMIT: a759353
CMSSW: CMSSW_14_1_X_2024-08-23-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/45328/41113/install.sh to create a dev area with all the needed externals and cmssw changes.

  • DAS Queries: The DAS query tests failed, see the summary page for details.

RelVals-INPUT

  • 2024.1040052024.104005_RunJetMET02024C_50k/step1_dasquery.log
  • 2024.1060052024.106005_RunMuonEG2024C_50k/step1_dasquery.log
  • 2024.3090052024.309005_RunParkingHH2024E_50k/step1_dasquery.log
Expand to see more relval errors ...

Comparison Summary

Summary:

@alja
Copy link
Contributor

alja commented Aug 26, 2024

+1

@cmsbuild
Copy link
Contributor

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. @rappoccio, @antoniovilela, @mandrenguyen, @sextonkennedy (and backports should be raised in the release meeting by the corresponding L2)
Notice This PR was tested with additional Pull Request(s), please also merge them if necessary: #45784

@cmsbuild
Copy link
Contributor

Milestone for this pull request has been moved to CMSSW_14_2_X. Please open a backport if it should also go in to CMSSW_14_1_X.

@cmsbuild cmsbuild modified the milestones: CMSSW_14_1_X, CMSSW_14_2_X Aug 27, 2024
@mandrenguyen
Copy link
Contributor

mandrenguyen commented Aug 28, 2024

+1
Merging together with #45784

@mandrenguyen
Copy link
Contributor

ignore tests-rejected with ib-failure

@cmsbuild cmsbuild merged commit 7a2e508 into cms-sw:master Aug 28, 2024
10 of 11 checks passed
@mandrenguyen
Copy link
Contributor

merge

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.

5 participants