cmssw icon indicating copy to clipboard operation
cmssw copied to clipboard

Removed _cfi.py which were duplicate of auto-generated ones

Open Dr15Jones opened this issue 1 year ago • 6 comments

PR description:

  • fixed a configuration bug that only happened if fillDescriptions contained a call to addDefault and one and only one call to add.
  • removed _cfi.py files which had names matching auto-generated files in cfipython. If the two files were not identical, fillDescriptions was changed to call addDefault which contains the old auto-generated values and a add call which uses the values from the _cfi.py file. In this way, a call to import will get the same values as were given before this change.

PR validation:

Code compiles. The unit tests for the checked-out code all passed (except one which failed to find its input file via an xrootd read from FNAL).

Dr15Jones avatar May 30 '24 19:05 Dr15Jones

please test

Dr15Jones avatar May 30 '24 19:05 Dr15Jones

cms-bot internal usage

cmsbuild avatar May 30 '24 19:05 cmsbuild

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45112/40439

  • This PR adds an extra 36KB to repository

cmsbuild avatar May 30 '24 19:05 cmsbuild

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

It involves the following packages:

  • EventFilter/L1TRawToDigi (l1)
  • FWCore/ParameterSet (core)
  • Geometry/HcalEventSetup (geometry)
  • L1Trigger/L1TGlobal (l1)

@bsunanda, @smuzaffar, @aloeliger, @civanch, @makortel, @Dr15Jones, @epalencia, @mdhildreth can you please review it and eventually sign? Thanks. @eyigitba, @Martin-Grunewald, @bsunanda, @missirol, @makortel, @dinyar, @thomreis, @wddgit, @fabiocos this is something you requested to watch as well. @antoniovilela, @sextonkennedy, @rappoccio you are the release manager for this.

cms-bot commands are listed here

cmsbuild avatar May 30 '24 19:05 cmsbuild

-1

Failed Tests: RelVals RelVals-INPUT AddOn Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-145b3d/39636/summary.html COMMIT: f14e060c70c3e4eeb0971d20783de6b2636f17df CMSSW: CMSSW_14_1_X_2024-05-30-1100/el8_amd64_gcc12 User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/45112/39636/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals

----- Begin Fatal Exception 30-May-2024 22:15:53 CEST-----------------------
An exception of category 'EventSetupConflict' occurred while
   [0] Calling beginJob
Exception Message:
two EventSetup Producers want to deliver type="CaloTowerTopology" label=""
 from record HcalRecNumberingRecord. The two providers are 
1) type="CaloTowerTopologyEP" label=""
2) type="CaloTowerTopologyEP" label="CaloTowerTopology"
Please either
   remove one of these Producers
   or find a way of configuring one of them so it does not deliver this data
   or use an es_prefer statement in the configuration to choose one.
----- End Fatal Exception -------------------------------------------------
----- Begin Fatal Exception 30-May-2024 22:15:53 CEST-----------------------
An exception of category 'EventSetupConflict' occurred while
   [0] Calling beginJob
Exception Message:
two EventSetup Producers want to deliver type="CaloTowerTopology" label=""
 from record HcalRecNumberingRecord. The two providers are 
1) type="CaloTowerTopologyEP" label=""
2) type="CaloTowerTopologyEP" label="CaloTowerTopology"
Please either
   remove one of these Producers
   or find a way of configuring one of them so it does not deliver this data
   or use an es_prefer statement in the configuration to choose one.
----- End Fatal Exception -------------------------------------------------
----- Begin Fatal Exception 30-May-2024 22:16:07 CEST-----------------------
An exception of category 'EventSetupConflict' occurred while
   [0] Calling beginJob
Exception Message:
two EventSetup Producers want to deliver type="CaloTowerTopology" label=""
 from record HcalRecNumberingRecord. The two providers are 
1) type="CaloTowerTopologyEP" label=""
2) type="CaloTowerTopologyEP" label="CaloTowerTopology"
Please either
   remove one of these Producers
   or find a way of configuring one of them so it does not deliver this data
   or use an es_prefer statement in the configuration to choose one.
