Primary vertex reconstruction: update configuration handling #43846

fabiocos · 2024-02-02T14:09:08Z

PR description:

This PR addresses issue #43787 following the primary vertex code reorganization made in #43592 . In particular:

the previous python master definitions are restored;
the fillDescription of PrimaryVertexProducer is updated to provide a default, but no explicit cfi python at runtime;
conditional statements are introduced for the definition of TkClusParameters and ~~logical alternatives for TkFilterParameters~~* in order to prevent the need for all possible versions always present in the PSet definition.

RecoHI/HiTracking code is neglected, as obsolete and anyway not properly working.

*: as HITrackFilterForPVFinding is just an extension of TrackFilterForPVFinding and the output PSet has always the same name TkFilterParameters, the use of logical statements for PSet like xor does not really works correctly. Using the extended version to provide the overall default seems the cleanest and safest solution.

PR validation:

Several benchmark reco configurations are expanded and compared to previous version. FWK calidation of parameters is tested at runtime for a number of benchmark cases.

cmsbuild · 2024-02-02T14:09:30Z

cms-bot internal usage

cmsbuild · 2024-02-02T14:16:43Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43846/38677

This PR adds an extra 72KB to repository

cmsbuild · 2024-02-02T14:17:06Z

A new Pull Request was created by @fabiocos (Fabio Cossutti) for master.

It involves the following packages:

Alignment/OfflineValidation (alca)
RecoVertex/Configuration (reconstruction)
RecoVertex/PrimaryVertexProducer (reconstruction)
Validation/RecoVertex (dqm)

@perrotta, @nothingface0, @mandrenguyen, @saumyaphor4252, @consuegs, @tjavaid, @rvenditti, @cmsbuild, @jfernan2, @syuvivida, @antoniovagnerini can you please review it and eventually sign? Thanks.
@rsreds, @venturia, @VourMa, @missirol, @yuanchao, @mmusich, @rovere, @VinInn, @tocheng, @JanFSchulte, @tlampen, @felicepantaleo, @adewit, @mtosi, @dgulhan, @GiacomoSguazzoni this is something you requested to watch as well.
@sextonkennedy, @rappoccio, @antoniovilela you are the release manager for this.

cms-bot commands are listed here

fabiocos · 2024-02-02T14:30:59Z

please test

cmsbuild · 2024-02-02T17:07:13Z

-1

Failed Tests: UnitTests RelVals RelVals-INPUT AddOn
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b2ad5c/37189/summary.html
COMMIT: 506faab
CMSSW: CMSSW_14_0_X_2024-02-02-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/43846/37189/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found 14 errors in the following unit tests:

---> test TestDQMOnlineClient-beampixel_dqm_sourceclient had ERRORS
---> test TestDQMOnlineClient-beam_dqm_sourceclient had ERRORS
---> test TestDQMOnlineClient-sistrip_approx_dqm_sourceclient had ERRORS
and more ...

RelVals

4.22

----- Begin Fatal Exception 02-Feb-2024 15:59:57 CET-----------------------
An exception of category 'Configuration' occurred while
   [0] Constructing the EventProcessor
   [1] Validating configuration of module: class=PrimaryVertexProducer label='offlinePrimaryVertices'
Exception Message:
Illegal parameter found in configuration.  The parameter is named:
 'TkFilterParameters'
You could be trying to use a parameter name that is not
allowed for this plugin or it could be misspelled.
----- End Fatal Exception -------------------------------------------------

5.1

----- Begin Fatal Exception 02-Feb-2024 16:00:16 CET-----------------------
An exception of category 'Configuration' occurred while
   [0] Constructing the EventProcessor
   [1] Validating configuration of module: class=PrimaryVertexProducer label='firstStepPrimaryVerticesBeforeMixing'
Exception Message:
Illegal parameter found in configuration.  The parameter is named:
 'TkFilterParameters'
You could be trying to use a parameter name that is not
allowed for this plugin or it could be misspelled.
----- End Fatal Exception -------------------------------------------------

135.4

----- Begin Fatal Exception 02-Feb-2024 16:00:16 CET-----------------------
An exception of category 'Configuration' occurred while
   [0] Constructing the EventProcessor
   [1] Validating configuration of module: class=PrimaryVertexProducer label='firstStepPrimaryVerticesBeforeMixing'
Exception Message:
Illegal parameter found in configuration.  The parameter is named:
 'TkFilterParameters'
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 ...

RelVals-INPUT

4.224.22_RunCosmics2011A/step2_RunCosmics2011A.log
4.64.6_MinimumBias2010A/step2_MinimumBias2010A.log
134.813134.813_RunCosmics2015C/step2_RunCosmics2015C.log

Expand to see more relval errors ...

AddOn Tests

fastsim

----- Begin Fatal Exception 02-Feb-2024 15:58:34 CET-----------------------
An exception of category 'Configuration' occurred while
   [0] Constructing the EventProcessor
   [1] Validating configuration of module: class=PrimaryVertexProducer label='firstStepPrimaryVerticesBeforeMixing'
Exception Message:
Illegal parameter found in configuration.  The parameter is named:
 'TkFilterParameters'
You could be trying to use a parameter name that is not
allowed for this plugin or it could be misspelled.
----- End Fatal Exception -------------------------------------------------

fastsim1

----- Begin Fatal Exception 02-Feb-2024 15:58:34 CET-----------------------
An exception of category 'Configuration' occurred while
   [0] Constructing the EventProcessor
   [1] Validating configuration of module: class=PrimaryVertexProducer label='firstStepPrimaryVerticesBeforeMixing'
Exception Message:
Illegal parameter found in configuration.  The parameter is named:
 'TkFilterParameters'
You could be trying to use a parameter name that is not
allowed for this plugin or it could be misspelled.
----- End Fatal Exception -------------------------------------------------

fastsim2

----- Begin Fatal Exception 02-Feb-2024 15:58:34 CET-----------------------
An exception of category 'Configuration' occurred while
   [0] Constructing the EventProcessor
   [1] Validating configuration of module: class=PrimaryVertexProducer label='firstStepPrimaryVerticesBeforeMixing'
Exception Message:
Illegal parameter found in configuration.  The parameter is named:
 'TkFilterParameters'
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 addon errors ...

cmsbuild · 2024-02-06T08:39:58Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43846/38731

This PR adds an extra 72KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoVertex/PrimaryVertexProducer/src/DAClusterizerInZ_vect.cc modified in PR(s): fix DAClusterizerInZ_vect for ASAN_X build #43874

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43846/38731/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43846/38731/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

jfernan2 · 2024-02-15T19:01:21Z

+1

perrotta · 2024-02-16T14:35:51Z

+alca

fabiocos · 2024-02-19T09:55:34Z

@cms-sw/dqm-l2 any comment?

tjavaid · 2024-02-20T08:39:21Z

+1

cmsbuild · 2024-02-20T08:39:42Z

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

rappoccio · 2024-02-21T17:33:35Z

+1

mmusich · 2024-02-24T18:47:53Z

@fabiocos as #43592 entered 14.0.X can you make a backport for this PR too?

Martin-Grunewald · 2024-03-03T08:10:22Z

RecoVertex/PrimaryVertexProducer/plugins/PrimaryVertexProducer.cc

@@ -451,7 +457,7 @@ void PrimaryVertexProducer::fillDescriptions(edm::ConfigurationDescriptions& des
  desc.add<bool>("useMVACut", false);
  desc.add<double>("minTrackTimeQuality", 0.8);

-  descriptions.add("primaryVertexProducer", desc);
+  descriptions.addDefault(desc);


This change messes up ConfDB parsing because no explicit cfi file is generated. I assume descriptions.addWithDefaultLabel would work?

Fixed by #44287

IIUC this change was intedended @fabiocos FYI

@Martin-Grunewald the whole point of the work behind this PR was to get rid of duplicated definitions of the PV configuration, already achieved in #43592 , without the need to carry forward in any case the complete configuration for all cases as it was in this PR, following the discussion #43592 (comment) .

From you action I understand that in ConfDB you do not only need an explicit definition of offlinePrimaryVertex, obviously granted by this PR, but also of primaryVertexProducer, am I correct? In this case keeping the construction of any other configuration from this latter, as it was in #43592 , was the appropriate thing. Your fix on top of this PR basically vanishes the goal, although I understand it is needed.

I need to verify whether the original target might be recovered in some way exploiting the new was in which the fillDescription is now written.

@fabiocos
I think the only effect of changing from addDefault to addWithDefaultLabel is writing out a cfi file primaryVertexProducer_cfi.py in ../cfipython. Since such a file does not exist elsewhere in CMSSW nor is included/imported in any other cfi/cff/cfg file, this change should be safe as it does not change any behaviour. AFAIK addDefault does still enforce fillDescriptions (and supplies missing parameters), while for ConfDb parsing we need the full list of all possible top-level parameters.

@Martin-Grunewald it is not used now because I removed it form some configurations, but sooner or later it might come back, as it already happened. At this point I would like a confirmation that the alternative definitions of TkClusParameters I have put in fillDescritption is compatible with ConfDB needs. It defines three possible alternatives depending on the algortihm value, but just one is present by default in the output _cfi.

BTW, is there in the IB integration a test where this issue can be possibly caught in time?

We (only) need the full set of top-level parameters in the cfi file for ConfDb parsing, as those are fixed by ConfDb. Any parameters within PSets etc. we can freely change/add/remove. So any variant of TkClusParameters content in the cfi file should be fine for ConfDb parsing.
Of course, the parameters used at HLT would need to be those supposed to be used at HLT, but that is a manual thing to check and to adjust (if it is not the default value).
Unfortunately for the ConfDb issue/problems related to not having all top-level parameters in the cfi file, or no cfi file at all, there is no test.

cmsbuild added this to the CMSSW_14_0_X milestone Feb 2, 2024

cmsbuild added reconstruction-pending dqm-pending alca-pending pending-signatures tests-pending orp-pending code-checks-pending tracking trk labels Feb 2, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 2, 2024

cmsbuild added tests-started and removed tests-pending labels Feb 2, 2024

fabiocos mentioned this pull request Feb 2, 2024

PrimaryVertexProducer configuration: manage alternative PSet definitions in fillDescription #43787

Closed

cmsbuild added tests-rejected and removed tests-started labels Feb 2, 2024

cmsbuild mentioned this pull request Feb 6, 2024

fix DAClusterizerInZ_vect for ASAN_X build #43874

Merged

cmsbuild added tests-pending code-checks-pending and removed tests-rejected code-checks-approved labels Feb 6, 2024

cmsbuild added code-checks-rejected and removed code-checks-pending labels Feb 6, 2024

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Feb 15, 2024

cmsbuild added alca-approved and removed alca-pending labels Feb 16, 2024

cmsbuild added dqm-approved fully-signed and removed dqm-pending pending-signatures labels Feb 20, 2024

cmsbuild added orp-approved and removed orp-pending labels Feb 21, 2024

cmsbuild merged commit 39df2dd into cms-sw:master Feb 21, 2024
11 checks passed

cmsbuild mentioned this pull request Feb 22, 2024

Update L1Scouting Data Format tests and minor code fixes #43984

Merged

This was referenced Feb 25, 2024

[Backport of 43846 to 14_0_X] Primary vertex reconstruction: update configuration handling #44079

Merged

Remove deprecated heavy-ion reconstruction #43824

Merged

fabiocos deleted the fc-fix_PV_config branch March 1, 2024 07:59

Martin-Grunewald reviewed Mar 3, 2024

View reviewed changes

mmusich added a commit to mmusich/cmssw that referenced this pull request Mar 13, 2024

fix OfflinePixel3DPrimaryVertices after PR cms-sw#43846

2696113

mmusich mentioned this pull request Mar 13, 2024

Miscellaneous fixes for dqm online clients #44384

Merged

mmusich added a commit to mmusich/cmssw that referenced this pull request Mar 13, 2024

fix OfflinePixel3DPrimaryVertices after PR cms-sw#43846

e17451c

mmusich mentioned this pull request Mar 13, 2024

[14.0.X] Miscellaneous fixes for dqm online clients #44385

Merged

mmusich added a commit to mmusich/cmssw that referenced this pull request Mar 13, 2024

fix OfflinePixel3DPrimaryVertices after PR cms-sw#43846

e45938c

mmusich added a commit to mmusich/cmssw that referenced this pull request Mar 13, 2024

fix OfflinePixel3DPrimaryVertices after PR cms-sw#43846

590a9b1

kdeleo mentioned this pull request Apr 30, 2024

MTD: fix phase-2 config for RecoVertex and RecoTrack #44872

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Primary vertex reconstruction: update configuration handling #43846

Primary vertex reconstruction: update configuration handling #43846

fabiocos commented Feb 2, 2024 •

edited

Loading

cmsbuild commented Feb 2, 2024 •

edited

Loading

cmsbuild commented Feb 2, 2024

cmsbuild commented Feb 2, 2024

fabiocos commented Feb 2, 2024

cmsbuild commented Feb 2, 2024

cmsbuild commented Feb 6, 2024

jfernan2 commented Feb 15, 2024

perrotta commented Feb 16, 2024

fabiocos commented Feb 19, 2024

tjavaid commented Feb 20, 2024

cmsbuild commented Feb 20, 2024

rappoccio commented Feb 21, 2024

mmusich commented Feb 24, 2024

Martin-Grunewald Mar 3, 2024 •

edited

Loading

Martin-Grunewald Mar 3, 2024

mmusich Mar 3, 2024

fabiocos Mar 3, 2024

Martin-Grunewald Mar 3, 2024 •

edited

Loading

fabiocos Mar 4, 2024

fabiocos Mar 4, 2024

Martin-Grunewald Mar 4, 2024 •

edited

Loading

Primary vertex reconstruction: update configuration handling #43846

Primary vertex reconstruction: update configuration handling #43846

Conversation

fabiocos commented Feb 2, 2024 • edited Loading

PR description:

PR validation:

cmsbuild commented Feb 2, 2024 • edited Loading

cmsbuild commented Feb 2, 2024

cmsbuild commented Feb 2, 2024

fabiocos commented Feb 2, 2024

cmsbuild commented Feb 2, 2024

Unit Tests

RelVals

RelVals-INPUT

AddOn Tests

cmsbuild commented Feb 6, 2024

jfernan2 commented Feb 15, 2024

perrotta commented Feb 16, 2024

fabiocos commented Feb 19, 2024

tjavaid commented Feb 20, 2024

cmsbuild commented Feb 20, 2024

rappoccio commented Feb 21, 2024

mmusich commented Feb 24, 2024

Martin-Grunewald Mar 3, 2024 • edited Loading

Choose a reason for hiding this comment

Martin-Grunewald Mar 3, 2024

Choose a reason for hiding this comment

mmusich Mar 3, 2024

Choose a reason for hiding this comment

fabiocos Mar 3, 2024

Choose a reason for hiding this comment

Martin-Grunewald Mar 3, 2024 • edited Loading

Choose a reason for hiding this comment

fabiocos Mar 4, 2024

Choose a reason for hiding this comment

fabiocos Mar 4, 2024

Choose a reason for hiding this comment

Martin-Grunewald Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

fabiocos commented Feb 2, 2024 •

edited

Loading

cmsbuild commented Feb 2, 2024 •

edited

Loading

Martin-Grunewald Mar 3, 2024 •

edited

Loading

Martin-Grunewald Mar 3, 2024 •

edited

Loading

Martin-Grunewald Mar 4, 2024 •

edited

Loading