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

Do not use a Task with SiPixelTemplateStoreESProducer #42597

Merged

Conversation

Dr15Jones
Copy link
Contributor

PR description:

The use of a Task at this time causes to much disruption. Moving ES modules to Tasks should wait till a dedicated campaign.

PR validation:

Ran unit tests for all packages changed plus all packages which had unit test failures in the IB. All tests now pass.

The use of a Task at this time causes to much disruption. Moving
ES modules to Tasks should wait till a dedicated campaign.
@Dr15Jones
Copy link
Contributor Author

please test

@mmusich
Copy link
Contributor

mmusich commented Aug 17, 2023

test parameters:

  • addpkg = Alignment/CommonAlignmentMonitor, CalibTracker/SiStripChannelGain, CalibTracker/SiStripCommon, CalibTracker/SiStripHitEfficiency, CalibTracker/SiStripHitResolution, DQM/Integration, TrackPropagation/Geant4e

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42597/36626

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @Dr15Jones (Chris Jones) for master.

It involves the following packages:

  • Alignment/CommonAlignment (alca)
  • Alignment/OfflineValidation (alca)
  • Calibration/TkAlCaRecoProducers (alca)
  • DQM/SiPixelPhase1Config (dqm)
  • FastSimulation/Configuration (fastsim)
  • RecoHI/HiTracking (reconstruction)
  • RecoTracker/FinalTrackSelectors (reconstruction)
  • RecoTracker/SpecialSeedGenerators (reconstruction)

@perrotta, @ssekmen, @tvami, @nothingface0, @civanch, @emanueleusai, @consuegs, @mdhildreth, @mandrenguyen, @pmandrik, @saumyaphor4252, @clacaputo, @syuvivida, @sbein, @tjavaid, @micsucmed, @francescobrivio, @rvenditti can you please review it and eventually sign? Thanks.
@VourMa, @felicepantaleo, @fioriNTU, @tlampen, @threus, @pakhotin, @JanFSchulte, @missirol, @mandrenguyen, @yetkinyilmaz, @sroychow, @GiacomoSguazzoni, @rovere, @VinInn, @tocheng, @jandrea, @idebruyn, @mmusich, @mtosi, @dgulhan, @arossi83, @adewit, @jazzitup, @yenjie, @kurtejung, @gpetruc, @matt-komm this is something you requested to watch as well.
@perrotta, @dpiparo, @antoniovilela, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@Dr15Jones
Copy link
Contributor Author

With these changes, the workflows 136.899, 138.1, and 138.2 now succeed.

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d1f747/34340/summary.html
COMMIT: adef830
CMSSW: CMSSW_13_3_X_2023-08-17-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/42597/34340/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 1 lines from the logs
  • Reco comparison results: 5 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3152915
  • DQMHistoTests: Total failures: 3
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3152890
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 207 log files, 159 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@mmusich
Copy link
Contributor

mmusich commented Aug 17, 2023

fwiw, with mmusich@d8dda07 all the unit tests that failed in CMSSW_13_3_X_2023-08-17-1100 do succeed:

Summary
Pass  109s ... CalibTracker/SiStripCommon/testCalibTrackerSiStripCommon
Pass  133s ... CalibTracker/SiStripHitResolution/testSiStripHitResolution
Pass  117s ... CalibTracker/SiStripHitEfficiency/testSiStripHitEfficiency
Pass  138s ... CalibTracker/SiStripChannelGain/testSSTGainPCL_fromRECO
Pass    1s ... CalibTracker/SiStripChannelGain/checkMultiRunHarvestingOutput
Pass  233s ... CalibTracker/SiStripChannelGain/testSSTGainPCL_fromCalibTree
Pass   58s ... Alignment/CommonAlignmentMonitor/testAlignmentStats
Pass   52s ... Calibration/TkAlCaRecoProducers/testAlCaHarvesting
Pass  142s ... Calibration/TkAlCaRecoProducers/testBeamSpotWorkflow
Pass   71s ... DQM/Integration/TestDQMOnlineClient-beam_dqm_sourceclient
Pass   10s ... DQM/Integration/TestDQMOnlineClient-beamhlt_dqm_sourceclient
Pass   68s ... DQM/Integration/TestDQMOnlineClient-beampixel_dqm_sourceclient
Pass   66s ... DQM/Integration/TestDQMOnlineClient-csc_dqm_sourceclient
Pass   45s ... DQM/Integration/TestDQMOnlineClient-ctpps_dqm_sourceclient
Pass   53s ... DQM/Integration/TestDQMOnlineClient-dt4ml_dqm_sourceclient
Pass   50s ... DQM/Integration/TestDQMOnlineClient-dt_dqm_sourceclient
Pass   77s ... DQM/Integration/TestDQMOnlineClient-ecal_dqm_sourceclient
Pass   12s ... DQM/Integration/TestDQMOnlineClient-ecalgpu_dqm_sourceclient
Pass   44s ... DQM/Integration/TestDQMOnlineClient-es_dqm_sourceclient
Pass   52s ... DQM/Integration/TestDQMOnlineClient-fed_dqm_sourceclient
Pass   54s ... DQM/Integration/TestDQMOnlineClient-gem_dqm_sourceclient
Pass   64s ... DQM/Integration/TestDQMOnlineClient-hcal_dqm_sourceclient
Pass   12s ... DQM/Integration/TestDQMOnlineClient-hcalgpu_dqm_sourceclient
Pass   50s ... DQM/Integration/TestDQMOnlineClient-hcalreco_dqm_sourceclient
Pass   32s ... DQM/Integration/TestDQMOnlineClient-hlt_dqm_clientPB
Pass   55s ... DQM/Integration/TestDQMOnlineClient-hlt_dqm_sourceclient
Pass   40s ... DQM/Integration/TestDQMOnlineClient-info_dqm_sourceclient
Pass   54s ... DQM/Integration/TestDQMOnlineClient-l1tstage2_dqm_sourceclient
Pass   60s ... DQM/Integration/TestDQMOnlineClient-l1tstage2emulator_dqm_sourceclient
Pass   51s ... DQM/Integration/TestDQMOnlineClient-mutracking_dqm_sourceclient
Pass    8s ... DQM/Integration/TestDQMOnlineClient-onlinebeammonitor_dqm_sourceclient
Pass  202s ... DQM/Integration/TestDQMOnlineClient-pixel_dqm_sourceclient
Pass    9s ... DQM/Integration/TestDQMOnlineClient-pixelgpu_dqm_sourceclient
Pass   47s ... DQM/Integration/TestDQMOnlineClient-pixellumi_dqm_sourceclient
Pass   40s ... DQM/Integration/TestDQMOnlineClient-rpc_dqm_sourceclient
Pass   35s ... DQM/Integration/TestDQMOnlineClient-scal_dqm_sourceclient
Pass  118s ... DQM/Integration/TestDQMOnlineClient-sistrip_dqm_sourceclient
Pass  141s ... DQM/Integration/TestDQMOnlineClient-visualization
Pass  137s ... DQM/Integration/TestDQMOnlineClient-visualization_secondInstance
Pass   46s ... TrackPropagation/Geant4e/testG4PropagatorAnalyzer
Pass   54s ... TrackPropagation/Geant4e/testG4Refitter
Pass   40s ... TrackPropagation/Geant4e/testG4SimplePropagator

@mandrenguyen
Copy link
Contributor

+1

@tvami
Copy link
Contributor

tvami commented Aug 17, 2023

I'd prefer to do mmusich@d8dda07 personally. Is there any disadvantage of doing that?

@Dr15Jones
Copy link
Contributor Author

The HLT can't use the Task mechanism so this makes everything consistent. Plus this is the only place in CMSSW beyond the framework tests that uses Tasks for EsProducers.

@Dr15Jones
Copy link
Contributor Author

This also fixed the failing RwlVal tests as well.

@emanueleusai
Copy link
Member

+1

@mmusich
Copy link
Contributor

mmusich commented Aug 18, 2023

Plus this is the only place in CMSSW beyond the framework tests that uses Tasks for EsProducers.

I am confused by the somewhat contradictory statement of this with respect to this.

The HLT can't use the Task mechanism so this makes everything consistent.

if that's the case, can't at least the unit tests (standalone configuration files) maintain the Task?

@perrotta
Copy link
Contributor

please test workflow 136.899,138.1,138.2

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d1f747/34349/summary.html
COMMIT: adef830
CMSSW: CMSSW_13_3_X_2023-08-17-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/42597/34349/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 87 lines to the logs
  • Reco comparison results: 7 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3152915
  • DQMHistoTests: Total failures: 6
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3152887
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 216 log files, 162 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@Dr15Jones
Copy link
Contributor Author

@mmusich

if that's the case, can't at least the unit tests (standalone configuration files) maintain the Task?

They certainly can. The difficulty is when I put the Task into the central configurations. The way the system works is if an ES module appears in at least one Task in the Process, then the ES module will only be added to the job if and only if that Task is on a Path (directly or indirectly) which is used in the Schedule. This change showed that there are a number of configurations in CMSSW which do not use the central configuration Sequences and instead just assemble the tracking ED modules by hand. Those configuration still loaded the central configuration to get the module definitions. It is that combination of loading the central configuration but hand building paths which leads to the problems being seen.

I only removed the use of Tasks in the unit tests so that the SiPixelTemplateStoreESProducer would be treated like every other ESProducer in the job.

@mmusich
Copy link
Contributor

mmusich commented Aug 18, 2023

I only removed the use of Tasks in the unit tests so that the SiPixelTemplateStoreESProducer would be treated like every other ESProducer in the job.

OK. I will follow-up on #42592 once this PR is merged.

@tvami
Copy link
Contributor

tvami commented Aug 18, 2023

+alca

@Dr15Jones
Copy link
Contributor Author

ping

these changes fix the problems seen in the IB.

@perrotta
Copy link
Contributor

+1

  • Needed to fix the IBs: let merge
  • Please @cms-sw/fastsim-l2 complain if you don't agree with the changes in FastSimulation/Configuration/python/Reconstruction_BefMix_cff.py

@perrotta
Copy link
Contributor

merge

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