fixing removal of modules from `FinalPath` #37022

missirol · 2022-02-22T08:57:03Z

PR description:

This PR follows #36932 and #36937, and concerns the removal of a module from a FinalPath.

It looks like said removal does not work correctly in 12_3_0_pre5, as [1] returns [2] (the issue was reported by @sanuvarghese from TSG).

The implementation in this PR is basically a copy-paste of the solution provided by @Dr15Jones in #36937.

The PR is mainly a way to point out the issue. I'm happy to close it if it's incorrect/incomplete, of if experts prefer to take care of the implementation of this fix.

FYI: @Martin-Grunewald @Sam-Harper @silviodonato

[1] Example borrowed from #36932

import FWCore.ParameterSet.Config as cms

process = cms.Process("HLT")

process.dqmOutput = cms.OutputModule("DQMRootOutputModule",
    fileName = cms.untracked.string("DQMIO.root")
)

process.DQMOutput = cms.FinalPath( process.dqmOutput )

process.DQMOutput.remove(process.dqmOutput)

[2]

Traceback (most recent call last):
  File "tmp.py", line 11, in <module>
    process.DQMOutput.remove(process.dqmOutput)
  File "/cvmfs/cms.cern.ch/slc7_amd64_gcc10/cms/cmssw/CMSSW_12_3_0_pre5/python/FWCore/ParameterSet/SequenceTypes.py", line 500, in remove
    self.associate(*v.result(self)[1])
TypeError: associate() missing 1 required positional argument: 'task'

PR validation:

The example in [1] works, and unit tests of FWCore/ParameterSet pass.

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

N/A

cmsbuild · 2022-02-22T09:04:50Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37022/28449

This PR adds an extra 28KB to repository

cmsbuild · 2022-02-22T09:05:15Z

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

It involves the following packages:

FWCore/ParameterSet (core)

@cmsbuild, @smuzaffar, @Dr15Jones, @makortel can you please review it and eventually sign? Thanks.
@makortel, @wddgit 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

makortel · 2022-02-22T14:14:50Z

Thanks @missirol! @Dr15Jones, could you take a look?

makortel · 2022-02-22T14:14:57Z

@cmsbuild, please test

Dr15Jones · 2022-02-22T15:00:57Z

FWCore/ParameterSet/python/SequenceTypes.py

+            fp = FinalPath(m1)
+            fp.remove(m1)
+            self.assertEqual(fp.dumpPython(), "cms.FinalPath()\n")


Could you extend the test to also cover the case where there are multiple items in the FinalPath and only one is removed? I just want to test the if branch

Suggested change

fp = FinalPath(m1)

fp.remove(m1)

self.assertEqual(fp.dumpPython(), "cms.FinalPath()\n")

fp = FinalPath(m1+m2)

fp.remove(m1)

self.assertEqual(fp.dumpPython(), "cms.FinalPath(process.m2)\n")

fp = FinalPath(m1)

fp.remove(m1)

self.assertEqual(fp.dumpPython(), "cms.FinalPath()\n")

Thanks. Done by force-push to minimise commits.

cmsbuild · 2022-02-22T15:36:48Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37022/28458

This PR adds an extra 28KB to repository

cmsbuild · 2022-02-22T15:37:08Z

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

makortel · 2022-02-22T15:40:35Z

@cmsbuild, please abort

makortel · 2022-02-22T15:40:41Z

@cmsbuild, please test

missirol · 2022-02-22T18:59:26Z

The bot reports 1 error, in the (HLT) addOnTest called hlt_mc_PRef, but I can't reproduce that locally (and don't know how that could be caused by this PR).

cmsbuild · 2022-02-22T19:42:37Z

-1

Failed Tests: AddOn
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ce79da/22582/summary.html
COMMIT: 43db69b
CMSSW: CMSSW_12_3_X_2022-02-22-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37022/22582/install.sh to create a dev area with all the needed externals and cmssw changes.

AddOn Tests

hlt_mc_PRefcmsDriver.py RelVal -s HLT:PRef,RAW2DIGI,L1Reco,RECO --mc --scenario=pp -n 10 --conditions auto:run3_mc_PRef --relval 9000,50 --datatier "RAW-HLT-RECO" --eventcontent FEVTDEBUGHLT --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --era Run3 --processName=HLTRECO --filein file:RelVal_Raw_PRef_MC.root --fileout file:RelVal_Raw_PRef_MC_HLT_RECO.root : FAILED - time: date Tue Feb 22 17:03:56 2022-date Tue Feb 22 16:56:28 2022 s - exit: 35584

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3965143
DQMHistoTests: Total failures: 7
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3965113
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 48 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 204 log files, 45 edm output root files, 49 DQM output files
TriggerResults: no differences found

Martin-Grunewald · 2022-02-22T19:48:55Z

Module: MkFitProducer:pixelLessStepTrackCandidatesMkFit (crashed)
Module: MkFitProducer:highPtTripletStepTrackCandidatesMkFit
Module: MultiHitFromChi2EDProducer:pixelLessStepHitTriplets
Module: MkFitProducer:highPtTripletStepTrackCandidatesMkFit

MkFit has introduced some problems, as discussed in the ORP today.

Martin-Grunewald · 2022-02-22T19:49:17Z

please test

missirol · 2022-02-22T19:54:05Z

