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

PPS: rename directories with misleading names #29037

Merged
merged 6 commits into from Mar 3, 2020
Merged

Conversation

jan-kaspar
Copy link
Contributor

PR description:

There were two instances of directories with misleading names in the (CT)PPS code tree:

  • CondFormats/CTPPSReadoutObjects: contained all PPS cond-formats, not just readout,
  • RecoCTPPS/TotemRPLocal: beyond the code for Si strip detectors (from TOTEM), contained also the code for diamond and UFSD sensors.

We profit from this renaming, to replace CTPPS with PPS in the affected directory names.

In RecoPPS, we put all the local-reconstruction (within a single RP) into a single directory "Local".

This is a technical RP, no changes in the results are expected.

PR validation:

Running

runTheMatrix.py -l limited -j 10 --ibeos

gave

34 33 32 26 16 4 1 1 1 tests passed, 0 0 0 0 0 0 0 0 0 failed

The plots below compare proton reconstruction with CMSSW_11_1_0_pre3 (blue) and this PR (red dashed) - there is no difference as expected.
make_cmp

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29037/13903

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @jan-kaspar for master.

It involves the following packages:

CalibPPS/ESProducers
CondCore/CTPPSPlugins
CondCore/Utilities
CondFormats/CTPPSReadoutObjects
CondFormats/PPSObjects
CondTools/CTPPS
Configuration/EventContent
Configuration/StandardSequences
DQM/CTPPS
DQM/Integration
EventFilter/CTPPSRawToDigi
Geometry/VeryForwardGeometry
Geometry/VeryForwardGeometryBuilder
IOMC/EventVertexGenerators
PhysicsTools/PatAlgos
RecoCTPPS/Configuration
RecoCTPPS/PixelLocal
RecoCTPPS/TotemRPLocal
RecoPPS/Configuration
RecoPPS/Local
RecoPPS/ProtonReconstruction
SimPPS/PPSPixelDigiProducer
SimPPS/RPDigiProducer
Validation/CTPPS

The following packages do not have a category, yet:

CondFormats/PPSObjects
RecoPPS/Configuration
RecoPPS/Local
RecoPPS/ProtonReconstruction
Please create a PR for https://github.com/cms-sw/cms-bot/blob/master/categories_map.py to assign category

@andrius-k, @schneiml, @ianna, @fioriNTU, @tlampen, @pohsun, @santocch, @perrotta, @civanch, @makortel, @cmsbuild, @davidlange6, @Dr15Jones, @cvuosalo, @mdhildreth, @jfernan2, @tocheng, @slava77, @ggovi, @fabiocos, @kmaeshima, @christopheralanwest, @silviodonato, @franzoni can you please review it and eventually sign? Thanks.
@rappoccio, @gouskos, @felicepantaleo, @forthommel, @hatakeyamak, @emilbols, @Martin-Grunewald, @ahinzmann, @threus, @peruzzim, @seemasharmafnal, @mmarionncern, @makortel, @smoortga, @jdolen, @ferencek, @GiacomoSguazzoni, @rovere, @VinInn, @jdamgov, @nhanvtran, @gkasieczka, @tocheng, @schoef, @mmusich, @dgulhan, @clelange, @batinkov, @riga, @JyothsnaKomaragiri, @mverzett, @lecriste, @gpetruc, @mariadalfonso, @andrzejnovak this is something you requested to watch as well.
@davidlange6, @silviodonato, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@perrotta
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 26, 2020

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/4886/console Started: 2020/02/26 11:43

@cmsbuild
Copy link
Contributor

+1
Tested at: f34dc81
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e25e49/4886/summary.html
CMSSW: CMSSW_11_1_X_2020-02-25-2300
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e25e49/4886/summary.html

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-e25e49/11634.0_TTbar_14TeV+TTbar_14TeV_TuneCP5_2021_GenSimFull+DigiFull_2021+RecoFull_2021+HARVESTFull_2021+ALCAFull_2021
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-e25e49/12434.0_TTbar_14TeV+TTbar_14TeV_TuneCP5_2023_GenSimFull+DigiFull_2023+RecoFull_2023+HARVESTFull_2023+ALCAFull_2023
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-e25e49/20034.0_TTbar_14TeV+TTbar_14TeV_TuneCP5_2026D35_GenSimHLBeamSpotFull14+DigiFullTrigger_2026D35+RecoFullGlobal_2026D35+HARVESTFullGlobal_2026D35
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-e25e49/20434.0_TTbar_14TeV+TTbar_14TeV_TuneCP5_2026D41_GenSimHLBeamSpotFull14+DigiFullTrigger_2026D41+RecoFullGlobal_2026D41+HARVESTFullGlobal_2026D41
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-e25e49/21234.0_TTbar_14TeV+TTbar_14TeV_TuneCP5_2026D44_GenSimHLBeamSpotFull14+DigiFullTrigger_2026D44+RecoFullGlobal_2026D44+HARVESTFullGlobal_2026D44
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-e25e49/23234.0_TTbar_14TeV+TTbar_14TeV_TuneCP5_2026D49_GenSimHLBeamSpotFull14+DigiFullTrigger_2026D49+RecoFullGlobal_2026D49+HARVESTFullGlobal_2026D49

