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 PPS Alignment to the Prompt Calibration Loop #35631

Merged
merged 13 commits into from Oct 20, 2021

Conversation

MatiXOfficial
Copy link
Contributor

@MatiXOfficial MatiXOfficial commented Oct 12, 2021

PR description:

The main purpose of this PR is to add some new files and modifications required to run the PPS alignment procedure in the PCL. The implementation is based on #33215. Note that the modifications include updating two files in CalibPPS/TimingCalibration.

It is important for us to be able to test it on Tier0 as soon as possible, so these changes are supposed to be included in CMSSW_12_1_0. Since the deadline for CMSSW_12_1_0_pre5 is 19/10, we kindly ask for a quick review.

This PR also improves PPSAlignmentWorker and PPSAlignmentHarvester. It applies a refactor, fixes some bugs and adds some new functionality to the code in the CalibPPS/AlignmentGlobal package. The main modifications are listed below:

  • huge refactor in the worker and the harvester
  • README update to include new parameters from the PPSAlignmentConfiguration conditions format
  • test folder update
  • Replaced PPSAlignmentConfig with PPSAlignmentConfiguration in the worker and the harvester.
  • Added support for the new parameters from PPSAlignmentConfiguration.
  • Now, the harvester can save an sqlite file with the alignment results.
  • Updated the debug mode in the harvester to save more plots.
  • Fixed odd fitting in the harvester (in the yAlignment method).

Apart from that, this PR adds a RetrieveCTPPSRPAlignmentCorrectionsData module required to properly test writing an sqlite file containing a CTPPSRPAlignmentCorrectionsData object.

The PR is a continuation of the strategy presented in the AlCaDB meeting on 30 August. The previous PR were:

There are still some smaller changes foreseen, but they have been moved to the next PR, since it is vital to merge the PCL changes as soon as possible.

This PR also changes the online GTs, by including the new tags required in this PR and also get in line with the status of the 12_0_X currently used. Here are the diffs wrt the previous 12_1_X GTs:

And the diffs wrt to the 12_0_X GTs:

PR validation:

  • The code returns correct results running on data from Run 2. Check out CalibPPS/AlignmentGlobal/test for an example.
  • A new matrix test workflow has been added - 1042. It does not crash, but I've observed that the harvester (4th step) does not see any plots booked by the worker (3rd step). I don't know if that's because of a faulty configuration of the relval steps or wrong command used to run it (runTheMatrix.py -l 1042 --ibeos). Please, let me know if you know the answer to that. If not, I can remove that matrix test from this PR. By the way, I've observed the same problem while running the workflow 1041, which is very similar to 1042.
  • To be able to run the matrix test, a candidate GT has been created - 121X_dataRun3_Prompt_Candidate_2021_10_09_12_20_39. For now, it is still listed as --conditions in relval_steps.py. Please, let me know if I have to replace it with auto:run3_data_express.
  • In case they are needed, I attach the memory consumption plots acquired during running the procedure using a representative dataset from 2018.
    alignment_memory_check

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35631/25902

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • CalibPPS/AlignmentGlobal (alca)
  • CalibPPS/TimingCalibration (alca)
  • CondTools/CTPPS (db)
  • Configuration/AlCa (alca)
  • Configuration/EventContent (operations)
  • Configuration/PyReleaseValidation (pdmv, upgrade)
  • Configuration/StandardSequences (operations)

@perrotta, @malbouis, @yuanchao, @jordan-martins, @bbilin, @wajidalikhan, @tvami, @cmsbuild, @AdrianoDee, @srimanob, @kskovpen, @ggovi, @qliphy, @francescobrivio, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks.
@fabiocos, @makortel, @felicepantaleo, @fabferro, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @Martin-Grunewald, @missirol, @ebrondol, @tocheng, @lecriste, @mmusich, @slomeo, @mtosi, @dgulhan, @kpedro88 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 Oct 13, 2021

Hi @MatiXOfficial please change the autoCond file to use the candidate GT so that we can trigger some Jenkins tests. After that's done, we'll make it a versioned GT. I have a question tho, PCL is run with the the Prompt GT so why you'd like to change auto:run3_data_express? It should be the prompt version, right?

@MatiXOfficial
Copy link
Contributor Author

Thanks @tvami!
I'm a little confused now... I thought that PCL actually runs in express, and that's the reason for run3_data_express. @vavati, @jan-kaspar, can you take a look at this?
Anyway, I moved the candidate GT to run3_data_prompt in autoCond.py.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35631/25911

@cmsbuild
Copy link
Contributor

Pull request #35631 was updated. @perrotta, @malbouis, @yuanchao, @jordan-martins, @bbilin, @wajidalikhan, @tvami, @cmsbuild, @AdrianoDee, @srimanob, @kskovpen, @ggovi, @qliphy, @francescobrivio, @fabiocos, @davidlange6 can you please check and sign again.

@vavati
Copy link

vavati commented Oct 13, 2021

@tvami Concerning the GT: the candidate was built vs the prompt version as the express_queue didn't include yet all the tags of PPS. There will be updates by @malbouis . For the time being to test this PR the prompt version should be ok.

@malbouis
Copy link
Contributor

@MatiXOfficial, we (AlCaDB) suggest that you update autoCond.py with the following GT candidates and test your code with those. In case all goes well we will create versioned GTs and ask you to update autoCond.py with those.

They include all PPS tags needed for the proton reconstruction with the two PPS Alignment tags added in this PR, PPSAlignmentConfiguration_v1_express and PPSAlignmentConfiguration_reference_v1_express. Please note that the PPS tags included here will have to updated in the near future.

121X_dataRun3_Prompt_Candidate_2021_10_13_14_28_13
121X_dataRun3_HLT_Candidate_2021_10_13_14_29_46
121X_dataRun3_Express_Candidate_2021_10_13_14_32_06

For the relval steps, please use the Express GT.

@tvami
Copy link
Contributor

tvami commented Oct 19, 2021

+db

@srimanob
Copy link
Contributor

+Upgrade

For the upgrade part, a new workflow 1042 is added with a new defined sequence. PR test runs fine.

@jordan-martins
Copy link
Contributor

+1

@tvami
Copy link
Contributor

tvami commented Oct 19, 2021

@perrotta @qliphy this is essentially fully signed

@tvami
Copy link
Contributor

tvami commented Oct 19, 2021

urgent

@qliphy
Copy link
Contributor

qliphy commented Oct 20, 2021

+1

@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 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

10 participants