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

[L1-O2O] make WriterProxyT to use ESGetToken #37681

Conversation

panoskatsoulis
Copy link
Contributor

PR description:

Required modifications in the L1 O2O for making it work under 12_3 (and afterwards)
This PR contains the code structural related commits from #37602 and drops the unit tests that will be implemented in separate PR
Also, it squashes some of the commits about code-checks

PR validation:

This is tested locally using the file
L1TriggerConfig/L1TConfigProducers/test/runL1-O2O-local.sh
and also the sqlite included in the externals
cms-data/L1TriggerConfig-L1TConfigProducers#1
Test commands

./runL1-O2O-iov.sh -k uGT,uGTrs -d uGMT,EMTF,OMTF,BMTF,CALO 500000 l1_trg_cosmics2022/v43 l1_trg_rs_cosmics2020/v23
./runL1-O2O-iov.sh -d uGT,uGTrs -k uGMT,EMTF,OMTF,BMTF,CALO 600000 l1_trg_collisions2021/v6 l1_trg_rs_collisions2021/v22

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37681/29502

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @panoskatsoulis (Panos) for master.

It involves the following packages:

  • CondTools/L1Trigger (db, l1)
  • CondTools/L1TriggerExt (db)
  • L1TriggerConfig/L1TConfigProducers (l1)

@malbouis, @epalencia, @cmsbuild, @rekovic, @ggovi, @tvami, @cecilecaillol, @francescobrivio can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @missirol, @mmusich 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

@perrotta
Copy link
Contributor

@cmsbuild please test

@tvami
Copy link
Contributor

tvami commented Apr 26, 2022

urgent

@tvami
Copy link
Contributor

tvami commented Apr 26, 2022

@panoskatsoulis given that this is going well (1 pending and 10 successful checks), can you please prepare the backport PR? Or modify #37601 so it doesnt have the unit tests? Thanks!

@panoskatsoulis
Copy link
Contributor Author

panoskatsoulis commented Apr 26, 2022

@tvami yes, I'll start preparing it now
However, I thing after the latest changes the unit tests in the #37602 will succeed now
if so, we might consider merge that one (and its ready backport)?
as it includes everything and we could eventually close this issue

@tvami
Copy link
Contributor

tvami commented Apr 26, 2022

@panoskatsoulis , yes, at this point might just do that! Let's discuss it at the ORP

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0e64f3/24221/summary.html
COMMIT: a8ba9a8
CMSSW: CMSSW_12_4_X_2022-04-26-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/37681/24221/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

@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-0e64f3/39434.75_TTbar_14TeV+2026D88_HLT75e33+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HLT75e33+HARVESTGlobal

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3695434
  • DQMHistoTests: Total failures: 8
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3695404
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 205 log files, 45 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@tvami
Copy link
Contributor

tvami commented Apr 26, 2022

+db

  • Jenkins test pass
  • this was tested locally, and the unit test testing is ongoing in CMSSW as well

@epalencia
Copy link
Contributor

+l1

@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 Apr 26, 2022

+1

@cmsbuild cmsbuild merged commit c18ac4f into cms-sw:master Apr 26, 2022
@panoskatsoulis
Copy link
Contributor Author

the unit tests in #37602 have succeed eventually and we were thinking to go with that one that was including the unit tests

now we will have conflicts I think

@perrotta
Copy link
Contributor

@qliphy @panoskatsoulis : would you agree reverting this one and merging the "complete" PR instead, as soon as the tests succeed?

@panoskatsoulis
Copy link
Contributor Author

@perrotta, @smuzaffar this is what we have been discussing with @tvami as well
also I plan to squash the 28 commits that are included in #37602 into 5 that will be clear what they do

If you all agree in this plan I will force push update the other long PR with the final commits

@perrotta
Copy link
Contributor

If you all agree in this plan I will force push update the other long PR with the final commits

Please go ahead with it now anyhow

@panoskatsoulis
Copy link
Contributor Author

@perrotta
done, you might revert this one
there will be conflicts

@perrotta
Copy link
Contributor

@panoskatsoulis I prepared a PR to revert this one in #37692
However, there are no conflicts even if this one is merged, as you can see in #37602: in fact

Then, from the point of view of github there will be no conflict, even if we can still revert this one, with the only practical consequence that there will be no a L1TriggerConfig/L1TConfigProducers/test/runL1-O2O-local.sh test any more.

Could you please remind me what was that L1TriggerConfig/L1TConfigProducers/test/runL1-O2O-local.sh meant for, and why it is not included any more in #37602?

@panoskatsoulis
Copy link
Contributor Author

@perrotta
L1TriggerConfig/L1TConfigProducers/test/runL1-O2O-local.sh was moved to
L1TriggerConfig/L1TConfigProducers/scripts/runL1-O2O-scramTests.sh

it's actually the same file, but an old version of it that was not being used for the unit tests yet

@perrotta
Copy link
Contributor

@perrotta L1TriggerConfig/L1TConfigProducers/test/runL1-O2O-local.sh was moved to L1TriggerConfig/L1TConfigProducers/scripts/runL1-O2O-scramTests.sh

it's actually the same file, but an old version of it that was not being used for the unit tests yet

Ok, then it is worh reverting this one first, and then merge #37602
Since there are no conflicts in sight, let finish the tests in the other PR first, just in case

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.

6 participants