Comparison Summary:

  • You potentially added 1510 lines to the logs
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2679706
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2679386
  • DQMHistoTests: Total skipped: 319
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 147 log files, 16 edm output root files, 34 DQM output files

@silviodonato
Copy link
Contributor

merge

db: @ggovi
alca: @franzoni @pohsun @tlampen @tocheng @christopheralanwest
analysis: @santocch
dqm: @andrius-k @jfernan2 @kmaeshima @fioriNTU @schneiml
Please have a look. This PR simply renames several directories.

@cmsbuild cmsbuild merged commit 4eebb04 into cms-sw:master Mar 3, 2020
@jfernan2
Copy link
Contributor

jfernan2 commented Mar 3, 2020

+1

@cvuosalo
Copy link
Contributor

cvuosalo commented Mar 3, 2020

+geometry

@silviodonato
Copy link
Contributor

@jan-kaspar I reverted this PR with #29093 because it is causing many IB errors in CMSSW_11_1_X_2020-03-03-2300 (slc7 amd64 gcc820).
The errors are
class CTPPSBeamParameters found in libCondFormatsCTPPSReadoutObjects.so is already in libCondFormatsPPSObjects.so

@jan-kaspar please open a new PR, I think we need some help from @ggovi to fix the problem with CondFormatsCTPPSReadoutObjects

@perrotta
Copy link
Contributor

perrotta commented Mar 4, 2020

@jan-kaspar I reverted this PR with #29093 because it is causing many IB errors in CMSSW_11_1_X_2020-03-03-2300 (slc7 amd64 gcc820).
The errors are
class CTPPSBeamParameters found in libCondFormatsCTPPSReadoutObjects.so is already in libCondFormatsPPSObjects.so

@jan-kaspar please open a new PR, I think we need some help from @ggovi to fix the problem with CondFormatsCTPPSReadoutObjects

It is supposedly an issue with root, see above
I would rather ask the advice of the fwk gurus first (@Dr15Jones,@smuzaffar,@makortel)

@jan-kaspar
Copy link
Contributor Author

It is supposedly an issue with root, see above
I would rather ask the advice of the fwk gurus first (@Dr15Jones,@smuzaffar,@makortel)

OK, I am waiting for further advice.

@mrodozov
Copy link
Contributor

mrodozov commented Mar 4, 2020

If you see the IBs, you'll notice the ROOT 620 IB which is also using the master branch of cmssw doesn't have the unit tests breaking and it's also the only one that is a full build (by chance, due to change relevant only to this specific build, this one cms-sw/cmsdist#5619 ).
I'm suspecting there is no problem with this PR but we have to build full builds to dispose of the unit test failures.

@perrotta
Copy link
Contributor

perrotta commented Mar 4, 2020

If you see the IBs, you'll notice the ROOT 620 IB which is also using the master branch of cmssw doesn't have the unit tests breaking and it's also the only one that is a full build (by chance, due to change relevant only to this specific build, this one cms-sw/cmsdist#5619 ).
I'm suspecting there is no problem with this PR but we have to build full builds to dispose of the unit test failures.

Thank you @mrodozov !
Then, it was reverted too quickly in the IB, and delaying its inclusion will only postpone the problem to pre5.
My impression is that we should rather "revert the revert" and have a full build in the IBs to clean up the unit tests.

@mrodozov
Copy link
Contributor

mrodozov commented Mar 4, 2020

this would be easier (merge it again in cmssw) but I'd rather try to do it manually to confirm so that we don't mess it up further 😅

@jan-kaspar
Copy link
Contributor Author

Thanks everyone! Is any action needed from our side?

@perrotta
Copy link
Contributor

perrotta commented Mar 6, 2020

@mrodozov @silviodonato : was any attempt made to merge this again manually in the IB and make a full build, as suggested above?

@silviodonato do you plan to eventually "revert the revert", or should we ask @jan-kaspar to prepare an identical PR to be merged after pre4? Please advise.

@silviodonato
Copy link
Contributor

@perrotta I made #29093

@jan-kaspar
Copy link
Contributor Author

Thanks @silviodonato !

@santocch
Copy link

santocch commented Mar 8, 2020

+1

@tlampen
Copy link
Contributor

tlampen commented Mar 9, 2020

+1

@forthommel forthommel deleted the pps_rename branch May 3, 2020 10:11
@forthommel forthommel restored the pps_rename branch May 3, 2020 10:11
@jan-kaspar jan-kaspar deleted the pps_rename branch September 23, 2020 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment