cms-patatrack / cmssw

CMSSW fork of the Patatrack project
https://patatrack.web.cern.ch/patatrack/index.html
Apache License 2.0
2 stars 5 forks source link

fix esConsumes for PR #31721 #577

Closed mmusich closed 3 years ago

mmusich commented 3 years ago

PR description:

Addresses comments about es consumes migration at https://github.com/cms-sw/cmssw/pull/31721

PR validation:

It compiles. Tested successfully with:

cmsDriver.py step3 --conditions auto:phase1_2021_realistic -n 1000 --era Run3 --eventcontent RECOSIM,DQM --procModifiers gpu -s RAW2DIGI:RawToDigi_pixelOnly,RECO:reconstruction_pixelTrackingOnly,VALIDATION:@pixelTrackingOnlyValidation,DQM:@pixelTrackingOnlyDQM --datatier GEN-SIM-RECO,DQMIO --geometry DB:Extended --dasquery="file dataset=/RelValTTbar_14TeV/CMSSW_11_2_0_pre6-PU_112X_mcRun3_2021_realistic_v7-v1/GEN-SIM-DIGI-RAW" --no_exec

on cmg-gpu1080

if this PR is a backport please specify the original PR and why you need to backport that PR:

Not a backport, no backport needed.

mmusich commented 3 years ago

@makortel @fwyzard Should it be against master or CMSSW_11_2_X_Patatrack ?

fwyzard commented 3 years ago

Should it be against master or CMSSW_11_2_X_Patatrack ?

Currently they are equivalent, but it's probably better to use CMSSW_11_2_X_Patatrack.

(master is what gets copied to CMSSW_11_2_Patatrack_X in the official repository and used for the IBs, so I sometimes move it around to solve conflicts etc.)

ferencek commented 3 years ago

There was a request in https://github.com/cms-sw/cmssw/issues/31061#issuecomment-669269815 not to touch SiPixelClusterProducer but I guess in this PR it should be fine to touch that producer.

mmusich commented 3 years ago

@ferencek that file is not explicitly mentioned in the review, but I can add it here if that's fine with the authors...

fwyzard commented 3 years ago

@mmusich @ferencek he reason for asking not to migrate these modules

If possible, please do not migrate these modules

SiPixelClusterProducer
SiPixelRecHitConverter
PixelTrackProducer

was to avoid conflicts with the ongoing Patatrack integration. Doing (part of) the migration in the Patatrack branch would be fine, since it avoids conflicts.

However, it might be cleaner to do it in a separate PR directly for the CMSSW 11.2.x master branch ?

mmusich commented 3 years ago

However, it might be cleaner to do it in a separate PR directly for the CMSSW 11.2.x master branch ?

@fwyzard do you mean also for the ones touched directly from cms-sw#31721 (the ones I modified here) or just:

SiPixelClusterProducer
SiPixelRecHitConverter
PixelTrackProducer

If the latter I agree given also that @ferencek has a standing PR which should hit those too (https://github.com/cms-sw/cmssw/pull/32093)

ferencek commented 3 years ago

However, it might be cleaner to do it in a separate PR directly for the CMSSW 11.2.x master branch ?

@fwyzard do you mean also for the ones touched directly from cms-sw#31721 (the ones I modified here) or just:

SiPixelClusterProducer
SiPixelRecHitConverter
PixelTrackProducer

If the latter I agree given also that @ferencek has a standing PR which should hit those too (cms-sw#32093)

SiPixelRecHitConverter already has esConsumes taken care of while PixelTrackProducer is in RecoPixelVertexing/PixelTrackFitting which I had no intention of touching in https://github.com/cms-sw/cmssw/pull/32093. In https://github.com/cms-sw/cmssw/pull/32093 I did make modifications to SiPixelClusterProducer and these modifications are orthogonal to the changes made in https://github.com/cms-sw/cmssw/pull/31721 so there should be no merge conflicts and https://github.com/cms-sw/cmssw/pull/32093 and https://github.com/cms-sw/cmssw/pull/31721 should not interfere with one another.

fwyzard commented 3 years ago

586cac2c0fa458f88ce308f4144a94c2a52d6ded has some follow-up clean up.