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

Remove Trajectory from event #17098

Merged
merged 73 commits into from
Jan 31, 2017
Merged

Conversation

VinInn
Copy link
Contributor

@VinInn VinInn commented Dec 23, 2016

for detail see https://its.cern.ch/jira/browse/CMSTRACK-150
(please use Jira for general discussion)

minor regression expected due to rounding

Pixel DQM is broken. will be discussed with Tracker DPG and experts
Untested/Private WFs may be broken as well.

A general customizer that will switch "save Trajectory in the Event" on for all affected producers in the process is provided

NB
please note that this PR is composed of three components:

  1. do not store Trajectories in the Event
    This saves memory and a bit of time
  2. migrate reco algorithms to not use Trajectories and use an upgraded TrackExtra instead
    can produce minor regression due to rounding
  3. migrate or protect DQM and Validation analyzers not to use Trajectories
    The most affected one is DQM for Pixel for Phase0. PhaseI will be easier to migrate still residuals cannot be computed as it stands (caveat: pulls are computed and stored in TrackExtra since ages)

PIxel DQM is now working using ad-hoc Refit Track

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Jan 31, 2017

recoTrackExtras_generalTracks__RECO size is up by ~33%. Was this the unavoidable intention?

@VinInn
Copy link
Contributor Author

VinInn commented Jan 31, 2017 via email

@slava77
Copy link
Contributor

slava77 commented Jan 31, 2017

I guess RECO data tier can handle the increase in the file size by having even shorter shelf life; it's only ~3% increase (checked 2016B and 2023D4Timing PU200)

@dmitrijus
Copy link
Contributor

+1

@slava77
Copy link
Contributor

slava77 commented Jan 31, 2017

+1

