Skip to content

add fillDescriptions to several plugins used at HLT (4/N) #47107

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

Merged
merged 6 commits into from
Jan 17, 2025

Conversation

mmusich
Copy link
Contributor

@mmusich mmusich commented Jan 15, 2025

PR description:

This PR is similar in spirit to #47017, #47045 and #47079. It adds fillDescriptions (and applies light modification to modernize the source code) to a bunch of plugins used at HLT for both Run 3 and Phase 2.

PR validation:

  • addOnTests.py runs fine.
  • hltPhase2UpgradeIntegrationTests runs fine.

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

N/A

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 15, 2025

cms-bot internal usage

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47107/43295

  • There are other open Pull requests which might conflict with changes you have proposed:

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • HLTrigger/Configuration (hlt)
  • RecoEgamma/EgammaPhotonProducers (reconstruction)
  • RecoLocalCalo/EcalRecProducers (reconstruction)
  • RecoTracker/TrackProducer (reconstruction)
  • Validation/RecoTrack (dqm)

@Martin-Grunewald, @antoniovagnerini, @cmsbuild, @jfernan2, @mandrenguyen, @mmusich, @rseidita can you please review it and eventually sign? Thanks.
@GiacomoSguazzoni, @Martin-Grunewald, @Prasant1993, @ReyerBand, @Sam-Harper, @SohamBhattacharya, @VinInn, @VourMa, @a-kapoor, @afiqaize, @apsallid, @argiro, @denizsun, @dgulhan, @felicepantaleo, @gpetruc, @jainshilpi, @lgray, @missirol, @mmusich, @mtosi, @ram1123, @rchatter, @rovere, @salimcerci, @sameasy, @sobhatta, @thomreis, @valsdav, @varuns23, @wang0jin, @wmtford, @youyingli this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

@mmusich
Copy link
Contributor Author

mmusich commented Jan 15, 2025

@cmsbuild, please test

@Martin-Grunewald
Copy link
Contributor

Seems my comments are stuck in 'pending'.

In the customisation routine:

How is this supposed to work? It looks like you remove the top-level parameter 'SimpleMagneticField' based on values/existance of other parameters. ConfDB does not allow such a conditional cross dependency: either the top=level parameter is there or it is not there, independent of other top-level parameters.
??

@mmusich
Copy link
Contributor Author

mmusich commented Jan 15, 2025

It looks like you remove the top-level parameter 'SimpleMagneticField' based on values/existance of other parameters. ConfDB does not allow such a conditional cross dependency: either the top=level parameter is there or it is not there, independent of other top-level parameters.

this is the whole concept of ifValue (see https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideConfigurationValidationAndHelp#A_Switch). I doubt this is the first time this is used across cmssw. I wonder how it works elsewhere.

@Martin-Grunewald
Copy link
Contributor

Yes, elsewhere in CMSSW, but unfortunately ConfDb fixes the top-level parameters in the plugin template to be a unique set. So at HLT we can have only one!

@mmusich
Copy link
Contributor Author

mmusich commented Jan 15, 2025

Yes, elsewhere in CMSSW, but unfortunately ConfDb fixes the top-level parameters in the plugin template to be a unique set.

What I mean, is that I think it's possible that there are already modules used at HLT that have the same feature in their fillDescriptions. We'll have to see then if there aren't other potential issue in other modules that use the same setup but just so happen to have not been used in the "non-default" setup.

@Martin-Grunewald
Copy link
Contributor

So far it seems we have gotten away with this (unless NOT discovered during validation).

@cmsbuild
Copy link
Contributor

-1

Failed Tests: RelVals
Size: This PR adds an extra 96KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5a9282/43768/summary.html
COMMIT: ba296ae
CMSSW: CMSSW_15_0_X_2025-01-14-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/47107/43768/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals

----- Begin Fatal Exception 15-Jan-2025 14:38:34 CET-----------------------
An exception of category 'Configuration' occurred while
   [0] Constructing the EventProcessor
   [1] Validating configuration of module: class=GsfTrackProducer label='hltEgammaGsfTracksUnseeded'
Exception Message:
Illegal parameter found in configuration.  The parameter is named:
 'producer'
You could be trying to use a parameter name that is not
allowed for this plugin or it could be misspelled.
----- End Fatal Exception -------------------------------------------------
----- Begin Fatal Exception 15-Jan-2025 14:38:35 CET-----------------------
An exception of category 'Configuration' occurred while
   [0] Constructing the EventProcessor
   [1] Validating configuration of module: class=GsfTrackProducer label='hltEgammaGsfTracksUnseeded'
Exception Message:
Illegal parameter found in configuration.  The parameter is named:
 'producer'
You could be trying to use a parameter name that is not
allowed for this plugin or it could be misspelled.
----- End Fatal Exception -------------------------------------------------
----- Begin Fatal Exception 15-Jan-2025 14:33:04 CET-----------------------
An exception of category 'Configuration' occurred while
   [0] Constructing the EventProcessor
   [1] Validating configuration of module: class=GsfTrackProducer label='hltEgammaGsfTracksUnseeded'
Exception Message:
Illegal parameter found in configuration.  The parameter is named:
 'producer'
You could be trying to use a parameter name that is not
allowed for this plugin or it could be misspelled.
----- End Fatal Exception -------------------------------------------------
Expand to see more relval errors ...

@mmusich
Copy link
Contributor Author

mmusich commented Jan 15, 2025

So far it seems we have gotten away with this (unless NOT discovered during validation).

for instance:

desc.ifValue(edm::ParameterDescription<bool>("vetoEndcap", false, true),
true >> edm::ParameterDescription<edm::InputTag>("vetoes", {"pfTICL"}, true) or
false >> edm::EmptyGroupDescription());

or

psd0.ifValue(
edm::ParameterDescription<std::string>("algorithm", "DA_vect", true),
"DA_vect" >> edm::ParameterDescription<edm::ParameterSetDescription>("TkDAClusParameters", psd1, true) or
"DA2D_vect" >>
edm::ParameterDescription<edm::ParameterSetDescription>("TkDAClusParameters", psd2, true) or
"gap" >> edm::ParameterDescription<edm::ParameterSetDescription>("TkGapClusParameters", psd3, true));
}

@mmusich mmusich force-pushed the mm_fillDesc_migration_v4 branch from ba296ae to 12aa7df Compare January 15, 2025 14:57
@mmusich
Copy link
Contributor Author

mmusich commented Jan 16, 2025

+hlt

  • please @Martin-Grunewald let me know if you think the current implementation is OK.

@Martin-Grunewald
Copy link
Contributor

Yes I think so!

@jfernan2
Copy link
Contributor

+1

@mmusich
Copy link
Contributor Author

mmusich commented Jan 17, 2025

@cms-sw/dqm-l2 this PR just introduces a fillDescriptions for Validation/RecoTrack/plugins/TrackFromSeedProducer.cc which happens to reside in the Validation package, but it's used in the Phase-2 HLT menu as well. Can you please have a look and sign? Thank you.

@antoniovagnerini
Copy link

+1

@cmsbuild
Copy link
Contributor

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

@mandrenguyen
Copy link
Contributor

+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.

6 participants