Allow a stream to skip a lumi if all events processed #43522
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43522/38104
|
|
A new Pull Request was created by @wddgit (W. David Dagenhart) for master. It involves the following packages:
@makortel, @cmsbuild, @Dr15Jones, @smuzaffar can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
-1 Failed Tests: UnitTests Unit TestsI found 2 errors in the following unit tests: ---> test TestFWCoreFrameworkTransitions had ERRORS ---> test TestFWCoreFrameworkGlobalStreamOne had ERRORS Comparison SummarySummary:
|
|
The two unit test failures are the ones I expected. I need to update the unit tests. I'll continue with this next week and add some new tests if I'm heading in the right direction on this... |
cmsbuild
added
tests-pending
code-checks-pending
and removed
tests-rejected
code-checks-approved
labels
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43522/38165
|
|
Pull request #43522 was updated. @makortel, @Dr15Jones, @smuzaffar, @cmsbuild can you please check and sign again. |
|
please test Just pushed a commit to fix unit test TestFWCoreFrameworkTransitions. Now it passes with this PR. Sometimes that test has a stream that skips a lumi. It does not happen always (seems like very roughly half the time).The test criteria were modified to allow the test to pass if a stream lumi begin or end transition is skipped. It also checks that the expected number of transitions equals the seen number of transitions plus the number skipped, that skipped stream lumi transitions come in matched pairs (a begin and end for the same stream and lumi), and that there is at least one stream begin lumi for every global begin lumi transition. (There's still one more failing. Working on that next.) |
|
I just force pushed. The code should be exactly the same. I squashed and rebased. The history will be a little cleaner. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43522/39460
|
|
Pull request #43522 was updated. @cmsbuild, @makortel, @Dr15Jones, @smuzaffar can you please check and sign again. |
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ef83f9/38109/summary.html Comparison SummarySummary:
|
|
+core |
cmsbuild
added
core-approved
fully-signed
and removed
core-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. @antoniovilela, @rappoccio, @sextonkennedy (and backports should be raised in the release meeting by the corresponding L2) |
|
I'd suggest to not merge this PR on Friday because of slight risk of breaking something. Merging today or Sunday/Monday would be preferred. |
|
+1 |
PR description:
This PR will make the Framework try to direct a stream to skip a lumi if all events have been processed by other streams and the stream has not started the luminosity block yet.
Note that a stream might have started a lumi but not gotten any events yet and then it is too late to skip the lumi. Even after this PR is merged, it will still be possible for a stream to just run begin stream lumi and end stream lumi with no events.
This new behavior will be a performance improvement when one event takes a very long time to process. Before this PR, the stream processing that event cannot process any subsequent lumis and those lumis cannot complete until after the event completes. If the limit on the number of lumis is reached, all other work might become stuck until the slow event completes.
PR validation:
All pre-existing unit Core unit tests pass.