for #17098 156550b

  • code changes look reasonable given the target and the knock-on effects necessary to address (although I didn't inspect DQM in detail for those with KSprob>10%)
  • jenkins tests pass and comparisons with baseline show only very small differences in reco quantities with a few exceptions
  • local tests with a few extra workflows (similar to the short matrix) show differences similar to jenkins
  • local tests with PU200 show very small changes in reco; roughly no change in CPU; about 0.4GB/core decrease in memory, and about 30% increase in the TrackExtra size for generalTracks (manageable)

Given the current importance of HI pixel DQM in 90X, I think we can include this PR.
So, signing off.

@davidlange6 davidlange6 merged commit 556156b into cms-sw:CMSSW_9_0_X Jan 31, 2017
@davidlange6
Copy link
Contributor

Hi @VinInn @slava77 - it looks like this broke a number of workflows in the IB. Can you provide a fix in the next few hours?

eg, workflow 25400

cmsRun: /build/cmsbld/jenkins-workarea/workspace/build-any-ib/w/tmp/BUILDROOT/689db570218df72d8bea981895f47e3d/opt/cmssw/slc6_amd64_gcc630/cms/cmssw-patch/CMSSW_9_0_X_2017-01-31-2300/src/FastSimulation/Tracking/plugins/RecoTrackAccumulator.cc:105: void RecoTrackAccumulator::accumulateEvent(const T&, const edm::EventSetup&, const edm::InputTag&) [with T = PileUpEventPrincipal]: Assertion `newExtra.recHitsSize()==newExtra.trajParams().size()' failed.

@davidlange6
Copy link
Contributor

that has a traceback

Traceback (most recent call last):
File "/cvmfs/cms-ib.cern.ch/nweek-02457/slc6_amd64_gcc630/cms/cmssw-patch/CMSSW_9_0_X_2017-01-31-2300/bin/slc6_amd64_gcc630/cmsDriver.py", line 55, in
run()
File "/cvmfs/cms-ib.cern.ch/nweek-02457/slc6_amd64_gcc630/cms/cmssw-patch/CMSSW_9_0_X_2017-01-31-2300/bin/slc6_amd64_gcc630/cmsDriver.py", line 27, in run
configBuilder.prepare()
File "/cvmfs/cms-ib.cern.ch/nweek-02457/slc6_amd64_gcc630/cms/cmssw-patch/CMSSW_9_0_X_2017-01-31-2300/python/Configuration/Applications/ConfigBuilder.py", line 2052, in prepare
self.addStandardSequences()
File "/cvmfs/cms-ib.cern.ch/nweek-02457/slc6_amd64_gcc630/cms/cmssw-patch/CMSSW_9_0_X_2017-01-31-2300/python/Configuration/Applications/ConfigBuilder.py", line 746, in addStandardSequences
getattr(self,"prepare_"+stepName)(sequence = getattr(self,stepName+"DefaultSeq"))
File "/cvmfs/cms-ib.cern.ch/nweek-02457/slc6_amd64_gcc630/cms/cmssw-patch/CMSSW_9_0_X_2017-01-31-2300/python/Configuration/Applications/ConfigBuilder.py", line 1750, in prepare_VALIDATION
self.loadDefaultOrSpecifiedCFF(sequence,self.VALIDATIONDefaultCFF)
File "/cvmfs/cms-ib.cern.ch/nweek-02457/slc6_amd64_gcc630/cms/cmssw-patch/CMSSW_9_0_X_2017-01-31-2300/python/Configuration/Applications/ConfigBuilder.py", line 1185, in loadDefaultOrSpecifiedCFF
l=self.loadAndRemember(defaultCFF,unsch)
File "/cvmfs/cms-ib.cern.ch/nweek-02457/slc6_amd64_gcc630/cms/cmssw-patch/CMSSW_9_0_X_2017-01-31-2300/python/Configuration/Applications/ConfigBuilder.py", line 324, in loadAndRemember
self.process.load(includeFile)
File "/cvmfs/cms-ib.cern.ch/nweek-02457/slc6_amd64_gcc630/cms/cmssw-patch/CMSSW_9_0_X_2017-01-31-2300/python/FWCore/ParameterSet/Config.py", line 519, in load
module = import(moduleName)
File "/cvmfs/cms-ib.cern.ch/nweek-02457/slc6_amd64_gcc630/cms/cmssw-patch/CMSSW_9_0_X_2017-01-31-2300/python/Configuration/StandardSequences/ValidationHeavyIons_cff.py", line 3, in
from Validation.Configuration.ValidationHI_cff import *
File "/cvmfs/cms-ib.cern.ch/nweek-02457/slc6_amd64_gcc630/cms/cmssw-patch/CMSSW_9_0_X_2017-01-31-2300/python/Validation/Configuration/ValidationHI_cff.py", line 5, in
from Validation.RecoHI.globalValidationHeavyIons_cff import *
File "/cvmfs/cms-ib.cern.ch/nweek-02457/slc6_amd64_gcc630/cms/cmssw-patch/CMSSW_9_0_X_2017-01-31-2300/python/Validation/RecoHI/globalValidationHeavyIons_cff.py", line 11, in
StripTrackingRecHitsValid.trajectoryInput = hiTracks
File "/cvmfs/cms-ib.cern.ch/nweek-02457/slc6_amd64_gcc630/cms/cmssw-patch/CMSSW_9_0_X_2017-01-31-2300/python/FWCore/ParameterSet/Mixins.py", line 223, in setattr
self.__addParameter(name, value)
File "/cvmfs/cms-ib.cern.ch/nweek-02457/slc6_amd64_gcc630/cms/cmssw-patch/CMSSW_9_0_X_2017-01-31-2300/python/FWCore/ParameterSet/Mixins.py", line 196, in __addParameter
self.__raiseBadSetAttr(name)
File "/cvmfs/cms-ib.cern.ch/nweek-02457/slc6_amd64_gcc630/cms/cmssw-patch/CMSSW_9_0_X_2017-01-31-2300/python/FWCore/ParameterSet/Mixins.py", line 246, in __raiseBadSetAttr
raise TypeError(name+" does not already exist, so it can only be set to a CMS python configuration type")
TypeError: trajectoryInput does not already exist, so it can only be set to a CMS python configuration type

@slava77
Copy link
Contributor

slava77 commented Feb 1, 2017

@VinInn
please confirm that you can work on these today (before lunch)
Thank you.

@davidlange6
Copy link
Contributor

i can also revert, make pre3 and put this back in.

@slava77
Copy link
Contributor

slava77 commented Feb 1, 2017

I can guess that the assert in fastsim can take an effort.
It will take me longer than 1-2 hours to fix given other commitments

Martin-Grunewald added a commit to Martin-Grunewald/cmssw that referenced this pull request Feb 7, 2017
cmsbuild added a commit that referenced this pull request Feb 7, 2017
Cleanup (following #17098) for HLT ConfDB parsing
davidlange6 pushed a commit to davidlange6/cmssw that referenced this pull request Feb 21, 2017
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