Regarding my previous comment, I should add that I was testing this PR on top of plain 12_3_0_pre5, not a recent IB.

Dr15Jones · 2022-02-22T19:55:40Z

As long as it passes the FWCore/ParameterSet unit tests, I'm happy.

cmsbuild · 2022-02-23T00:10:05Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ce79da/22590/summary.html
COMMIT: 43db69b
CMSSW: CMSSW_12_3_X_2022-02-22-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37022/22590/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: 49
DQMHistoTests: Total histograms compared: 3965143
DQMHistoTests: Total failures: 8
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3965113
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 204 log files, 45 edm output root files, 49 DQM output files
TriggerResults: no differences found

Backport to CMSSW_12_2_X of the following PRs: - cms-sw#36937 (b1f49c8) - cms-sw#37022 (43db69b)

missirol · 2022-02-23T07:46:31Z

type bugfix

I took the liberty to open a backport of these FinalPath fixes to 12_2_X, because HLT makes use of FinalPath in that release cycle, too: #37043.

Dr15Jones · 2022-02-23T13:49:45Z

+1

cmsbuild · 2022-02-23T13:50:06Z

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)

qliphy · 2022-02-24T01:39:02Z

+1

cmsbuild added this to the CMSSW_12_3_X milestone Feb 22, 2022

cmsbuild added code-checks-pending core-pending orp-pending pending-signatures tests-pending labels Feb 22, 2022

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

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

Dr15Jones reviewed Feb 22, 2022

View reviewed changes

fixed removal of modules from FinalPath

43db69b

missirol force-pushed the devel_removeFromFinalPath branch from a4585a6 to 43db69b Compare February 22, 2022 15:28

cmsbuild added code-checks-pending tests-pending and removed tests-started code-checks-approved labels Feb 22, 2022

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

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

cmsbuild added tests-rejected and removed tests-started labels Feb 22, 2022

cmsbuild added tests-started and removed tests-rejected labels Feb 22, 2022

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

missirol added a commit to missirol/cmssw that referenced this pull request Feb 23, 2022

fixes for replace/remove with FinalPath

500b117

Backport to CMSSW_12_2_X of the following PRs: - cms-sw#36937 (b1f49c8) - cms-sw#37022 (43db69b)

missirol mentioned this pull request Feb 23, 2022

fixes for replace/remove with FinalPath [12_2_X] #37043

Merged

cmsbuild added the bug-fix label Feb 23, 2022

cmsbuild added core-approved fully-signed and removed core-pending pending-signatures labels Feb 23, 2022

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

cmsbuild merged commit cf9ed10 into cms-sw:master Feb 24, 2022

This was referenced Feb 24, 2022

Add UL pixel resolution histograms for all years cms-data/FastSimulation-TrackingRecHitProducer#4

Closed

DAQ - fixing streamer count of HLT paths with the GlobalEvFOutputModule (12_3_X) #37054

Merged

missirol deleted the devel_removeFromFinalPath branch February 25, 2022 10:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixing removal of modules from `FinalPath` #37022

fixing removal of modules from `FinalPath` #37022

missirol commented Feb 22, 2022

cmsbuild commented Feb 22, 2022

cmsbuild commented Feb 22, 2022

makortel commented Feb 22, 2022

makortel commented Feb 22, 2022

Dr15Jones Feb 22, 2022

missirol Feb 22, 2022

cmsbuild commented Feb 22, 2022

cmsbuild commented Feb 22, 2022

makortel commented Feb 22, 2022

makortel commented Feb 22, 2022

missirol commented Feb 22, 2022

cmsbuild commented Feb 22, 2022

Martin-Grunewald commented Feb 22, 2022 •

edited

Martin-Grunewald commented Feb 22, 2022

missirol commented Feb 22, 2022

Dr15Jones commented Feb 22, 2022

cmsbuild commented Feb 23, 2022

missirol commented Feb 23, 2022

Dr15Jones commented Feb 23, 2022

cmsbuild commented Feb 23, 2022

qliphy commented Feb 24, 2022

fixing removal of modules from FinalPath #37022

fixing removal of modules from FinalPath #37022

Conversation

missirol commented Feb 22, 2022

PR description:

PR validation:

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

cmsbuild commented Feb 22, 2022

cmsbuild commented Feb 22, 2022

makortel commented Feb 22, 2022

makortel commented Feb 22, 2022

Dr15Jones Feb 22, 2022

Choose a reason for hiding this comment

missirol Feb 22, 2022

Choose a reason for hiding this comment

cmsbuild commented Feb 22, 2022

cmsbuild commented Feb 22, 2022

makortel commented Feb 22, 2022

makortel commented Feb 22, 2022

missirol commented Feb 22, 2022

cmsbuild commented Feb 22, 2022

AddOn Tests

Comparison Summary

Martin-Grunewald commented Feb 22, 2022 • edited

Martin-Grunewald commented Feb 22, 2022

missirol commented Feb 22, 2022

Dr15Jones commented Feb 22, 2022

cmsbuild commented Feb 23, 2022

Comparison Summary

missirol commented Feb 23, 2022

Dr15Jones commented Feb 23, 2022

cmsbuild commented Feb 23, 2022

qliphy commented Feb 24, 2022

fixing removal of modules from `FinalPath` #37022

fixing removal of modules from `FinalPath` #37022

Martin-Grunewald commented Feb 22, 2022 •

edited