updates to `customizeHLTforPatatrack` to limit overwriting of HLT modules #36721

missirol · 2022-01-17T10:58:59Z

PR description:

This PR updates customizeHLTforPatatrack to limit cases where the customisation overwrites modules that may already exist in the HLT menu (e.g. the cpu 'branch' of certain SwitchProducers).

Reminder : currently, the HLT menu in cmssw:master contains the CPU part of the Patatrack developments (in other words, it contains SwitchProducers that only have a cpu branch, not a cuda one).

With this update, if a certain object exists already, it is not overwritten anymore, giving precedence to what is in the input configuration (usually, the HLT menu from ConfDB).

In particular, this resolves an action item from CMSHLT-2187, regarding the value of the parameter hltPixelTracksSoA.cpu.idealConditions; the latter is set to False in ConfDB, but is currently changed to True in customizeHLTforPatatrack; with this PR, the parameter remains as in the HLT menu if it is already present (i.e. False with the latest menus).

The PR also includes

an addition to customisePixelGainForRun2Input, to ensure that the parameter (SiPixelRawToClusterCUDA).isRun2 is set to True when Run-2 inputs are used (this would be needed once instances of SiPixelRawToClusterCUDA will come directly from the HLT config in ConfDB, with isRun2 set to False);
a bugfix for the handling of process.hltEcalRecHit.cuda (the bug was introduced in make customizeHLTforPatatrack even more robust #36536).

Changes in outputs are expected for wfs where customizeHLTforPatatrack is applied to the GRun menu, e.g. wf 11634.{506,512,522} (for example, because of hltPixelTracksSoA.cpu.idealConditions changing from True to False).

Profited from suggestions by @fwyzard.

FYI: @silviodonato

PR validation:

runTheMatrix.py -w gpu -l 11634.506,11634.512,11634.522, plus manual tests with hltGetConfiguration [..] --customise [..].

If this PR is a backport, please specify the original PR and why you need to backport that PR:

N/A

missirol · 2022-01-17T10:59:35Z

type bugfix

a bugfix for the handling of process.hltEcalRecHit.cuda (the bug was introduced in #36536).

missirol · 2022-01-17T11:02:59Z

HLTrigger/Configuration/python/customizeHLTforPatatrack.py

+        process.hltEcalRecHit.cuda = process.hltEcalRecHit.cpu.clone(
+            triggerPrimitiveDigiCollection = 'unused'
+        )
+


a bugfix for the handling of process.hltEcalRecHit.cuda (the bug was introduced in #36536).

After #36536, process.hltEcalRecHit.cuda would be added only if process.hltEcalRecHit was not a SwitchProducer in the input configuration. This condition is false in the latest menus, so for those process.hltEcalRecHit.cuda was not being added.

cmsbuild · 2022-01-17T11:03:51Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36721/27818

This PR adds an extra 20KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File HLTrigger/Configuration/python/customizeHLTforPatatrack.py modified in PR(s): Add a generic mechanism to specify compute accelerators to use in the configuration #36699

cmsbuild · 2022-01-17T11:04:11Z

A new Pull Request was created by @missirol (Marino Missiroli) for master.

It involves the following packages:

HLTrigger/Configuration (hlt)

@cmsbuild, @missirol, @Martin-Grunewald can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @silviodonato 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

missirol · 2022-01-17T11:06:47Z

HLTrigger/Configuration/python/customizeHLTforPatatrack.py

+    elif hasattr(process.hltPixelTracksSoA, 'cpu'):
+        # if cpu branch of SwitchProducerCUDA exists, take hltPixelTracksCUDA (gpu)
+        # from hltPixelTracksSoA.cpu (cpu) to enforce same configuration parameters
+        process.hltPixelTracksCUDA = process.hltPixelTracksSoA.cpu.clone(
+            pixelRecHitSrc = "hltSiPixelRecHitsCUDA",
+            onGPU = True
+        )


This relates to the treatment of the value of process.hltPixelTracksSoA.idealConditions. If process.hltPixelTracksSoA.cpu already exists, it is not modified, and the GPU module (hltPixelTracksCUDA) is configured based on process.hltPixelTracksSoA.cpu.

fwyzard · 2022-01-28T09:40:29Z

You could define some helper functions to simplify the customisation:

def load_if_mising(process, label, config):
if label not in process.__dict__:
    process.load(config)

def load_if_missing_and_clone(_process, _config, _label, _alias = None, **kwargs):
    if _alias is None:
        _alias = _label
    if _alias not in _process.__dict__:
        module = __import__(_config, globals(), locals(), [ _label ], 0)
        setattr(_process, _alias, getattr(module, _label).clone(**kwargs)

to be used as

# these should be used only on GPUs, will crash otherwise
load_if_mising(process, 'siPixelGainCalibrationForHLTGPU', 'CalibTracker.SiPixelESProducers.siPixelGainCalibrationForHLTGPU_cfi')
load_if_mising(process, 'siPixelROCsStatusAndMappingWrapperESProducer', 'CalibTracker.SiPixelESProducers.siPixelROCsStatusAndMappingWrapperESProducer_cfi')
...

# transfer the beamspot to the gpu
load_if_missing_and_clone(process, 'RecoVertex.BeamSpotProducer.offlineBeamSpotToCUDA_cfi', 'offlineBeamSpotToCUDA', 'hltOnlineBeamSpotToCUDA',
    src = "hltOnlineBeamSpot"
)

Please note that I did not test that they actually work... and maybe you can find more compact names :-)

I didn't try to come up with something that support a SwitchProducer, but I can look into it if needed.

missirol · 2022-01-28T14:01:40Z

Thanks for the suggestion, @fwyzard . I'm trying to implement it.

What do you think about the following?

# Utility to load file "config" in the process,
# if the process does not already hold a module named "label"
# Example:
#   _load_if_missing(process, 'SomeESProducer', 'SubSystem.Package.SomeESProducer_cfi')
def _load_if_missing(process, label, config, check = True):
    if label not in process.__dict__:
        process.load(config)
    if check and label not in process.__dict__:
        raise Exception('process does not have a module labelled "'+label+'" after loading "'+config+'"')

# Utility to add "member" to "obj" (if "member" is missing),
# using a clone of module "label" from file "config"
# (if "label" is not specified, it is set equal to "member")
# Example:
#   _clone_if_missing(process, 'hltMyFilter', 'SubSystem.Package.hltSomeFilter_cfi', 'hltSomeFilter', src = 'mySrc')
def _clone_if_missing(obj, member, config, label = None, **kwargs):
    if label is None:
        label = member
    if not hasattr(obj, member):
        module = __import__(config, globals(), locals(), [ label ], 0)
        setattr(obj, member, getattr(module, label).clone(**kwargs))

The last function is slightly changed wrt to the original suggestion (also, from your snippet I didn't grasp if starting the function arguments with _ is important). It would allow to do both

_clone_if_missing(process, 'hltOnlineBeamSpotToCUDA', 'RecoVertex.BeamSpotProducer.offlineBeamSpotToCUDA_cfi', 'offlineBeamSpotToCUDA',
    src = "hltOnlineBeamSpot"
)

and

_clone_if_missing(process.hltSiPixelDigis, 'cuda', 'EventFilter.SiPixelRawToDigi.siPixelDigiErrorsFromSoA_cfi', 'siPixelDigiErrorsFromSoA',
    digiErrorSoASrc = "hltSiPixelDigiErrorsSoA",
    UsePhase1 = True
)

.. so, one part of the SwitchProducer customisations.

fwyzard · 2022-01-28T14:04:57Z

from your snippet I didn't grasp if starting the function arguments with _ is important

that's to make it less likely that a function argument would clash with an argument intended for clone()

fwyzard · 2022-01-28T14:10:11Z

I'd suggest

def _clone_if_missing(_obj, _label, _config, _original_label = None, **kwargs):

to make the naming more consistent among the two functions.

cmsbuild · 2022-01-28T17:47:31Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36721/27996

This PR adds an extra 20KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File HLTrigger/Configuration/python/customizeHLTforPatatrack.py modified in PR(s): Add a generic mechanism to specify compute accelerators to use in the configuration #36699

cmsbuild · 2022-01-28T17:47:50Z

Pull request #36721 was updated. @cmsbuild, @missirol, @Martin-Grunewald can you please check and sign again.

cmsbuild · 2022-02-09T20:55:49Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36721/28233

This PR adds an extra 20KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File HLTrigger/Configuration/python/customizeHLTforPatatrack.py modified in PR(s): Add a generic mechanism to specify compute accelerators to use in the configuration #36699

cmsbuild · 2022-02-09T20:56:11Z

Pull request #36721 was updated. @cmsbuild, @missirol, @Martin-Grunewald can you please check and sign again.

missirol · 2022-02-09T21:05:29Z

please test

Rebased, and improved handling of parameters for timing of ECAL RecHits (thanks to suggestions from Silvio and Andrea M.), in view of the upcoming updates in CMSHLT-2211.

@amassiro (cc: @fwyzard, @silviodonato), please have a look at 579db0b, and let me know if you have comments/corrections. Note that, in this customisation, hltEcalRecHit.cuda is cloned from hltEcalRecHit.cpu, so there is no need to edit hltEcalRecHit.cuda.skipTimeCalib explicitly.

cmsbuild · 2022-02-10T00:59:06Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b93920/22313/summary.html
COMMIT: 579db0b
CMSSW: CMSSW_12_3_X_2022-02-09-1100/slc7_amd64_gcc10
Additional Tests: GPU
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36721/22313/install.sh to create a dev area with all the needed externals and cmssw changes.

GPU Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 24 differences found in the comparisons
DQMHistoTests: Total files compared: 4
DQMHistoTests: Total histograms compared: 19811
DQMHistoTests: Total failures: 953
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 18858
DQMHistoTests: Total skipped: 0
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 3 files compared)
Checked 12 log files, 9 edm output root files, 4 DQM output files
TriggerResults: found differences in 2 / 3 workflows

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 6 differences found in the comparisons
DQMHistoTests: Total files compared: 46
DQMHistoTests: Total histograms compared: 3765080
DQMHistoTests: Total failures: 13
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3765044
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 45 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 193 log files, 42 edm output root files, 46 DQM output files
TriggerResults: no differences found

missirol · 2022-02-11T18:08:27Z

please test

re-running the tests, as the latest IBs include updates to the HLT menu (wrt the last tests of this PR)

cmsbuild · 2022-02-11T22:38:11Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b93920/22363/summary.html
COMMIT: 579db0b
CMSSW: CMSSW_12_3_X_2022-02-11-1400/slc7_amd64_gcc10
Additional Tests: GPU
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36721/22363/install.sh to create a dev area with all the needed externals and cmssw changes.

GPU Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 6 differences found in the comparisons
DQMHistoTests: Total files compared: 4
DQMHistoTests: Total histograms compared: 19811
DQMHistoTests: Total failures: 1927
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 17884
DQMHistoTests: Total skipped: 0
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 3 files compared)
Checked 12 log files, 9 edm output root files, 4 DQM output files
TriggerResults: found differences in 2 / 3 workflows

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 46
DQMHistoTests: Total histograms compared: 3764395
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3764371
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 45 files compared)
Checked 193 log files, 42 edm output root files, 46 DQM output files
TriggerResults: no differences found

missirol · 2022-02-12T07:22:30Z

+hlt

Improvement of the GPU customisation functions in order to (1) avoid overwriting the content of the HLT menu, and (2) better ensure consistent configurations of CPU and GPU modules.
The PR also fixes a small bug, see updates to customizeHLTforPatatrack to limit overwriting of HLT modules #36721 (comment).
Differences in (GPU) wfs using this customisation function are expected and observed, see updates to customizeHLTforPatatrack to limit overwriting of HLT modules #36721 (comment).
TSG would like to have this PR in CMSSW_12_3_0_pre5; this customisation function is used (among other things) for timing studies.

cmsbuild · 2022-02-12T07:22:53Z

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 · 2022-02-13T07:42:00Z

+1

cmsbuild added this to the CMSSW_12_3_X milestone Jan 17, 2022

cmsbuild added code-checks-pending hlt-pending orp-pending pending-signatures tests-pending labels Jan 17, 2022

cmsbuild added the bug-fix label Jan 17, 2022

missirol commented Jan 17, 2022

View reviewed changes

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 17, 2022

missirol commented Jan 17, 2022

View reviewed changes

cmsbuild mentioned this pull request Jan 20, 2022

HLT development for 12_3_X + migration to 12_3_0_pre3 template #36731

Merged

silviodonato mentioned this pull request Jan 24, 2022

reproducibility of GPU vs CPU results at HLT #35376

Open

cmsbuild mentioned this pull request Jan 26, 2022

HLT menu development for 12_3_X #36807

Merged

missirol force-pushed the devel_hltCustomPatatrackLimitOverrides2 branch from 1556946 to e3f38a0 Compare January 28, 2022 17:40

cmsbuild added code-checks-pending and removed code-checks-approved labels Jan 28, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 28, 2022

cmsbuild added code-checks-pending and removed code-checks-approved labels Jan 28, 2022

cmsbuild added code-checks-pending tests-pending labels Feb 9, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 9, 2022

cmsbuild added tests-started and removed tests-pending labels Feb 9, 2022

cmsbuild added tests-approved and removed tests-started labels Feb 10, 2022

cmsbuild added tests-started and removed tests-approved labels Feb 11, 2022

cmsbuild added tests-approved and removed tests-started labels Feb 11, 2022

cmsbuild added fully-signed hlt-approved and removed hlt-pending pending-signatures labels Feb 12, 2022

cmsbuild added orp-approved and removed orp-pending labels Feb 13, 2022

cmsbuild merged commit e84715c into cms-sw:master Feb 13, 2022

cmsbuild mentioned this pull request Feb 14, 2022

Add const to char* to fix compiler errors with PyString_AsString cms-externals/coral#7

Merged

missirol deleted the devel_hltCustomPatatrackLimitOverrides2 branch March 27, 2022 13:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

updates to `customizeHLTforPatatrack` to limit overwriting of HLT modules #36721

updates to `customizeHLTforPatatrack` to limit overwriting of HLT modules #36721

missirol commented Jan 17, 2022

missirol commented Jan 17, 2022

missirol Jan 17, 2022

cmsbuild commented Jan 17, 2022

cmsbuild commented Jan 17, 2022

missirol Jan 17, 2022 •

edited

Loading

fwyzard commented Jan 28, 2022

missirol commented Jan 28, 2022

fwyzard commented Jan 28, 2022

fwyzard commented Jan 28, 2022 •

edited

Loading

cmsbuild commented Jan 28, 2022

cmsbuild commented Jan 28, 2022

cmsbuild commented Feb 9, 2022

cmsbuild commented Feb 9, 2022

missirol commented Feb 9, 2022

cmsbuild commented Feb 10, 2022

missirol commented Feb 11, 2022

cmsbuild commented Feb 11, 2022

missirol commented Feb 12, 2022

cmsbuild commented Feb 12, 2022

perrotta commented Feb 13, 2022

updates to customizeHLTforPatatrack to limit overwriting of HLT modules #36721

updates to customizeHLTforPatatrack to limit overwriting of HLT modules #36721

Conversation

missirol commented Jan 17, 2022

PR description:

PR validation:

If this PR is a backport, please specify the original PR and why you need to backport that PR:

missirol commented Jan 17, 2022

missirol Jan 17, 2022

Choose a reason for hiding this comment

cmsbuild commented Jan 17, 2022

cmsbuild commented Jan 17, 2022

missirol Jan 17, 2022 • edited Loading

Choose a reason for hiding this comment

fwyzard commented Jan 28, 2022

missirol commented Jan 28, 2022

fwyzard commented Jan 28, 2022

fwyzard commented Jan 28, 2022 • edited Loading

cmsbuild commented Jan 28, 2022

cmsbuild commented Jan 28, 2022

cmsbuild commented Feb 9, 2022

cmsbuild commented Feb 9, 2022

missirol commented Feb 9, 2022

cmsbuild commented Feb 10, 2022

GPU Comparison Summary

Comparison Summary

missirol commented Feb 11, 2022

cmsbuild commented Feb 11, 2022

GPU Comparison Summary

Comparison Summary

missirol commented Feb 12, 2022

cmsbuild commented Feb 12, 2022

perrotta commented Feb 13, 2022

updates to `customizeHLTforPatatrack` to limit overwriting of HLT modules #36721

updates to `customizeHLTforPatatrack` to limit overwriting of HLT modules #36721

missirol Jan 17, 2022 •

edited

Loading

fwyzard commented Jan 28, 2022 •

edited

Loading