----- End Fatal Exception -------------------------------------------------
Expand to see more relval errors ...

RelVals-INPUT

  • 141.005141.005_RunHcalNZS2023B/step2_RunHcalNZS2023B.log
  • 141.006141.006_RunHLTPhysics2023B/step2_RunHLTPhysics2023B.log
  • 141.002141.002_RunZeroBias2023B/step2_RunZeroBias2023B.log
Expand to see more relval errors ...

AddOn Tests

----- Begin Fatal Exception 30-May-2024 22:16:15 CEST-----------------------
An exception of category 'EventSetupConflict' occurred while
   [0] Calling beginJob
Exception Message:
two EventSetup Producers want to deliver type="CaloTowerTopology" label=""
 from record HcalRecNumberingRecord. The two providers are 
1) type="CaloTowerTopologyEP" label=""
2) type="CaloTowerTopologyEP" label="CaloTowerTopology"
Please either
   remove one of these Producers
   or find a way of configuring one of them so it does not deliver this data
   or use an es_prefer statement in the configuration to choose one.
----- End Fatal Exception -------------------------------------------------
----- Begin Fatal Exception 30-May-2024 22:15:43 CEST-----------------------
An exception of category 'EventSetupConflict' occurred while
   [0] Calling beginJob
Exception Message:
two EventSetup Producers want to deliver type="CaloTowerTopology" label=""
 from record HcalRecNumberingRecord. The two providers are 
1) type="CaloTowerTopologyEP" label=""
2) type="CaloTowerTopologyEP" label="CaloTowerTopology"
Please either
   remove one of these Producers
   or find a way of configuring one of them so it does not deliver this data
   or use an es_prefer statement in the configuration to choose one.
----- End Fatal Exception -------------------------------------------------
----- Begin Fatal Exception 30-May-2024 22:14:32 CEST-----------------------
An exception of category 'EventSetupConflict' occurred while
   [0] Calling beginJob
Exception Message:
two EventSetup Producers want to deliver type="CaloTowerTopology" label=""
 from record HcalRecNumberingRecord. The two providers are 
1) type="CaloTowerTopologyEP" label=""
2) type="CaloTowerTopologyEP" label="CaloTowerTopology"
Please either
   remove one of these Producers
   or find a way of configuring one of them so it does not deliver this data
   or use an es_prefer statement in the configuration to choose one.
----- End Fatal Exception -------------------------------------------------
Expand to see more addon errors ...

cmsbuild avatar May 30 '24 21:05 cmsbuild

-1

this happens because now the HLT menu has explicitly:

process.CaloTowerTopologyEP = cms.ESProducer( "CaloTowerTopologyEP",
  appendToDataLabel = cms.string( "" )
)

while the rest of cmssw gets:

CaloTowerTopology = cms.ESProducer( "CaloTowerTopologyEP")

from the auto-generated cfi file from fillDescriptions.

https://github.com/cms-sw/cmssw/blob/0bce22cef41ee67a6a5392461812fe86f9803202/Geometry/HcalEventSetup/src/CaloTowerTopologyEP.cc#L38-L40

so either one of the two has to be renamed. I think renaming the one in the HLT menu via customization function is the least invasive solution.

For my own understanding I tried this on top of this PR:

diff --git a/HLTrigger/Configuration/python/customizeHLTforCMSSW.py b/HLTrigger/Configuration/python/customizeHLTforCMSSW.py
index b35dfef990f..d485059013d 100644
--- a/HLTrigger/Configuration/python/customizeHLTforCMSSW.py
+++ b/HLTrigger/Configuration/python/customizeHLTforCMSSW.py
@@ -268,6 +268,31 @@ def customizeHLTfor44576(process):
         break
     return process
 
+def customizeHLTfor45112(process):
+    # Collect module names to be modified
+    modules_to_rename = []
+
+    # Iterate over all the modules in the process
+    for module_name, module in process.__dict__.items():
+        if isinstance(module, cms.ESProducer):
+            # Check if the module is an instance of CaloTowerTopologyEP
+            if module.type_() == 'CaloTowerTopologyEP':
+                modules_to_rename.append(module_name)
+    
+    # Rename the modules after collecting their names
+    for module_name in modules_to_rename:
+        # Get the existing module
+        old_module = getattr(process, module_name)
+        # Create a new CaloTowerTopology module with the same parameters
+        new_module = cms.ESProducer("CaloTowerTopologyEP", **old_module.parameters_())
+        # Remove the old module
+        delattr(process, module_name)
+        # Add the new module with the desired name
+        new_module_name = "CaloTowerTopology"
+        setattr(process, new_module_name, new_module)
+
+    return process
+
 # CMSSW version specific customizations
 def customizeHLTforCMSSW(process, menuType="GRun"):
 
@@ -278,5 +303,6 @@ def customizeHLTforCMSSW(process, menuType="GRun"):
 
     process = checkHLTfor43774(process)
     process = customizeHLTfor44576(process)
-
+    process = customizeHLTfor45112(process)
+    
     return process

and both runTheMatrix.py -l 141.046 --ibeos and addOnTests.py -t hlt_data_GRun run OK. I am sure there are better solutions.

mmusich avatar May 30 '24 22:05 mmusich

Hmm, I guess the question is why the generated cfi file in cfipython changed from:

import FWCore.ParameterSet.Config as cms

CaloTowerTopology = cms.ESProducer('CaloTowerTopologyEP',
  appendToDataLabel = cms.string('')
)

to now:

import FWCore.ParameterSet.Config as cms

from .CaloTowerTopologyEP import CaloTowerTopologyEP

CaloTowerTopology = CaloTowerTopologyEP()

where somehow the appendToDataLabel = cms.string('') has gotten lost ??

Even if it is still here in Geometry/HcalEventSetup/CaloTowerTopologyEP.py

import FWCore.ParameterSet.Config as cms

def CaloTowerTopologyEP(**kwargs):
  mod = cms.ESProducer('CaloTowerTopologyEP',
    appendToDataLabel = cms.string('')
  )
  for k,v in kwargs.items():
    setattr(mod, k, v)
  return mod

Martin-Grunewald avatar May 31 '24 10:05 Martin-Grunewald

The underlying problem is the original file Geometry/HcalEventSetup/python/CaloTowerTopology_cfi.py violated our naming convention as the label applied to the module in the file was CaloTowerTopologyEP rather than CaloTowerTopology (the latter matching the name of the file).

I'll see how bad it is to changing the auto-generated code to use CaloTowerTopologyEP as a label and then change all import statements to use the new name. That would make the label consistent with what HLT is using now.

Dr15Jones avatar May 31 '24 13:05 Dr15Jones

Looking at the sheer quantity of places where CaloTowerTopology_cfi is imported, I decided to take a two prong approach.

  1. add back the CaloTowerTopology_cfi but have it just import from an autogenerated CaloTowerTopologyEP_cfi. This will allow this PR to function
  2. in another PR, change all import of CaloTowerTopology_cfi to CaloTowerTopologyEP_cfi. This will allow a cleaner separation of the work.

Dr15Jones avatar May 31 '24 13:05 Dr15Jones

please test

Dr15Jones avatar May 31 '24 13:05 Dr15Jones

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45112/40448

  • This PR adds an extra 16KB to repository

cmsbuild avatar May 31 '24 13:05 cmsbuild

Pull request #45112 was updated. @civanch, @aloeliger, @bsunanda, @epalencia, @smuzaffar, @mdhildreth, @makortel, @Dr15Jones can you please check and sign again.

cmsbuild avatar May 31 '24 13:05 cmsbuild

-1

