Add new ProcessBlock feature to the Framework #30117
Conversation
|
The code-checks are being triggered in jenkins. |
|
A new Pull Request was created by @wddgit (W. David Dagenhart) for master. It involves the following packages: DataFormats/Provenance @makortel, @smuzaffar, @cmsbuild, @Dr15Jones can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
The tests are being triggered in jenkins. |
|
@wddgit could you elaborate on what is the purpose of a ProcessBlock ? |
|
Hi Andrea, Issue #28354 contains the primary discussion that I am aware of that lead to this pull request. The primary purpose is the DQM use case. The feature is general enough that it might be useful for other use cases as well. For example there are some generator products where it might be useful to use this instead of Run products. Possibly there are uses we have not thought of as well. Chris and Matti might want to add to my reply as they requested I start working on this. FYI @schneiml |
|
The most immediate motivation comes indeed from DQM (multi-run) harvesting (#28354), which this PR should be sufficient to solve. The full ProcessBlock feature will be more generic to accommodate other use cases as well. The ProcessBlock` is likely the closest thing to a "file-level storage" that we can get. |
|
-1 Tested at: 35ad468 CMSSW: CMSSW_11_2_X_2020-06-04-1100 I found follow errors while testing this PR Failed tests: UnitTests
I found errors in the following unit tests: ---> test EcalDCS_O2O_test had ERRORS |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
The unit test failure is not related to this PR. It looks unrelated from the content of the log. I can see the same error in other PRs. Probably I see the same unit test failure in the IBs as well (there is one in the same package, although I can't see to see any error at all in the IB log file, not sure why) |
|
@wddgit @makortel thanks for the pointer to #28354 . So, a "ProcessBlock" spans all the work done within a cmsRun job, or at least all the work done for a set of output files (this last part is not very clear). It has the usual begin/end transitions - which IIUC are global, not per-stream. Can it have data attached to it ? |
|
Yes. A "ProcessBlock" spans all the work done within a cmsRun job. There is an exceptional case though. There is a maxSize parameter to OutputModule's. When the output file size exceeds this limit it tells the Framework to look for an opportunity to close all output files of all output modules and open new ones. There are a lot of details related to this that I am not going to try to describe in this comment (ask if you are interested). As far as I know, no one actually uses this capability. Although, maybe someone does that I do not know about. I was just looking through our unit tests and so far I have not even found any Core unit tests of this. Maybe we should add one. Anyway, in this special case the Framework will add additional "ProcessBlock" transitions as if each set of output files was from a different process. ProcessBlock transitions are all global. There are no stream ProcessBlock transitions. Yes, data can be attached to it. One can put data products into a ProcessBlock and get them with getByToken, just like for a Run, LuminosityBlock, or Event. There are unit tests in the PR that test this ability. Data can be produced at the beginning, end, or both. This PR does not implement persistence. With only this PR you cannot save per-ProcessBlock to a an output file. But the plan is to implement persistence in a future PR. Then you will be able to save per-ProcessBlock data to an output file. (Although just with the features in this PR, you can do useful things. One alternative to our current plan would be to never implement persistence.) |
|
@cmsbuild, please test Let's refresh the tests though |
|
The tests are being triggered in jenkins.
|
|
This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
|
Comparison job queued. |
@smuzaffar What happened to the summary of the comparisons? The |
|
@mrodozov , can you please look in to this? |
|
unfortunately the file is empty. there was some failures in the compare-root-files-short-matrix job. I restarted the job (I don't remember if this is possible, to run it standalone) |
|
Comparison is ready Comparison Summary:
|
|
+1 |
This introduces the new transitions beginProcessBlock, endProcessBlock, beginProcessBlockProduce, and endProcessBlockProduce. These can be used by producers, filters, and analyzers in a way that is analogous to beginRun, endRun, beginRunProduce, and endRunProduce. These abilities become available when the types ProcessBlockCache, BeginProcessBlockProducer, or EndProcessBlockProducer are passed to the base class of the module as template parameters. These abilities are not available to legacy module types.
The order modules are executed respects product dependencies established with calls to consumes and produces. One module can put products into a ProcessBlock and another module get them.
beginProcessBlock occurs after beginJob and after beginStream, but before any run, lumi or event transitions. endProcessBlock occurs after all run, lumi, and event transitions, but before endStream and endJob. Each occurs only once per job, except in the very unusual case where all output files are closed and new output files opened. In that case only, there will be a pair of end and begin transitions in between the output file close and open.
This is only the first of a series of PRs implementing this feature. Future PRs will add persistency and the ability to access this information from Events, Runs, and Lumis through a cache. With this PR, these abilities cannot be used by OutputModules. That will be added later. (In this PR, the OutputModule part of this is partially implemented. In the code from the EventProcessor to the functions in the OutputModuleBase classes, the implementation is complete. The functions in OutputModuleBase in this PR do nothing. The PoolOutputModule only has the minimal changes required to prevent this PR from breaking it. The next PR will complete the implementation for output.)
Similarly, the implementation is not done in the InputSource. The future transition accessInputProcessBlock is also only partially implemented. (The part in the InputSource is not implemented although the part of the code from EventProcessor down to the modules is complete. Those functions will never get called with the code in this PR.)
There should be no changes in output. After this PR, these new abilities are only used by modules in Core tests. Nothing extra should be running in production jobs and nothing should be modified. You can see the new transitions in the output from the Tracer. There are new signals ActivityRegistry. The MessageLogger is modified to properly handle these new contexts. I expect any performance impact to be negligible until modules start doing nontrivial work in these transitions.
In comments to this PR requesting changes, it would be useful to identify issues that can be handled in future PRs. This PR is already large.
PR validation:
There are several new Framework unit tests and extensions to existing Framework unit tests to cover the new abilities. Output from tests in runTheMatrix and relVals should be identical.
I expect there will be conflicts between this PR and #29553 in at least EDConsumerBase. They should not be difficult to resolve though.