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 fixed LHCInfo PopCon skipping fills #39238

Merged
merged 1 commit into from
Sep 1, 2022

Conversation

JanChyczynski
Copy link
Contributor

PR description:

Fixes a bug in LHCInfo PopCon. Before the fix it couldn't process two consecutive fills with stable beam with endTime of the first equal to startTime of the next one. In such case only the first one was processed and written to the database.

The fix consists of adding filterGE and filterLE methods to cond::OMSServiceQuery and using the "greater or equal" filter in the mechanism of choosing the next fill to process in the LHCInfo PopCon.

PR validation:

Verified processing of every fill after a certian timestamp with local tests running LHCInfoPopConAnalyzerStartFill.py and LHCInfoPopConAnalyzerEndFill.py and checking the logs. In the logs there are the same numbers of fills as there are fills with stable beams in OMS in the relevant time period. Particulary when running the scripts for fills after 2022-08-13 10:00:00 the fills: 8125, 8143, 8147, 8149 are not missing in the logs anymore. The tests were run both for empty tags and for tags with fill 8142 already written.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39238/31868

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @JanChyczynski (jan_chyczynski) for master.

It involves the following packages:

  • CondTools/RunInfo (db)

@malbouis, @cmsbuild, @saumyaphor4252, @ggovi, @francescobrivio, @tvami can you please review it and eventually sign? Thanks.
@mmusich 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

@tvami
Copy link
Contributor

tvami commented Aug 29, 2022

type bugfix,ctpps

@tvami
Copy link
Contributor

tvami commented Aug 29, 2022

@cmsbuild , please test

@tvami
Copy link
Contributor

tvami commented Aug 29, 2022

I still have the question about the unit test as I commented on the other PR #39188 (comment) ... @ggovi please let us know of your opinion.

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9d56af/27170/summary.html
COMMIT: 4e55f06
CMSSW: CMSSW_12_6_X_2022-08-29-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/39238/27170/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: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 51
  • DQMHistoTests: Total histograms compared: 3695708
  • DQMHistoTests: Total failures: 22
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3695663
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.004 KiB( 50 files compared)
  • DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
  • Checked 212 log files, 49 edm output root files, 51 DQM output files
  • TriggerResults: no differences found

@ggovi
Copy link
Contributor

ggovi commented Aug 30, 2022

It sounds a very good validation/debugging work. Thanks a lot Jan.

@tvami
Copy link
Contributor

tvami commented Aug 31, 2022

+db

  • it seems Giacomo agreed too

@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, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

@rappoccio
Copy link
Contributor

+1

@rappoccio rappoccio merged commit e3a73ac into cms-sw:master Sep 1, 2022
@tvami
Copy link
Contributor

tvami commented Sep 8, 2022

hi @JanChyczynski do you think this development would be worthwhile to report on the next AlCaDB meeting (Monday, Sept 12)?

@JanChyczynski
Copy link
Contributor Author

I spoke to Valentina

hi @JanChyczynski do you think this development would be worthwhile to report on the next AlCaDB meeting (Monday, Sept 12)?

I've just spoken to Valentina and I'll give a full report of my development on one of the upcoming meetings so there is no need of reporting just this PR on today's meeting

@JanChyczynski JanChyczynski changed the title fixed LHCInfo PopCon skipping fills PPS fixed LHCInfo PopCon skipping fills Sep 24, 2022
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.

5 participants