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

Fix for TTRHBuilders with PixelCPEFast #40206

Merged
merged 1 commit into from Dec 1, 2022

Conversation

AdrianoDee
Copy link
Contributor

@AdrianoDee AdrianoDee commented Nov 30, 2022

PR description:

Running the just merged #38761 on top of the latest IB (CMSSW_12_6_X_2022-11-30-1100) I noticed that the combination of #40003 and #38761 makes RECO steps fail with:

  File "/data/user/adiflori/fix/src/RecoTracker/TransientTrackingRecHit/python/TTRHBuilders_cff.py", line 10, in <module>
    from RecoLocalTracker.SiPixelRecHits.PixelCPEFastESProducer_cfi import *
ModuleNotFoundError: No module named 'RecoLocalTracker.SiPixelRecHits.PixelCPEFastESProducer_cfi'

This was not spotted as a conflict because TTRHBuilders_cff.py was not modified by #38761 and did not show up in the tests because #40003 was included in the IBs just after the last tests were launched for #38761.

This tiny PR fix the discrepancy.

PR validation:

runTheMatrix.py -w upgrade -l 11634.* -t 8 run

@AdrianoDee
Copy link
Contributor Author

type bug-fix

@AdrianoDee
Copy link
Contributor Author

urgent

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40206/33224

  • This PR adds an extra 12KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • RecoTracker/TransientTrackingRecHit (reconstruction)

@cmsbuild, @mandrenguyen, @clacaputo can you please review it and eventually sign? Thanks.
@VourMa, @felicepantaleo, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @missirol, @gpetruc, @mmusich, @mtosi, @dgulhan this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@AdrianoDee
Copy link
Contributor Author

test parameters:

@AdrianoDee
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19a271/29359/summary.html
COMMIT: 925ed52
CMSSW: CMSSW_13_0_X_2022-11-30-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/40206/29359/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 8 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3545695
  • DQMHistoTests: Total failures: 633
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3545040
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 214 log files, 51 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@@ -7,7 +7,7 @@
from RecoTracker.TkSeedingLayers.TTRHBuilderWithoutAngle4PixelPairs_cfi import *
from RecoTracker.TkSeedingLayers.TTRHBuilderWithoutAngle4PixelTriplets_cfi import *
#TransientTRH builder with template
from RecoLocalTracker.SiPixelRecHits.PixelCPEFastESProducer_cfi import *
from RecoLocalTracker.SiPixelRecHits.pixelCPEFastESProducer_cfi import pixelCPEFastESProducer as PixelCPEFastESProducer
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this "renaming" of an object that ends up in the Process in a file where the object was not defined in quite strange (without testing I'm a bit concerned it could lead to strange behavior in some circumstances). Is there something that cares of the actual label of the ESProducer? (framework doesn't unless some consumer uses edm::ESInputTag with the module label (i.e. first) argument set to the expected module label instead of empty string)

Anyway, given the urgency I suggest to proceed with this PR and address my comment later (I see similar pattern in

from RecoLocalTracker.SiPixelRecHits.pixelCPEFastESProducer_cfi import pixelCPEFastESProducer as PixelCPEFastESProducer
#from RecoLocalTracker.SiPixelRecHits.pixelCPEFastESProducerPhase1_cfi import pixelCPEFastESProducerPhase1 as PixelCPEFastESProducerPhase1
from RecoLocalTracker.SiPixelRecHits.pixelCPEFastESProducerPhase2_cfi import pixelCPEFastESProducerPhase2 as PixelCPEFastESProducerPhase2

)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed not, there's no real reason, only, yes, there are consumers with a non-empty edm::ESInputTag. I can fix it immediately after this goes in.

@smuzaffar
Copy link
Contributor

@cms-sw/reconstruction-l2 , @cms-sw/orp-l2 can we get this in? Last night IBs are broken and I would like to start an IB as soon as possible with this change

@perrotta
Copy link
Contributor

perrotta commented Dec 1, 2022

+1

  • Trying to merge in time for the 1100 IB
  • @AdrianoDee please proceed with the fixes discussed in Fix for TTRHBuilders with PixelCPEFast #40206 (review) as soon as this PR is merged
  • @cms-sw/reconstruction-l2 I'm bypassing your signature because I have a meeting starting now and I don't know if I will still be able to merge before 1100: please take your possible remarks to the fix PR that @AdrianoDee is going to prepare next

@perrotta
Copy link
Contributor

perrotta commented Dec 1, 2022

merge

@cmsbuild cmsbuild merged commit 82bcefc into cms-sw:master Dec 1, 2022
@mandrenguyen
Copy link
Contributor

+reconstruction

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 1, 2022

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 be automatically merged.

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

6 participants