Problem in dumping Fireworks Geometry #43097
Comments
|
A new Issue was created by @waredjeb Wahid Redjeb. @Dr15Jones, @rappoccio, @sextonkennedy, @smuzaffar, @makortel, @antoniovilela can you please review it and eventually sign/assign? Thanks. cms-bot commands are listed here |
|
@bsunanda fyi |
|
Certainly the GlobalTag here cmssw/Fireworks/Geometry/python/dumpRecoGeometry_cfg.py Lines 78 to 83 in bca672b does not look right. |
|
assign geometry, visualization |
|
New categories assigned: geometry,visualization @Dr15Jones,@Dr15Jones,@alja,@civanch,@bsunanda,@makortel,@makortel,@mdhildreth you have been requested to review this Pull request/Issue and eventually sign? Thanks |
cmsbuild
added
geometry-pending
visualization-pending
pending-signatures
and removed
pending-assignment
labels
|
Marco's comment, to be followed up: |
|
Hello! I am reviving this issue because I am still having some problem in dumping the geometry. Release = This time, the GlobalTag looks good to me! @rovere FYI |
|
cms-bot internal usage |
|
@waredjeb Hi Wahid,
unfortunately it's not.
so you need and @cms-sw/alca-l2 @cms-sw/upgrade-l2 FYI. |
Hi Marco, I see, thank you very much! from Configuration.AlCa.GlobalTag import GlobalTag
process.GlobalTag = GlobalTag(process.GlobalTag, 'auto:phase2_realistic_T33', '')Seems to work, not sure if this Exception Message is concerning: |
I don't know either, but seems to concern |
|
Ok, including the Tracker raises another problem: |
this looks more concerning and might have to do with the sensor splitting in TBPix layer 1 (subdet ID 1). @emiglior @cms-sw/trk-dpg-l2 |
|
so this: diff --git a/Fireworks/Geometry/python/dumpRecoGeometry_cfg.py b/Fireworks/Geometry/python/dumpRecoGeometry_cfg.py
index 73c80aa320d..f975d372dd5 100644
--- a/Fireworks/Geometry/python/dumpRecoGeometry_cfg.py
+++ b/Fireworks/Geometry/python/dumpRecoGeometry_cfg.py
@@ -43,7 +43,7 @@ def versionCheck(ver):
print("")
help()
-def recoGeoLoad(score):
+def recoGeoLoad(score, properties):
print("Loading configuration for tag ", options.tag ,"...\n")
if score == "Run1":
@@ -78,8 +78,29 @@ def recoGeoLoad(score):
elif "2026" in score:
versionCheck(options.version)
process.load("Configuration.StandardSequences.FrontierConditions_GlobalTag_cff")
- from Configuration.AlCa.autoCond import autoCond
- process.GlobalTag.globaltag = autoCond['phase2_realistic']
+
+ # Import the required configuration from the CMSSW module
+ from Configuration.AlCa.autoCond import autoCond # Ensure autoCond is imported
+
+ # Ensure options.version is defined and set correctly
+ version_key = '2026' + options.version # This constructs the key for accessing the properties dictionary
+ print(f"Constructed version key: {version_key}")
+
+ # Check if the key exists in properties for 2026
+ if version_key in properties[2026]:
+ # Get the specific global tag for this version
+ global_tag_key = properties[2026][version_key]['GT']
+ print(f"Global tag key from properties: {global_tag_key}")
+
+ # Check if this key exists in autoCond
+ if global_tag_key.replace("auto:", "") in autoCond:
+ # Set the global tag
+ from Configuration.AlCa.GlobalTag import GlobalTag
+ process.GlobalTag = GlobalTag(process.GlobalTag, global_tag_key, '')
+ else:
+ raise KeyError(f"Global tag key '{global_tag_key}' not found in autoCond.")
+ else:
+ raise KeyError(f"Version key '{version_key}' not found in properties[2026].")
process.load('Configuration.Geometry.GeometryExtended2026'+options.version+'Reco_cff')
elif score == "MaPSA":
@@ -114,11 +135,8 @@ def recoGeoLoad(score):
help()
-
-
options = VarParsing.VarParsing ()
-
defaultOutputFileName="cmsRecoGeom.root"
options.register ('tag',
@@ -171,16 +189,33 @@ options.register ('out',
options.parseArguments()
-
-
-
-process = cms.Process("DUMP")
+from Configuration.PyReleaseValidation.upgradeWorkflowComponents import upgradeProperties as properties
+# Determine version_key based on the value of options.tag
+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":
+ prop_key = 2017
+ version_key = options.tag
+else:
+ prop_key = None
+ version_key = None
+
+if(prop_key and version_key):
+ print(f"Constructed version key: {version_key}")
+ era_key = properties[prop_key][str(version_key)]['Era']
+ print(f"Constructed era key: {era_key}")
+ from Configuration.StandardSequences.Eras import eras
+ era = getattr(eras, era_key)
+ process = cms.Process("DUMP",era)
+else:
+ process = cms.Process("DUMP")
process.add_(cms.Service("InitRootHandlers", ResetRootErrHandler = cms.untracked.bool(False)))
process.source = cms.Source("EmptySource")
process.maxEvents = cms.untracked.PSet(input = cms.untracked.int32(1))
-recoGeoLoad(options.tag)
+recoGeoLoad(options.tag,properties)
if ( options.tgeo == True):
if (options.out == defaultOutputFileName )allows for a slightly better automatic treatement of GlobalTags and eras in the configuration. Unfortunately it does not solve neither the warnings nor the crash for Tracker in D110. |
concerning the crash in the Tracker part, it looks it's due this line: in particular at the very beginning of the DetId loop in BPix (presumably Layer1 but I didn't explicitly cross-check), this call
fails on detid |
|
hi, is there any way this dumper can be run as a test automatically for every PR? Or at least for any PR that introduces a new version of the geometry? |
sure there is, this is what I was suggesting: #43098 (comment) |
|
@makortel ( PS: I'm on vacation starting tomorrow until Tuesday) |
I'm not sure I understand your question. Unit tests can be added in just a PR. Having a unit test run in PR tests of an unrelated package (*) is generally unresolved problem (for which we might already have an issue open). In any case @smuzaffar should be able to help further. (*) I didn't check if the package defining the geometries, and the straightforward place for the unit test are related in any way or not (being on vacation myself) |
|
@alja , as @makortel wrote, you can open the PR to add the unit test in one of the existing packages. About running the new unit test for all PR might be over killed. Is there any way to find if a PR is changing geometry? If yes then we can teach bot to checkout the package which contains this unit test Or we can also teach bot to check out the package container new unit tests if PR requires |
|
@smuzaffar I don't know how the PR in some other package can trigger unit tests in Fireworks/Geometry. Now I'm not even sure if reco geometry and sim geometry dumper are supposed to be in Fireworks module. I also don't know if more cmssw modules can cause the crashing of Fireworks reco geometry dumper. Should I post the question at cms mattermost? |
|
As workaround, we are removing the exception here: cmssw/Geometry/TrackerGeometryBuilder/src/TrackerGeometry.cc Lines 183 to 191 in 64d4f3b and skipping the tracker detIds that give |
so I packaged two (would-be) improvements in #45328:
|
|
type trk |
|
Hello! Any news on this? |
|
@cms-sw/trk-dpg-l2 @alja @bsunanda @cms-sw/upgrade-l2 The workaround we had planned to use does not work. We managed to dump the reco geometry in pre1 for D99 and use it in fireworks in pre6 (the only change wrt D110 is the tracker geometry version). We are observing big problems in calogeometry and we would urgently need fireworks to work again for D110 and other geometries to debug, develop and validate. |
|
@felicepantaleo can you post exactly how to reproduce the problem? I.e. what the command you issued? |
|
Hi @Dr15Jones And by replacing: from Configuration.AlCa.GlobalTag import GlobalTag
process.GlobalTag = GlobalTag(process.GlobalTag, 'auto:phase2_realistic_T33', ''), as suggested in #43097 (comment) I get [wredjeb@olhsw-21 python]$ cmsRun dumpRecoGeometry_cfg.py tag=2026 version=D110
Loading configuration for tag 2026 ...
Dumping geometry in cmsRecoGeom-2026.root
Begin processing the 1st record. Run 1, Event 1, LumiSection 1 on stream 0 at 23-Aug-2024 15:37:49.844 CEST
----- Begin Fatal Exception 23-Aug-2024 15:38:08 CEST-----------------------
An exception of category 'WrongTrackerSubDet' occurred while
[0] Processing Event run: 1 lumi: 1 event: 1 stream: 0
[1] Running path 'p'
[2] Prefetching for module DumpFWRecoGeometry/'dump'
[3] Calling method for EventSetup module FWRecoGeometryESProducer/''
Exception Message:
Invalid DetID: no GeomDetUnit associated with raw ID 303042564 of subdet ID 1
----- End Fatal Exception ------------------------------------------------- |
|
In principal, I see what caused the problem. The ID cmssw/Fireworks/Geometry/src/FWRecoGeometryESProducer.cc Lines 399 to 400 in dfc9a3c but then expects those to be available in but the only guarantee is those items are in So the question I have is what is the difference between a Det and a DetUnit ? The documentation of the code provides no explanation. Also, is id |
|
So it looks like address |
I am no geometry expert, but from the Tracker point of view a |
|
Let me notice also, that this issue seems to be linked to another issue I spotted at #45764 (comment), when trying to migrate the tracker misalignment tools to use this geometry D110. |
|
See #45784 for my attempt to fix the issue. The code changes skips over the composing detector components. |
|
We used the Fireworks dumper as a base for MkFit geometry dumper ... but there we ended up getting the tracker geometry from the TrackerDigiGeometryRecord -- and this one never seemed to have this problem. I was trying to look into this problem twice over the last two months (as HGCal folks told me this blocks their usage of D110 geometry in Fireworks) but the complexity of the setup and all the dependencies made it too hard for me to come to any meaningful conclusions. The double/glued modules in Phase1 have separate detids ... as the space-point hits made of the two strips of the glued modules use it. One tests this through TrackerTopology on det-subid, through a sub-det specific if-else-if: In mkFit we drop those, as we always work with strips only. In Fireworks we might (I'm not sure if we actually do) get products that reference those double-sided space-points. I still do not quite know what to think about it all ... I hope I'm not just confusing everyone :) |
|
And thank you all for diving into this! |
|
Thank you for debugging and correcting! The #45784 has the correction only for pixel barrel. I don't understand why the 'leaf' restriction is only for this detector? Is it possible there is a bug in the pixel barrel geometry definition? |
|
#45993 provides a better fix. |
it is already reverted in #45993 |
|
Can this issue be closed? |
I am having some problems in dumping a new Fireworks Geometry.
Release =
CMSSW_13_3_0_pre3Command:
cmsRun dumpRecoGeometry_cfg.py tag=2026 version=D98Error:
I am seeing the same thing in
CMSSW_13_2_0_pre3The text was updated successfully, but these errors were encountered: