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

Explicit lazy parsing in SimpleFlatTableProducer #44575

Closed

Conversation

Dr15Jones
Copy link
Contributor

@Dr15Jones Dr15Jones commented Mar 28, 2024

PR description:

The T0 had a failure from the string parser which we have previously attributed to using the lazy option of the string parser. Switched code to allow lazy parsing to be set in the configuration. The default is no lazy parsing as turning on
lazy parsing can lead to problems.

PR validation:

Code compiles. Tested workflow 135.4 with the change and it ran fine.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 28, 2024

cms-bot internal usage

@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44575/39730

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • PhysicsTools/NanoAOD (xpog)

@vlimant, @hqucms can you please review it and eventually sign? Thanks.
@gpetruc, @AnnikaStein this is something you requested to watch as well.
@rappoccio, @sextonkennedy, @antoniovilela you are the release manager for this.

cms-bot commands are listed here

@cmsbuild
Copy link
Contributor

-1

Failed Tests: RelVals RelVals-INPUT
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-340323/38501/summary.html
COMMIT: fedd39e
CMSSW: CMSSW_14_1_X_2024-03-28-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/44575/38501/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals

----- Begin Fatal Exception 28-Mar-2024 22:31:36 CET-----------------------
An exception of category 'Configuration' occurred while
   [0] Constructing the EventProcessor
   [1] Constructing module: class=SimpleCandidateFlatTableProducer label='boostedTauTable'
Exception Message:
Expression parser error:no method or data member named "tauID" found for type "reco::Candidate"
It has the following methods
 charge
 Candidate
 ~Candidate
 setCharge
 threeCharge
 setThreeCharge
 p4
 polarP4
 momentum
 boostToCM
 p
 energy
 et
 et2
 mass
 massSqr
 mt
 mtSqr
 px
 py
 pz
 pt
 phi
 theta
 eta
 rapidity
 y
 setP4
 setP4
 setMass
 setPz
 vertex
 vx
 vy
 vz
 setVertex
 pdgId
 setPdgId
 status
 setStatus
 setLongLived
 longLived
 setMassConstraint
 massConstraint
 clone
 begin
 end
 begin
 end
 numberOfDaughters
 daughter
 daughter
 daughter
 daughter
 numberOfMothers
 mother
 numberOfSourceCandidatePtrs
 sourceCandidatePtr
 setSourceCandidatePtr
 vertexChi2
 vertexNdof
 vertexNormalizedChi2
 vertexCovariance
 vertexCovariance
 fillVertexCovariance
 hasMasterClone
 masterClone
 hasMasterClonePtr
 masterClonePtr
 bestTrack
 dzError
 dxyError
 isElectron
 isMuon
 isStandAloneMuon
 isGlobalMuon
 isTrackerMuon
 isCaloMuon
 isPhoton
 isConvertedPhoton
 isJet
 overlap
 operator=
 Candidate
and the following data members
 dimension
 size
 (char 0)
----- End Fatal Exception -------------------------------------------------
----- Begin Fatal Exception 28-Mar-2024 22:31:55 CET-----------------------
An exception of category 'Configuration' occurred while
   [0] Constructing the EventProcessor
   [1] Constructing module: class=SimpleCandidateFlatTableProducer label='boostedTauTable'
Exception Message:
Expression parser error:no method or data member named "tauID" found for type "reco::Candidate"
It has the following methods
 charge
 Candidate
 ~Candidate
 setCharge
 threeCharge
 setThreeCharge
 p4
 polarP4
 momentum
 boostToCM
 p
 energy
 et
 et2
 mass
 massSqr
 mt
 mtSqr
 px
 py
 pz
 pt
 phi
 theta
 eta
 rapidity
 y
 setP4
 setP4
 setMass
 setPz
 vertex
 vx
 vy
 vz
 setVertex
 pdgId
 setPdgId
 status
 setStatus
 setLongLived
 longLived
 setMassConstraint
 massConstraint
 clone
 begin
 end
 begin
 end
 numberOfDaughters
 daughter
 daughter
 daughter
 daughter
 numberOfMothers
 mother
 numberOfSourceCandidatePtrs
 sourceCandidatePtr
 setSourceCandidatePtr
 vertexChi2
 vertexNdof
 vertexNormalizedChi2
 vertexCovariance
 vertexCovariance
 fillVertexCovariance
 hasMasterClone
 masterClone
 hasMasterClonePtr
 masterClonePtr
 bestTrack
 dzError
 dxyError
 isElectron
 isMuon
 isStandAloneMuon
 isGlobalMuon
 isTrackerMuon
 isCaloMuon
 isPhoton
 isConvertedPhoton
 isJet
 overlap
 operator=
 Candidate
and the following data members
 dimension
 size
 (char 0)
----- End Fatal Exception -------------------------------------------------
----- Begin Fatal Exception 28-Mar-2024 22:34:21 CET-----------------------
An exception of category 'Configuration' occurred while
   [0] Constructing the EventProcessor
   [1] Constructing module: class=SimpleCandidateFlatTableProducer label='boostedTauTable'
Exception Message:
Expression parser error:no method or data member named "tauID" found for type "reco::Candidate"
It has the following methods
 charge
 Candidate
 ~Candidate
 setCharge
 threeCharge
 setThreeCharge
 p4
 polarP4
 momentum
 boostToCM
 p
 energy
 et
 et2
 mass
 massSqr
 mt
 mtSqr
 px
 py
 pz
 pt
 phi
 theta
 eta
 rapidity
 y
 setP4
 setP4
 setMass
 setPz
 vertex
 vx
 vy
 vz
 setVertex
 pdgId
 setPdgId
 status
 setStatus
 setLongLived
 longLived
 setMassConstraint
 massConstraint
 clone
 begin
 end
 begin
 end
 numberOfDaughters
 daughter
 daughter
 daughter
 daughter
 numberOfMothers
 mother
 numberOfSourceCandidatePtrs
 sourceCandidatePtr
 setSourceCandidatePtr
 vertexChi2
 vertexNdof
 vertexNormalizedChi2
 vertexCovariance
 vertexCovariance
 fillVertexCovariance
 hasMasterClone
 masterClone
 hasMasterClonePtr
 masterClonePtr
 bestTrack
 dzError
 dxyError
 isElectron
 isMuon
 isStandAloneMuon
 isGlobalMuon
 isTrackerMuon
 isCaloMuon
 isPhoton
 isConvertedPhoton
 isJet
 overlap
 operator=
 Candidate
and the following data members
 dimension
 size
 (char 0)
----- End Fatal Exception -------------------------------------------------
Expand to see more relval errors ...

RelVals-INPUT

  • 136.72412136.72412_RunJetHT2016B_reminiaodUL/step2_RunJetHT2016B_reminiaodUL.log
  • 140.202140.202_RunJetMET2022D_reMINI/step2_RunJetMET2022D_reMINI.log
  • 13234.013234.0_TTbar_14TeV+2021FS/step2_TTbar_14TeV+2021FS.log
Expand to see more relval errors ...

@vlimant
Copy link
Contributor

vlimant commented Mar 29, 2024

Hello, string parser are not only instantiated from where you indicated for nano, AFAICT. Should we change everywhere else ? maybe we can override the lazyness directly at the source (i.e. not leave the option of being lazy)

@Dr15Jones Dr15Jones force-pushed the noLazySimpleFlatTableProducer branch from fedd39e to 83c0f0a Compare March 29, 2024 18:10
@Dr15Jones Dr15Jones changed the title No lazy parsing in SimpleFlatTableProducer Explicit lazy parsing in SimpleFlatTableProducer Mar 29, 2024
@Dr15Jones
Copy link
Contributor Author

I took a different tack now. Now one sets lazyEval in the configuration explicitly if it is needed.

Personal note: the large number of lazyEval = True which were needed in my opinion points to the wrong class type (i.e. the base reco::Candidate) being read from the Event, instead of having modules which read the proper concrete type.

@cmsbuild
Copy link
Contributor

- added where missing
- fixed a merge error
- removed an unnecessary lazyEval
@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44575/39986

  • This PR adds an extra 104KB to repository

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

@cmsbuild
Copy link
Contributor

Pull request #44575 was updated. @vlimant, @hqucms can you please check and sign again.

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-340323/38911/summary.html
COMMIT: 2af481a
CMSSW: CMSSW_14_1_X_2024-04-17-1100/el8_amd64_gcc12
Additional Tests: NANO
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/44575/38911/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

There are some workflows for which there are errors in the baseline:
24834.78 step 2
The results for the comparisons for these workflows could be incomplete
This means most likely that the IB is having errors in the relvals.The error does NOT come from this pull request

Summary:

  • You potentially added 9 lines to the logs
  • Reco comparison results: 2983 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3319599
  • DQMHistoTests: Total failures: 6038
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3313541
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 202 log files, 165 edm output root files, 48 DQM output files
  • TriggerResults: found differences in 1 / 46 workflows

NANO Comparison Summary

Summary:

  • You potentially removed 7 lines from the logs
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 15
  • DQMHistoTests: Total histograms compared: 16792
  • DQMHistoTests: Total failures: 0
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 16792
  • DQMHistoTests: Total skipped: 0
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 14 files compared)
  • Checked 55 log files, 32 edm output root files, 15 DQM output files

Nano size comparison Summary:

Sample kb/ev ref kb/ev diff kb/ev ev/s/thd ref ev/s/thd diff rate mem/thd ref mem/thd
2500.0 2.774 2.774 0.000 ( +0.0% ) 3.49 3.55 -1.7% 2.262 2.258
2500.001 2.887 2.887 0.000 ( +0.0% ) 3.13 3.17 -1.4% 2.690 2.666
2500.002 2.834 2.834 0.000 ( +0.0% ) 3.27 3.30 -0.9% 2.659 2.648
2500.01 1.440 1.440 0.000 ( +0.0% ) 6.14 6.15 -0.0% 2.379 2.286
2500.011 1.898 1.898 0.000 ( +0.0% ) 3.33 3.33 +0.0% 2.568 2.443
2500.012 1.754 1.754 0.000 ( +0.0% ) 4.84 4.78 +1.1% 2.475 2.364
2500.1 2.345 2.345 0.000 ( +0.0% ) 4.51 4.52 -0.3% 2.074 2.071
2500.2 2.449 2.449 0.000 ( +0.0% ) 5.16 5.18 -0.3% 1.989 1.990
2500.21 1.279 1.279 0.000 ( +0.0% ) 3.49 3.50 -0.3% 2.273 2.195
2500.211 1.659 1.659 0.000 ( +0.0% ) 3.23 3.17 +1.8% 2.360 2.258
2500.3 2.219 2.219 0.000 ( +0.0% ) 9.62 9.58 +0.5% 1.968 1.967
2500.301 2.822 2.822 0.000 ( +0.0% ) 8.38 8.09 +3.5% 1.954 1.960
2500.31 7.164 7.164 0.000 ( +0.0% ) 1.43 1.39 +3.2% 1.700 1.709
2500.311 1.568 1.568 0.000 ( +0.0% ) 8.87 6.26 +41.6% 1.052 1.059
2500.312 540.457 540.457 0.000 ( +0.0% ) 0.54 0.53 +1.8% 1.596 1.604
2500.313 817.694 817.694 0.000 ( +0.0% ) 0.75 0.73 +1.7% 1.579 1.611
2500.32 1.341 1.341 0.000 ( +0.0% ) 12.90 12.90 +0.0% 2.385 2.335
2500.321 1.748 1.748 0.000 ( +0.0% ) 9.30 8.84 +5.2% 2.450 2.447
2500.322 1.240 1.240 0.000 ( +0.0% ) 9.59 9.32 +2.9% 2.228 2.228
2500.323 7.772 7.772 0.000 ( +0.0% ) 4.25 3.49 +21.7% 1.906 1.819
2500.324 1.870 1.870 0.000 ( +0.0% ) 9.42 9.28 +1.5% 2.179 2.206
2500.325 4.156 4.156 0.000 ( +0.0% ) 4.34 4.37 -0.7% 2.168 2.200
2500.326 3.309 3.309 0.000 ( +0.0% ) 1.65 1.63 +1.1% 2.204 2.116
2500.327 1.804 1.804 0.000 ( +0.0% ) 9.24 9.36 -1.2% 2.322 2.291
2500.4 2.363 2.363 0.000 ( +0.0% ) 9.19 9.39 -2.1% 1.811 1.848
2500.401 1.891 1.891 0.000 ( +0.0% ) 8.04 8.31 -3.2% 1.680 1.729
2500.402 2.937 2.937 0.000 ( +0.0% ) 8.10 8.12 -0.2% 1.777 1.776
2500.403 8.687 8.687 0.000 ( +0.0% ) 2.97 3.01 -1.3% 1.791 1.794
2500.404 5.438 5.438 0.000 ( +0.0% ) 1.18 1.21 -2.5% 1.813 1.807
2500.405 2.847 2.847 0.000 ( +0.0% ) 8.18 8.09 +1.1% 1.907 1.770
2500.5 5.194 5.194 0.000 ( +0.0% ) 16.33 15.81 +3.3% 1.521 1.503
2500.51 9.120 9.120 0.000 ( +0.0% ) 9.81 9.52 +3.1% 1.477 1.492

@hqucms
Copy link
Contributor

hqucms commented Apr 18, 2024

We still get a large number of differences in wf 11634.0 -- anything wrong in the IB?

@Dr15Jones
Copy link
Contributor Author

We still get a large number of differences in wf 11634.0 -- anything wrong in the IB?

It seems that other PRs are also showing large differences in 11634.0: see

#44624 (comment)

@makortel
Copy link
Contributor

makortel commented Apr 18, 2024

We still get a large number of differences in wf 11634.0 -- anything wrong in the IB?

It seems that other PRs are also showing large differences in 11634.0: see

#44624 (comment)

Now also in an issue #44779

@hqucms
Copy link
Contributor

hqucms commented Apr 19, 2024

@Dr15Jones I think this can be superseded by #44782 -- let me know what you think.

@Dr15Jones
Copy link
Contributor Author

@hqucms wrote:

I think this can be superseded by #44782 -- let me know what you think.

I agree that using the concrete types along with the ability to explicitly turn on lazy evaluation from the configuration is a better approach. Once #44782 is merged, we can close this PR.

@antoniovilela
Copy link
Contributor

#44782 is merged.

@hqucms
Copy link
Contributor

hqucms commented Apr 24, 2024

please close

superseded by #44782

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.

7 participants