DQM: Allow end lumi work in DQMOneEDAnalyzer #30326
Conversation
Turns out we can set EndLuminosityBlockProducer without using WatchLuminosityBlocks? That gives an option to enable dqmEndLuminosityBlock by default. This is once again risky, since the semantics now are subtly different compared to 2018: The same callbacks exist, but events of multiple lumis can arrive alternatingly. This could lead to subtle bugs. The good thing is, we have eliminated almost all usages of the old API, so we know there are not many places where such bugs could happen. This is sort-of required to allow 'per-lumi ME with lumi data' with concurrent lumis.
|
The code-checks are being triggered in jenkins. |
The result does not compile.
|
The code-checks are being triggered in jenkins. |
|
Ok, the result of the code checks does not compile, and the version that compiles does not code-check. I feared sth. along those lines. I'll see it there is a better solution, @Dr15Jones @makortel @smuzaffar let me know if you have any ideas. |
|
-code-checks ERROR: Build errors found during clang-tidy run. |
|
@schneiml , may be inlining is issue here? |
|
@schneiml the compiler is telling you about a real problem. You made DQMOneEDAnalyzer always be an is not part of the |
|
@Dr15Jones The error now is obvious. The problem is this: But the Now, the version from 49cce0f works fine: It declares a Edit to add: The options I see are 1) simply not banning |
I removed the cache type where it was not used.
|
The code-checks are being triggered in jenkins. |
|
please test |
|
The tests are being triggered in jenkins.
|
|
+1 |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
hold Turns out this does block concurrent lumis. Nevermind, then. |
|
Pull request has been put on hold by @schneiml |
cmsbuild
added
code-checks-pending
comparison-pending
tests-pending
and removed
code-checks-approved
comparison-available
tests-approved
labels
|
The code-checks are being triggered in jenkins. |
|
Pull request #30326 was updated. @benkrikler, @kmaeshima, @schneiml, @andrius-k, @cmsbuild, @rekovic, @jfernan2, @fioriNTU can you please check and sign again. |
Correct (I was actually first surprised as well, but it actually makes sense). I clarified in the documentation that the The only way to support concurrent lumis with any of |
|
@makortel I think I'll give it another try with the caches and global transitions. That will require yet another |
|
Actually, I don't plan to try this with global transitions, it might work but would violate the "DQM behaves like products" principle. So, as long as nobody has a real need for this, I'll close it. |
PR description:
This PR adds
dqmEndLuminosityBlocktoDQMOneEDAnalyzer.Turns out we can set
EndLuminosityBlockProducerwithout usingWatchLuminosityBlocks?(Edit: no, this also blocks concurrent lumis. Back to the drawing board then.)
That gives an option to enable
dqmEndLuminosityBlockby default.This is once again risky, since the semantics now are subtly different compared to 2018:
The same callbacks exist, but events of multiple lumis can arrive alternatingly.
This could lead to subtle bugs. The good thing is, we have eliminated almost
all usages of the old API, so we know there are not many places where such bugs
could happen.
globalEndLuminosityBlockin DQM modules is banned in favor ofdqmEndLuminosityBlocknow for consistency, and all existing users changed. The semantics might be slightly different (the EDM docs hint that some lumi products might not be availale inennLuminosityBlockProduce, but inglobalEndLuminostyBlock, let's see if this survives validation.PR validation:
Compiles.
Further validation needed.