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

Fix typo in L1T prescales files #37582

Merged
merged 1 commit into from Apr 18, 2022

Conversation

francescobrivio
Copy link
Contributor

@francescobrivio francescobrivio commented Apr 15, 2022

PR description:

This PR fixes a typo introduced in #37453 by adding back the .xml string in the L1T prescale files.
The typo is a blocker for the correct operations of the L1T O2Os so it should be fixed asap.

PR validation:

Code compiles.
@cms-sw/l1-l2 should comment further if other tests are needed.

Backport:

Not a backport. A backport to 12_3_X is available in #37583

FYI: @dpiparo @cms-sw/db-l2 @panoskatsoulis @elfontan @boudoul

@francescobrivio
Copy link
Contributor Author

urgent

  • blocks L1T O2O operations

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37582/29338

  • This PR adds an extra 12KB to repository

@tvami
Copy link
Contributor

tvami commented Apr 15, 2022

type bug-fix

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @francescobrivio for master.

It involves the following packages:

  • L1Trigger/L1TGlobal (l1)

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

@francescobrivio
Copy link
Contributor Author

@cmsbuild please test

@tvami
Copy link
Contributor

tvami commented Apr 15, 2022

@cms-sw/orp-l2 given that nothing tests this in CMSSW (otherwise we would have noticed this issue) is there a point in waiting for the tests to finish?

@qliphy
Copy link
Contributor

qliphy commented Apr 15, 2022

@cms-sw/orp-l2 given that nothing tests this in CMSSW (otherwise we would have noticed this issue) is there a point in waiting for the tests to finish?

It seems we still need to wait for comment from https://github.com/orgs/cms-sw/teams/l1-l2

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b53d74/23952/summary.html
COMMIT: f0284b8
CMSSW: CMSSW_12_4_X_2022-04-15-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/37582/23952/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: 3 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3593159
  • DQMHistoTests: Total failures: 8
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3593129
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 200 log files, 45 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@dpiparo
Copy link
Contributor

dpiparo commented Apr 15, 2022

Pinging explicitly @panoskatsoulis in case the comment of Qiang was not noticed.

@panoskatsoulis
Copy link
Contributor

I needed to overcome some authentication issues
and now testing

@panoskatsoulis
Copy link
Contributor

panoskatsoulis commented Apr 15, 2022

So, this commit included here fixes the issue that occurred yesterday.

However, seems there is an other issue that was hiding behind this.
Now I get this exception when running the jobs with 12_3_0.
Have you seen this error again? seems that it's related to the transaction session to the DB (Prep here)

----- Begin Fatal Exception 15-Apr-2022 22:02:16 CEST-----------------------
An exception of category 'ConditionDatabase' occurred while
   [0] Processing  Event run: 4294967295 lumi: 1 event: 1 stream: 0
   [1] Running path 'p'
   [2] Prefetching for module L1CondDBPayloadWriterExt/'L1CondDBPayloadWriterExt'
   [3] Prefetching for EventSetup module L1TriggerKeyOnlineProdExt/'L1TriggerKeyOnlineExt'
   [4] Prefetching for EventSetup module L1TUtmTriggerMenuObjectKeysOnlineProd/'L1TUtmTriggerMenuObjectKeysOnline'
   [5] Calling method for EventSetup module L1SubsystemKeysOnlineProdExt/'L1SubsystemKeysOnlineExt'
Exception Message:
The transaction is not active. from SessionImpl::openIovDb 
----- End Fatal Exception -------------------------------------------------

Here is the latest log from Prep
https://cms-conddb.cern.ch/cmsDbBrowser/logs/show_O2O_log/Prep/L1TMenu/2022-04-15%2020:02:01.742289

@dpiparo
Copy link
Contributor

dpiparo commented Apr 16, 2022

@panoskatsoulis who exactly is supposed to look at this problem?
The error singles out L1 related modules. I try to ping @francescobrivio @tvami (pinging privately helena, cannot find the github name) @ggovi

@tvami
Copy link
Contributor

tvami commented Apr 16, 2022

I think we really need @ggovi for this. My understanding is that Helena @malbouis actually already called him.

@perrotta
Copy link
Contributor

@francescobrivio , all: do I understand correctly that this fix is correct and should eventually get integrated, BUT another independent bug was discovered that also needs to be fixed?
If so, in order to speed up, wouldn't you suggest to merge this for now, while waiting for the other fix? Or should they preferably get merged together?

@panoskatsoulis
Copy link
Contributor

Hi, yes this typo fix is needed in any case. (We may merge it)

The above error is not L1 related,
I was able to overcome this by reverting the commits below locally and pushing them to the custom conddb-1 patch for testing. The error didn't come up while running both locally and on Prep.

  • 0f24a5d (Removed deprecated methods)
  • 47c0881 (Made code-checks happy)
  • c797340 (Test workflows refurbished) just for building
  • 87d7095 (Revisited transaction model)

seems that this is a transaction issue related to the CondCore/DBOutputService and the below files

CondCore/DBOutputService/interface/OnlineDBOutputService.h
CondCore/DBOutputService/interface/PoolDBOutputService.h
CondCore/DBOutputService/src/OnlineDBOutputService.cc
CondCore/DBOutputService/src/PoolDBOutputService.cc

I didn't look into this further because of the following problem that came up. As Tamas says we need @ggovi for this.

However for L1 there is an other issue as well that was not expected.
Something is missing about the migration of the new methods with the esconsumers.
(The use of ESGetToken instead of the old deprecated method)
For the L1 I am looking into this today.
Unfortunately, it was not me who done the migration and new I'm searching what is missing.

Here is the latest log from prep (after reverting the above commits to make the transaction again working)
https://cms-conddb.cern.ch/cmsDbBrowser/logs/show_O2O_log/Prep/L1TMenu/2022-04-16%2011:02:36.821735

@mmusich
Copy link
Contributor

mmusich commented Apr 17, 2022

However for L1 there is an other issue as well that was not expected.
Something is missing about the migration of the new methods with the esconsumers.
(The use of ESGetToken instead of the old deprecated method)

Was this #36383 (comment) eventually followed up by the L1 team?

@panoskatsoulis
Copy link
Contributor

not that I know, let me have a look.
thx for the reference

@francescobrivio
Copy link
Contributor Author

@francescobrivio , all: do I understand correctly that this fix is correct and should eventually get integrated, BUT another independent bug was discovered that also needs to be fixed? If so, in order to speed up, wouldn't you suggest to merge this for now, while waiting for the other fix? Or should they preferably get merged together?

@perrotta I agree that this should be merged in any case and then we can move on to fixing the other issue(s).
I guess you can bypass the @cms-sw/l1-l2 signature right?

@qliphy
Copy link
Contributor

qliphy commented Apr 18, 2022

@panoskatsoulis @francescobrivio To give an updated summary as below. Let me know if there is anything missing:

  1. Fix typo in L1T prescales files #37582 and [12_3_X] Fix typo in L1T prescales files #37583 should be merged as typo fixes.
  2. Something is missing about the migration of the new methods with the esconsumers. And there have been PRs ([12_3_X] [L1-O2O] ESGetToken migration L1CondDBPayloadWriter and O2O unit tests #37601 [L1-O2O] ESGetToken migration L1CondDBPayloadWriter and O2O unit tests #37602) made but there are crashes to be understood.
  3. a transaction issue related to the CondCore/DBOutputService, need @ggovi for this.

@tvami
Copy link
Contributor

tvami commented Apr 18, 2022

Hi @qliphy @perrotta please consider merging this w/o the L1T signature -- Panos already agreed.

@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 18, 2022

+1

@cmsbuild cmsbuild merged commit c84c56e into cms-sw:master Apr 18, 2022
@francescobrivio francescobrivio deleted the alca-fix_L1T_O2Os branch April 19, 2022 15:19
@panoskatsoulis
Copy link
Contributor

@francescobrivio we will need a backport for the to 12_3
do you want me to make it or you can do it?

@elfontan
Copy link
Contributor

elfontan commented Apr 20, 2022

Hey @panoskatsoulis,
it is here I guess :) #37583

@panoskatsoulis
Copy link
Contributor

ah yes - I didnt notice
thx

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