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

Introduce tracking-only reco, validation, and dqm sequences #12542

Merged
merged 2 commits into from
Dec 6, 2015

Conversation

makortel
Copy link
Contributor

This PR is request for comments for introduction of tracking-only reconstruction, validation, and dqm sequences allowing their use with cmsDriver as

cmsDriver.py step3 ... -s RAW2DIGI,RECO:reconstruction_trackingOnly,VALIDATION:@trackingOnlyValidation,DQM:@trackingOnlyDQM
cmsDriver.py step4 ... -s HARVESTING:@trackingOnlyValidation+@trackingOnlyDQM

If accepted, would it be possible to add one workflow with it in the IBs? (even though we probably will exercise this ourselves frequently, a specific test would help to ensure that the workflow stays heathy)

Tested in CMSSW_8_0_X_2015-11-22-1100, no changes expected.

@rovere @VinInn

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @makortel (Matti Kortelainen) for CMSSW_8_0_X.

It involves the following packages:

Configuration/StandardSequences
DQMOffline/Configuration
SLHCUpgradeSimulations/Configuration
Validation/Configuration
Validation/RecoTrack

@civanch, @cvuosalo, @mdhildreth, @cmsbuild, @franzoni, @deguio, @slava77, @danduggan, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @istaslis, @GiacomoSguazzoni, @rovere, @VinInn, @Martin-Grunewald, @wmtford, @cerati, @threus, @dgulhan, @rociovilar this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

Following commands in first line of a comment are recognized

  • +1|approve[d]|sign[ed]: L1/L2's to approve it
  • -1|reject[ed]: L1/L2's to reject it
  • assign <category>[,<category>[,...]]: L1/L2's to request signatures from other categories
  • unassign <category>[,<category>[,...]]: L1/L2's to remove signatures from other categories
  • hold: L1/all L2's/release manager to mark it as on hold
  • unhold: L1/user who put this PR on hold
  • merge: L1/release managers to merge this request
  • [@cmsbuild,] please test: L1/L2 and selected users to start jenkins tests
  • [@cmsbuild,] please test with cms-sw/cmsdist#<PR>: L1/L2 and selected users to start jenkins tests using externals from cmsdist



### 'slim' sequences that only depend on track and tracking particle collections
tracksValidationSelectorsSlim = tracksValidationSelectors.copyAndExclude([cutsRecoTracksBtvLike,ak4JetTracksAssociatorExplicitAll,cutsRecoTracksAK4PFJets])
tracksValidationSelectorsSlim = tracksValidationSelectorsTrackingOnly.copyAndExclude([cutsRecoTracksBtvLike])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lveldere After this, the only difference between "trackingOnly" and "slim" would be that the former includes "BTV-like" track selection while the latter does not. I don't mind keeping "slim", but thought to ask anyway if keeping it is motivated?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Matti

I agree with you.

Thanks

Lukas

On Mon, Nov 23, 2015 at 2:58 PM, Matti Kortelainen <notifications@github.com

wrote:

In Validation/RecoTrack/python/TrackValidation_cff.py
#12542 (comment):

'slim' sequences that only depend on track and tracking particle collections

-tracksValidationSelectorsSlim = tracksValidationSelectors.copyAndExclude([cutsRecoTracksBtvLike,ak4JetTracksAssociatorExplicitAll,cutsRecoTracksAK4PFJets])
+tracksValidationSelectorsSlim = tracksValidationSelectorsTrackingOnly.copyAndExclude([cutsRecoTracksBtvLike])

@lveldere https://github.com/lveldere After this, the only difference
between "trackingOnly" and "slim" would be that the former includes
"BTV-like" track selection while the latter does not. I don't mind keeping
"slim", but thought to ask anyway if keeping it is motivated?


Reply to this email directly or view it on GitHub
https://github.com/cms-sw/cmssw/pull/12542/files#r45604961.

@VinInn
Copy link
Contributor

VinInn commented Nov 23, 2015

@cmsbuild , please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/9901/console

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Nov 23, 2015

from reco perspective, a separate sequence in Configuration/StandardSequences/python/Reconstruction_cff.py looks OK, rather technical.
@boudoul @hengne please follow up about a relval workflow here.

@@ -310,8 +310,8 @@ def customise_Reco(process,pileup):
# add the correct tracking back in
process.load("RecoTracker.Configuration.RecoTrackerPhase1PU"+str(nPU)+"_cff")

process.globalreco.insert(itIndex,process.trackingGlobalReco)
process.reconstruction.insert(grIndex,process.globalreco)
process.globalreco_tracking.insert(itIndex,process.trackingGlobalReco)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mark-grimes I guess these changes should not interfere with the phase1 era migration, but FYI nevertheless.

@civanch
Copy link
Contributor

civanch commented Nov 24, 2015

+1

@cvuosalo
Copy link
Contributor

+1

For #12542 91474dc

Adding tracking-only reconstruction, validation, and DQM sequences.

Reco has no objections to the new sequences. Jenkins tests show no significant differences.

@makortel
Copy link
Contributor Author

makortel commented Dec 2, 2015

Thanks @deguio. I changed the name to @trackingOnlyDQM, but did not shuffle the sequences yet. I started to think if it would be more clear to do that in a subsequent PR, because I would like to do it on both offline and HLT tracking and vertex validation sequences (and maybe including all those here would be out of scope of this PR?). I had probably misunderstood the separation between prevalidation and validation sequences (I had thought producers and analyzers), if the requirement for a module to be in validation sequence is only the trigger results, I think I can move all our modules to the prevalidation sequence (we need only the objects, not decisions on paths).

Would it be sufficient to address the sequences in a separate PR?

@deguio
Copy link
Contributor

deguio commented Dec 2, 2015

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 2, 2015

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/10067/console

@deguio
Copy link
Contributor

deguio commented Dec 2, 2015

+1
fine, thanks.
the validation sequence is run in an EndPath to make the modules which need it, to be able to access trigger results which are produced and injected into the event after Paths and before EndPaths. on the other hand modules in the EndPaths could not be freely reshuffled when running unscheduled. they will remain confined there.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 2, 2015

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 2, 2015

@civanch
Copy link
Contributor

civanch commented Dec 2, 2015

+1

@cvuosalo
Copy link
Contributor

cvuosalo commented Dec 3, 2015

+1

For #12542 103f3ae

Second approval after tiny code change.

New Jenkins tests still show no significant differences.

davidlange6 added a commit that referenced this pull request Dec 6, 2015
Introduce tracking-only reco, validation, and dqm sequences
@davidlange6 davidlange6 merged commit 2c9e763 into cms-sw:CMSSW_8_0_X Dec 6, 2015
@smuzaffar
Copy link
Contributor

@makortel , looks like this PR has caused a unit test to fail in 80X IBs https://cmssdt.cern.ch/SDT/cgi-bin/buildlogs/slc6_amd64_gcc493/CMSSW_8_0_X_2015-12-09-0000/unitTestLogs/Configuration/DataProcessing
You can reproduce it by doing

scram project  CMSSW_8_0_X_2015-12-05-2300
cd CMSSW_8_0_X_2015-12-05-2300
cmsenv
git cms-merge-topic 12542
git cms-addpkg Configuration/DataProcessing
scram b -v -j 20
cmsenv
scram b runtests_TestConfigDP

@makortel
Copy link
Contributor Author

makortel commented Dec 9, 2015

Investigating

@makortel
Copy link
Contributor Author

makortel commented Dec 9, 2015

@smuzaffar The fix is in #12730. Thanks for reporting.

@smuzaffar
Copy link
Contributor

thanks

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.

10 participants