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

Do not redefine twice in CMSSW the TkAl muon selectors #43878

Merged
merged 1 commit into from
Feb 9, 2024

Conversation

perrotta
Copy link
Contributor

@perrotta perrotta commented Feb 6, 2024

PR description:

The config in Alignment/APEEstimation/python/AlcaRecoSelection_cff.py redefines the two selectors:

  • TkAlGoodIdMuonSelector
  • TkAlRelCombIsoMuonSelector

already defined in Alignment/CommonAlignmentProducer/python/TkAlMuonSelectors_cfi.py

This could insert unwanted mistakes in case the two python configs are imported in the same workflow.

Those two selectors in the file touched here are not even needed themselves, because are just meant to get cloned into the actual selectors to be used in the mySeqALCARECOTkAlMuonIsolated sequence.

An objection raised by @mmusich in #43859 (comment) is that importing those selectors from Alignment/CommonAlignmentProducer could make the ALCARECOTkAlMuonIsolated* sequences depend on some other confinguration not fully under control of the APE team. (At the moment the selections are coded identically in the two python files).

My impression is that, if so, one should rather explicitly define all relevant parameters into the clone() function and avoid as such redefining the same module in two different places. Of course, if the TkAl people does not agree with me I will remove this PR.

PR validation:

At this moment the configurations and the sequences in Alignment/APEEstimation/python/AlcaRecoSelection_cff.py end up being identical to the previous ones. No further validation was attempted.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 6, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 6, 2024

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43878/38735

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 6, 2024

A new Pull Request was created by @perrotta (Andrea Perrotta) for master.

It involves the following packages:

  • Alignment/APEEstimation (alca)

@cmsbuild, @consuegs, @perrotta, @saumyaphor4252 can you please review it and eventually sign? Thanks.
@yuanchao, @rsreds, @adewit, @tocheng, @mmusich this is something you requested to watch as well.
@antoniovilela, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

@perrotta
Copy link
Contributor Author

perrotta commented Feb 6, 2024

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 6, 2024

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-92840a/37247/summary.html
COMMIT: 076b08c
CMSSW: CMSSW_14_0_X_2024-02-06-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/43878/37247/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

@perrotta
Copy link
Contributor Author

perrotta commented Feb 8, 2024

@mmusich do you (or the Tk alignment team in general) have any remark on this PR?
If you don't like it as such, an alternative could be to explicitely redefine all parameters in the cloned modules, so that everything will remain insensitive to era changes or so. But at least there will be no re-definitions of modules used elsewhere in CMSSW.

@mmusich
Copy link
Contributor

mmusich commented Feb 8, 2024

do you (or the Tk alignment team in general) have any remark on this PR?

I don't have remarks, actually after checking with @mteroerd it looks like this file is obsolete and targeted for deletion at the next round of updates, so feel free to modify it.

@perrotta
Copy link
Contributor Author

perrotta commented Feb 8, 2024

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 8, 2024

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

@rappoccio
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 49c7015 into cms-sw:master Feb 9, 2024
11 checks passed
@perrotta perrotta deleted the doNotDefineTwiceTkAlMuonSelectors branch February 9, 2024 18:27
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.

4 participants