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

Add a workaround for AlCa paths to the Patatrack customisation #35566

Merged

Conversation

fwyzard
Copy link
Contributor

@fwyzard fwyzard commented Oct 7, 2021

PR description:

The AlCa LumiPixelsCounts paths use the pixel cluster modules directly, without any Sequence or Task.
After applying the Patatrack customisation, this leaves those modules both "scheduled" (on the AlCa paths) and "unscheduled" (on every other path), breaking the latter.

To work around this issue, the Patatrack customisation now redefines the AlCa LumiPixelsCounts using the HLTDoLocalPixelSequence and the HLTDoLocalPixelTask, making the pixel modules unscheduled.

PR validation:

Applying the Patatrack customisation to the latest HLT menu and running over 2018 data now works.

@fwyzard
Copy link
Contributor Author

fwyzard commented Oct 7, 2021

enable gpu

@fwyzard
Copy link
Contributor Author

fwyzard commented Oct 7, 2021

please test

@fwyzard
Copy link
Contributor Author

fwyzard commented Oct 7, 2021

type bug-fix

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 7, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35566/25809

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 7, 2021

A new Pull Request was created by @fwyzard (Andrea Bocci) for master.

It involves the following packages:

  • HLTrigger/Configuration (hlt)

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

@@ -217,6 +217,32 @@ def customisePixelLocalReconstruction(process):
process.HLTDoLocalPixelSequence = cms.Sequence(process.HLTDoLocalPixelTask)


# workaround for AlCa paths

if 'AlCa_LumiPixelsCounts_Random_v1' in process.__dict__:
Copy link
Contributor

Choose a reason for hiding this comment

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

"v1" because newer versions are / will be already fixed: correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hopefully :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, we have NOt increased version numbers for a long time now...

Copy link
Contributor

Choose a reason for hiding this comment

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

So I understand this should go into ConfDB as an update to the paths in 12_1?

Copy link
Contributor

Choose a reason for hiding this comment

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

ie, for both cpu and gpu

Copy link
Contributor

Choose a reason for hiding this comment

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

and then it should be an overall customisation until then (ie, in customiseHLTforCMSSW)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume we are getting close to migrating the whole menu to the Task-enabled ConfDB; when that happens, this change can be applied there directly.
Until then, we can stay at "v1"...

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 7, 2021

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-034412/19457/summary.html
COMMIT: e9ee084
CMSSW: CMSSW_12_1_X_2021-10-06-2300/slc7_amd64_gcc900
Additional Tests: GPU
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35566/19457/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: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 4
  • DQMHistoTests: Total histograms compared: 19735
  • DQMHistoTests: Total failures: 6
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 19729
  • 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: no differences found

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 40
  • DQMHistoTests: Total histograms compared: 3219394
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3219371
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 39 files compared)
  • Checked 169 log files, 37 edm output root files, 40 DQM output files
  • TriggerResults: no differences found

@missirol
Copy link
Contributor

missirol commented Oct 8, 2021

Hi @fwyzard, this looks okay to me as is.

Just one clarification: in the way you rewrote the paths, the order of some filters is slightly different (for example, in HLT_FULL_cff the filter hltPixelTrackerHVOn is before the prescale filter); I assume this re-ordering won't need to be done in ConfDB?

@fwyzard
Copy link
Contributor Author

fwyzard commented Oct 8, 2021

Actually, I think it would make sense to reorder them in ConfDB as well: by having the hltPixelTrackerHVOn after the prescaler, we ensure that this path can use consistent prescales as all other paths seeded by zero bias.

@missirol
Copy link
Contributor

missirol commented Oct 8, 2021

Okay, thanks. Then, I guess I'll need to follow up with the group(s) responsible for the paths, and make this change via JIRA.

@missirol
Copy link
Contributor

missirol commented Oct 8, 2021

+hlt

  • workaround for the Patatrack customisation function, to be removed once the HLT configs are centrally moved to Tasks (and modules running scheduled inside paths are avoided)
  • noted that the order of filters in AlCa_LumiPixelsCounts_Random_v and AlCa_LumiPixelsCounts_ZeroBias_v should be corrected

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 8, 2021

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
Copy link
Contributor

perrotta commented Oct 8, 2021

+1

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.

None yet

5 participants