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

Added PPSAlignmentConfiguration - a new conditions format for PPS global alignment #35174

Merged
merged 8 commits into from Sep 17, 2021

Conversation

MatiXOfficial
Copy link
Contributor

PR description:

This PR adds a new conditions format - PPSAlignmentConfiguration - which will be used in PPS global alignment running in PCL. An older version of this format - PPSAlignmentConfig - was uploaded to CMSSW in #31728 and fixed in #34844, but since it required some modifications, a new format had to be created. Although the obsolete PPSAlignmentConfig class and its record cannot be removed, this PR removes its ESProducer and serialization tests.

Main modifications:

  • some parameters were moved to PPSAlignmentHarvester config,
  • added a couple of new parameters to e.g. increase efficiency for low-statistics data,
  • deleted a few redundant parameters,
  • added a new parameter - extraParams that for now will be empty, but will be used in case any new parameters need to be added to the class. It is not planned, but still might be necessary.

This is not the last PR in PPS global alignment roadmap for Run 3. The next PR will include updates to PPSAlignmentWorker and PPSAlignmentHarvester, define a new matrix test and add a few files required by PCL. The whole strategy was presented in the AlCaDB meeting on 30 August. Note that the naming was changed from PPSAlignmentConfigRun3v1 to PPSAlignmentConfiguration after the presentation.

PR validation:

This PR adds a serialization test (testSerializationPPSAlignmentConfiguration). It also adds modules that store a PPSAlignmentConfiguration object in an SQLite file, and then retrieve the data from it. I was able to run these modules and retrieve correct objects.

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 6, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35174/25100

  • This PR adds an extra 240KB to repository

  • Found files with invalid states:

    • CondFormats/PPSObjects/src/PPSAlignmentConfigRun3v1.cc:
    • CondTools/CTPPS/test/retrieve_PPSAlignmentConfigRun3v1_cfg.py:
    • CondFormats/PPSObjects/interface/PPSAlignmentConfigRun3v1.h:
    • CondFormats/DataRecord/interface/PPSAlignmentConfigRun3v1Rcd.h:
    • CondTools/CTPPS/plugins/RetrievePPSAlignmentConfigRun3v1.cc:
    • CondTools/CTPPS/plugins/WritePPSAlignmentConfigRun3v1.cc:
    • CondFormats/PPSObjects/src/T_EventSetup_PPSAlignmentConfigRun3v1.cc:
    • CondFormats/PPSObjects/test/testSerializationPPSAlignmentConfigRun3v1.cc:
    • CondFormats/DataRecord/src/PPSAlignmentConfigRun3v1Rcd.cc:
    • CalibPPS/ESProducers/plugins/PPSAlignmentConfigRun3v1ESSource.cc:
    • CondTools/CTPPS/test/write_PPSAlignmentConfigRun3v1_cfg.py:
  • There are other open Pull requests which might conflict with changes you have proposed:

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 6, 2021

A new Pull Request was created by @MatiXOfficial (Mateusz Kocot) for master.

It involves the following packages:

  • CalibPPS/ESProducers (alca)
  • CondCore/CTPPSPlugins (db)
  • CondCore/Utilities (db)
  • CondFormats/DataRecord (db, alca)
  • CondFormats/PPSObjects (alca)
  • CondTools/CTPPS (db)

@malbouis, @yuanchao, @cmsbuild, @ggovi, @francescobrivio, @tvami can you please review it and eventually sign? Thanks.
@tocheng, @fabferro, @jan-kaspar, @mmusich, @seemasharmafnal this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@tvami
Copy link
Contributor

tvami commented Sep 6, 2021

@cmsbuild , please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 6, 2021

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-374bc0/18346/summary.html
COMMIT: b0a572a
CMSSW: CMSSW_12_1_X_2021-09-05-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35174/18346/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
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 39
  • DQMHistoTests: Total histograms compared: 3001001
  • DQMHistoTests: Total failures: 0
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3000979
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
  • Checked 165 log files, 37 edm output root files, 39 DQM output files
  • TriggerResults: no differences found

@tvami
Copy link
Contributor

tvami commented Sep 7, 2021

+alca

@@ -10,6 +10,6 @@
<use name="CondFormats/PPSObjects"/>
</bin>

<bin file="testSerializationPPSAlignmentConfig.cc">
Copy link
Contributor

@ggovi ggovi Sep 8, 2021

Choose a reason for hiding this comment

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

The PPSAlignmentConfig class is left in the code, or I did not notice the delete line? In the first case, why you remove its test ( and the above entries in the FETCH/IMPORT/PAYLOAD_2XML macros )?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, I did not remove the PPSAlignmentConfig class. Even though it's obsolete, as far as I understand, I can't remove it (and the record) since it is a conditions format (with some payloads already in the Conditions DB). Because it won't be used any more, I removed the test, the write/retrieve modules and the entries in CondCore/Utilities (772b3e4), but I can revert these changes. What do you suggest?

Copy link
Contributor

Choose a reason for hiding this comment

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

If no workflow code is consuming the old payloads, in principle we could remove it. However, it is a choice that presents some risks... I would propose to consider its deletion in a future PR. In this one, please revert the changes related to the old CondFormats (tests and FETC/IMPORT/PAYLOAD_2XML). Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done - now the files/lines of code related to the PPSAlignmentConfig class are not modified in this PR.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35174/25250

@cmsbuild
Copy link
Contributor

Pull request #35174 was updated. @malbouis, @yuanchao, @cmsbuild, @ggovi, @francescobrivio, @tvami can you please check and sign again.

@tvami
Copy link
Contributor

tvami commented Sep 14, 2021

@cmsbuild , please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-374bc0/18589/summary.html
COMMIT: 8a347c5
CMSSW: CMSSW_12_1_X_2021-09-13-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35174/18589/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

The workflows 140.53 have different files in step1_dasquery.log than the ones found in the baseline. You may want to check and retrigger the tests if necessary. You can check it in the "files" directory in the results of the comparisons

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 1299 differences found in the comparisons
  • DQMHistoTests: Total files compared: 39
  • DQMHistoTests: Total histograms compared: 3001001
  • DQMHistoTests: Total failures: 3671
  • DQMHistoTests: Total nulls: 19
  • DQMHistoTests: Total successes: 2997289
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -45.703 KiB( 38 files compared)
  • DQMHistoSizes: changed ( 140.53 ): -44.531 KiB Hcal/DigiRunHarvesting
  • DQMHistoSizes: changed ( 140.53 ): -1.172 KiB RPC/DCSInfo
  • Checked 165 log files, 37 edm output root files, 39 DQM output files
  • TriggerResults: no differences found

@ggovi
Copy link
Contributor

ggovi commented Sep 14, 2021

+1

@tvami
Copy link
Contributor

tvami commented Sep 14, 2021

+alca

@cmsbuild
Copy link
Contributor

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 now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@qliphy
Copy link
Contributor

qliphy commented Sep 17, 2021

+1

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

5 participants