Support concurrent LuminosityBlocks in HLTrigReport #27961
Conversation
Switched to using a LuminosityBlockCache to allow HLTrigReport to not be sensitive to the ordering of completion of a LuminosityBlock. This allows concurrent LuminosityBlocks to be processed properly. Support for concurrent Runs would require further work.
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27961/11834
|
|
please test |
|
The tests are being triggered in jenkins. |
|
A new Pull Request was created by @Dr15Jones (Chris Jones) for master. It involves the following packages: HLTrigger/HLTanalyzers @Martin-Grunewald, @cmsbuild, @fwyzard can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
@Dr15Jones running 1361.181 with "-t 4" I still see: Do you confirm that this is the expected behaviour? |
|
@fabiocos When I run a modified version of 1361.18 (to have 4 threads and 2 concurrent lumis) under CMSSW_11_0_0_pre7 without this pull request I see that message. If I run the exact same configuration file but with my changes in a CMSSW_11_0_0_pre7 build area, the message does not appear. |
| from test_hltrigreport_base_cfg import process | ||
|
|
||
| process.hlTrigReport.resetBy = "run" | ||
| process.hlTrigReport.reportBy = "job" |
There was a problem hiding this comment.
does it really make sense to reset every run and report only at the end of the job ?
There was a problem hiding this comment.
It would give you the last run of the job.
It was what the original code supported so I did the same.
There was a problem hiding this comment.
OK... IIRC the possibility was just an accident in the original code, not an intended feature.
Of course, having it doesn't harm, as long as it didn't require extra work to reimplement.
There was a problem hiding this comment.
Of course, having it doesn't harm, as long as it didn't require extra work to reimplement.
It actually does require additional logic in order to implement. But it is there and working.
| from test_hltrigreport_base_cfg import process | ||
|
|
||
| process.hlTrigReport.resetBy = "lumi" | ||
| process.hlTrigReport.reportBy = "job" |
There was a problem hiding this comment.
does it really make sense to reset every lumi and report only at the end of the job ?
There was a problem hiding this comment.
It would give you the last lumi of the job.
Again, it was what the original coe supported so I kept the same behavior.
|
@Dr15Jones my questions is due to the fact that I did a standalone test with 1361.181 |
|
@fabiocos I appreciate that you did independent testing, I don't know why you do not see the same results as I do. I also added the Tracer service to a very simple job using the HLTrigReport and it also shows that concurrent LuminosityBlocks are working. |
|
@Dr15Jones may be something wrong in my setup, I'll try oncve more, then we can see in the IB teh output of the new test wfs. @Martin-Grunewald could you please check the PR? |
|
@fabiocos I did scram project CMSSW_11_0_X_2019-09-11-2300
cd CMSSW_11_0_X_2019-09-11-2300/src
cmsenv
git cms-addpkg HLTrigger/HLTanalyzers
git checkout concurrentLumiHLTrigReport
scram b -j 12
cd ..
mkdir matrix
cd matrix/
runTheMatrix.py -l 1361.181 -t 4Then looking at the log file for step2, I get I.e. no message about HLTrigReport prohibiting concurrent LuminosityBlocks. |
|
@Dr15Jones I did something wrong in my initial test, I confirm your results. |
Oh good. You had me worried. |
|
+1 |
cmsbuild
added
fully-signed
hlt-approved
and removed
hlt-pending
pending-signatures
labels
|
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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
PR description:
Switched to using a LuminosityBlockCache to allow HLTrigReport to not be sensitive to the ordering of completion of a LuminosityBlock. This allows concurrent LuminosityBlocks to be processed properly.
Support for concurrent Runs would require further work.
Added unit tests to be sure the behavior remains the same.
This change is needed to allow workflows containing this module to be able to use concurrent LuminosityBlocks. This includes the new workflow which is testing such concurrency, 1631.181.
PR validation:
The new unit test passes.