Failed Tests: RelVals Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-145b3d/39646/summary.html COMMIT: 5d5a5e035626728d49cea9b836e3da7f589be44f CMSSW: CMSSW_14_1_X_2024-05-31-1100/el8_amd64_gcc12 User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/45112/39646/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals

  • 4.534.53_RunPhoton2012B/step2_RunPhoton2012B.log
  • 7.37.3_CosmicsSPLoose2018/step1_CosmicsSPLoose2018.log
  • 8.08.0_BeamHalo/step1_BeamHalo.log
Expand to see more relval errors ...

cmsbuild avatar May 31 '24 14:05 cmsbuild

These failures make no sense. cmsRun starts but then ends immediately with NO reasonable end code. The Framework Job Report only has the UnexpectedJobTermination which is actually just a filler we stick at the end of the FJR until the final exit status is sent to the service!

Dr15Jones avatar May 31 '24 15:05 Dr15Jones

These failures make no sense. cmsRun starts but then ends immediately with NO reasonable end code. The Framework Job Report only has the UnexpectedJobTermination which is actually just a filler we stick at the end of the FJR until the final exit status is sent to the service!

https://github.com/cms-sw/cmssw/issues/45116

mmusich avatar May 31 '24 15:05 mmusich

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-145b3d/39646/summary.html COMMIT: 5d5a5e035626728d49cea9b836e3da7f589be44f CMSSW: CMSSW_14_1_X_2024-05-31-1100/el8_amd64_gcc12 User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/45112/39646/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

There are some workflows for which there are errors in the baseline: 1000.0 step 2 1001.0 step 2 101.0 step 1 10224.0 step 1 11634.0 step 1 12434.0 step 1 12834.0 step 1 12834.7 step 1 12846.0 step 1 13034.0 step 1 1306.0 step 1 13234.0 step 1 1330.0 step 1 135.4 step 1 136.731 step 2 136.7611 step 2 136.793 step 2 136.8311 step 2 136.874 step 2 136.88811 step 2 139.001 step 2 140.023 step 2 140.043 step 2 140.063 step 2 140.56 step 2 14034.0 step 1 141.042 step 2 141.044 step 2 141.046 step 2 14234.0 step 1 158.01 step 2 23234.0 step 1 24834.0 step 1 24834.911 step 1 24896.0 step 1 24900.0 step 1 25.0 step 1 2500.4 step 2 250202.181 step 1 25034.999 step 1 25202.0 step 1 312.0 step 1 4.22 step 2 4.53 step 2 5.1 step 1 7.3 step 1 8.0 step 1 9.0 step 1 The results for the comparisons for these workflows could be incomplete This means most likely that the IB is having errors in the relvals.The error does NOT come from this pull request

Summary:

  • You potentially added 19179 lines to the logs
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 1
  • DQMHistoTests: Total histograms compared: 0
  • DQMHistoTests: Total failures: 0
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 0
  • DQMHistoTests: Total skipped: 0
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0 KiB( 0 files compared)
  • Checked 66 log files, 0 edm output root files, 1 DQM output files

cmsbuild avatar May 31 '24 17:05 cmsbuild

@cmsbuild, please test

makortel avatar Jun 03 '24 14:06 makortel

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-145b3d/39676/summary.html COMMIT: 5d5a5e035626728d49cea9b836e3da7f589be44f CMSSW: CMSSW_14_1_X_2024-06-03-1100/el8_amd64_gcc12 User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/45112/39676/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 3 lines from the logs
  • Reco comparison results: 11 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3338862
  • DQMHistoTests: Total failures: 9
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3338833
  • 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 avatar Jun 03 '24 17:06 cmsbuild

+core

makortel avatar Jun 05 '24 13:06 makortel

+geometry

civanch avatar Jun 06 '24 08:06 civanch

@cms-sw/l1-l2 ping

Dr15Jones avatar Jun 07 '24 16:06 Dr15Jones

+l1

aloeliger avatar Jun 10 '24 08:06 aloeliger

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

cmsbuild avatar Jun 10 '24 08:06 cmsbuild

+1

antoniovilela avatar Jun 11 '24 15:06 antoniovilela