Implement persistence for ProcessBlock products #33969
Conversation
|
A new Pull Request was created by @wddgit (W. David Dagenhart) for master. It involves the following packages: DQMServices/FwkIO @smuzaffar, @civanch, @Dr15Jones, @kmaeshima, @andrius-k, @mdhildreth, @ErnestaP, @cmsbuild, @makortel, @jfernan2, @ahmad3213, @rvenditti can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6e4464/15627/summary.html Comparison SummarySummary:
|
|
+1 |
|
+1 |
|
|
||
| namespace edm { | ||
|
|
||
| StoredProcessBlockHelper::StoredProcessBlockHelper() {} |
There was a problem hiding this comment.
| StoredProcessBlockHelper::StoredProcessBlockHelper() {} | |
| StoredProcessBlockHelper::StoredProcessBlockHelper() = default; |
would be tiny bit better ().
There was a problem hiding this comment.
Done. That is better.
| } | ||
|
|
||
| SECTION("Constructor") { | ||
| std::vector<std::string> testStrings{std::string("test1"), std::string("test2"), std::string("test3")}; |
There was a problem hiding this comment.
How about
| std::vector<std::string> testStrings{std::string("test1"), std::string("test2"), std::string("test3")}; | |
| std::vector<std::string> testStrings{"test1", "test2", "test3"}; |
?
There was a problem hiding this comment.
Done. That is better.
|
|
||
| edm::StoredProcessBlockHelper const* storedProcessBlockHelperConstPtr = &storedProcessBlockHelper; | ||
| REQUIRE(storedProcessBlockHelperConstPtr->processesWithProcessBlockProducts().empty()); | ||
| REQUIRE(storedProcessBlockHelperConstPtr->processBlockCacheIndices().empty()); |
There was a problem hiding this comment.
Is the point of these two tests wrt. the ones on lines 14-15 that the functions can be called from a const object?
There was a problem hiding this comment.
There are two different function overloads for each of those functions, one const and one non-const. I was just trying to force all the functions to get called in the test. Maybe overkill for such trivial functions...
There was a problem hiding this comment.
When I changed StoredProcessBlockAccessor to use set* functions instead of const accessors, I deleted this because it was not needed anymore.
|
|
||
| bool productsFromInputKept() const { return productsFromInputKept_; } | ||
|
|
||
| static constexpr unsigned int invalidCacheIndex() { return 0xffffffff; } |
There was a problem hiding this comment.
Is this invalidCacheIndex() the same as StoredProcessBlockHelper::invalidCacheIndex()? (I don't mean by value but by functionality, i.e. if one of them would be changed for some reason, would the other need to be changed)
There was a problem hiding this comment.
Thanks for noticing this. After looking at this more closely, I realize that we only need the functions invalidCacheIndex and invalidProcessIndex defined in ProcessBlockHelperBase. The ones in StoredProcessBlockHelper and OutputProcessBlockHelper can be deleted. I am implementing this change now and it will be in the next set of commits I push.
StoredProcessBlockHelper does not need to know these special values at all. In an earlier version, I had some non-trivial functions in StoredProcessBlockHelper. But I moved those out of that class because it is a DataFormat. And it is not a problem for OutputProcessBlockHelper.cc to use the value from ProcessBlockHelperBase. It already includes the header. Much better to remove this duplication.
|
|
||
| class ProcessBlockHelperBase { | ||
| public: | ||
| virtual ~ProcessBlockHelperBase() = default; |
There was a problem hiding this comment.
Could you move the definition to the .cc file (i.e. ProcessBlockHelperBase::~ProcessBlockHelperBase() = default;)?
In my recollection it is still beneficial for the vtable placement to have one of the virtual functions to defined in the .cc file (and in this case all others are pure virtual).
| static constexpr unsigned int invalidCacheIndex() { return 0xffffffff; } | ||
| static constexpr unsigned int invalidProcessIndex() { return 0xffffffff; } |
There was a problem hiding this comment.
Are these functionally the same as in StoredProcessBlockHelper?
There was a problem hiding this comment.
This is fixed now. See response to related comment above in OutputProcessBlockHelper.h
| unsigned int position = 0; | ||
| bool found = false; | ||
| for (auto const& processFromHelper : processesWithProcessBlockProducts_) { | ||
| if (processFromHelper == desc.processName()) { | ||
| found = true; | ||
| break; | ||
| } | ||
| ++position; | ||
| } | ||
| if (found && position >= bestPosition) { |
There was a problem hiding this comment.
I believe this could be replaced with std::find_if() along
| unsigned int position = 0; | |
| bool found = false; | |
| for (auto const& processFromHelper : processesWithProcessBlockProducts_) { | |
| if (processFromHelper == desc.processName()) { | |
| found = true; | |
| break; | |
| } | |
| ++position; | |
| } | |
| if (found && position >= bestPosition) { | |
| auto found = std::find_if(processesWithProcessBlockProducts_.begin(), | |
| processesWithProcessBlockProducts_.end(), | |
| [&desc](auto const& processFromHelper) { | |
| return processFromHelper == desc.processName()); | |
| }); | |
| if (found != processesWithProcessBlockProducts_.end()) { | |
| const unsigned int position = std::distance(processesWithProcessBlockProducts_.begin(), found); | |
| if (position >= bestPosition) { |
There was a problem hiding this comment.
Done. That is better. Thanks.
| public: | ||
| virtual ~ProcessBlockHelperBase() = default; | ||
|
|
||
| std::vector<std::string>& processesWithProcessBlockProducts() { return processesWithProcessBlockProducts_; } |
There was a problem hiding this comment.
Is the non-const accessor ("setter") used anywhere else than in the test code? There I'd find something along setProcessWithProcessBlockProducts() to be more clear (because of existing patterns elsewhere in the codebase).
There was a problem hiding this comment.
I replaced the non-const accessor with the following two functions:
void setProcessesWithProcessBlockProducts(std::vector<std::string> const& val)
void emplaceBackProcessName(std::string const& processName)
Also fixed the related test
| return processesWithProcessBlockProducts_; | ||
| } | ||
|
|
||
| std::vector<std::string>& addedProcesses() { return addedProcesses_; } |
There was a problem hiding this comment.
Same question on non-const accessor.
There was a problem hiding this comment.
I replaced the non-const accessor with the following two functions:
void setAddedProcesses(std::vector<std::string> const& val)
void emplaceBackAddedProcessName(std::string const& processName)
Also fixed the related test
| std::vector<TTree*>&& processBlockTrees, | ||
| std::vector<std::string>&& processesWithProcessBlockTrees, |
There was a problem hiding this comment.
| std::vector<TTree*>&& processBlockTrees, | |
| std::vector<std::string>&& processesWithProcessBlockTrees, | |
| std::vector<TTree*> processBlockTrees, | |
| std::vector<std::string> processesWithProcessBlockTrees, |
would be a bit more general (allows call site to not move the arguments) with at most one move-constructor call more. Or is there a reason to require the caller to move the arguments (or construct temporaries)?
There was a problem hiding this comment.
Done. That is better. I made the same change to the FileBlock update function. At the moment these are both only called from one call site in RootFile.cc, but the extra flexibility might be useful someday.
| } | ||
|
|
||
| struct TokenInfo { | ||
| public: |
There was a problem hiding this comment.
struct is public by default
| public: |
| } | ||
|
|
||
| // This gets used for stream type modules instead of registerProcessBlockCacheFiller | ||
| void copyProcessBlockCacheFiller(std::vector<edm::impl::TokenInfo>& tokenInfos, |
There was a problem hiding this comment.
Given that the arguments are moved from, would moveProcessBlockCacheFiller() be a better name?
There was a problem hiding this comment.
Done, I agree that name is better.
| std::get<ICacheType>(functors_).func_ = | ||
| std::function<std::shared_ptr<CacheType>(ProcessBlock const&, std::shared_ptr<CacheType> const&)>( | ||
| std::forward<Func>(func)); |
There was a problem hiding this comment.
Is the explicit type on the right hand side really needed, or would the following work?
| std::get<ICacheType>(functors_).func_ = | |
| std::function<std::shared_ptr<CacheType>(ProcessBlock const&, std::shared_ptr<CacheType> const&)>( | |
| std::forward<Func>(func)); | |
| std::get<ICacheType>(functors_).func_ = std::forward<Func>(func); |
There was a problem hiding this comment.
Done. It compiles and it works. I agree it is easier to read using the implicit type conversion.
| read from a EDM/ROOT file. This source should provide for delayed loading | ||
| of data, thus the quotation marks around contain. | ||
| 3) DAQSource: creats EventPrincipals which contain raw data, as | ||
| delivered by the L1 trigger and event builder. |
There was a problem hiding this comment.
Have the removed comments become (or long been) obsolete?
There was a problem hiding this comment.
Yes, I think these comments have been obsolete for more than 10 years.
| @@ -467,6 +485,16 @@ namespace edm { | |||
|
|
|||
| InputSource::RunSourceSentry::~RunSourceSentry() { source_.actReg()->postSourceRunSignal_(index_); } | |||
|
|
|||
| InputSource::ProcessBlockSourceSentry::ProcessBlockSourceSentry(InputSource const& source, | |||
| std::string const& processName) | |||
| : source_(source), processName_(processName) { | |||
There was a problem hiding this comment.
I know we follow this pattern elsewhere too, but storing const references this way is potentially dangerous, because the const reference arguments bind to temporaries. Somehow the std::string const& looks more dangerous to my eye than the InputSource const& (can't really explain why). E.g. if we would ever change processBlockPrincipal.processName() to return a copy of the string, std::string_view, or char const*, the processName_ would be an invalid reference.
Changing the pattern elsewhere is clearly out of scope of this PR, but I wonder if we should at least consider taking the arguments e.g. by pointer?
There was a problem hiding this comment.
I understand your general concern with this pattern. It requires an awareness that the object held by reference stays alive longer than the object holding the reference. Maybe it is not so bad for sentries where the intent is for them to go out of scope relatively soon. (mostly here I just copied what we do for Runs...)
I checked and the string is a data member of the ProcessBlockPrincipal. The ProcessBlockPrincipal lives until EventProcessor gets destroyed at the very end of the process. It gets reused as necessary, not destroyed. At the moment, there isn't an actual problem where the variable goes out of scope. I think it is unlikely that we change the code such that the string becomes invalid before readProcessBlock completes.
How does a pointer help? If the object goes away, then the pointer is as invalid as the reference.
We could have the sentry hold the string as a data member (neither pointer nor reference). It could just copy it. I imagine the cost of creating one short string each time we read a ProcessBlock would be negligible. I could do that if you want.
The InputSource is more difficult. We could use a shared_ptr as an argument. Currently, it is held by a unique_ptr in EventProcessor and also lives until the EventProcessor is destroyed.
My first inclination is to leave this as is. If you want me to change it, let me know and I will.
There was a problem hiding this comment.
My first inclination is to leave this as is
I would keep it as it is in this PR (for consistency, and to avoid "feature creep"), and possibly address the issue later with a separate PR.
How does a pointer help?
Function foo(string const&) binds to temporary strings (e.g. foo("bar")), whereas foo(string const*) does not (foo(&std:string("bar")) gives an error by default). Therefore, in general, taking objects whose address is stored (either as a pointer or a reference) for longer time than the function by pointer helps to catch cases where a temporary object is given.
I completely believe we don't have a real problem now. I'm more concerned of the general pattern (and towards heavier refactoring where object lifetimes could change).
| void setIndex(unsigned int value) { index_ = value; } | ||
|
|
||
| private: | ||
| unsigned int index_ = 0; |
There was a problem hiding this comment.
Is index 0 not allowed for actual uses?
There was a problem hiding this comment.
PoolSource always calls setIndex and explicitly sets the value of index_ for every event and this is saved in EventPrincipal via the function fillEventPrincipal. Zero is an allowed and legal value. For all other sources, there is still an EventToProcessBlockIndexes object in EventPrincipal. It holds the zero that comes from line 20, but it is never used for any purpose.
| // This constructor exists for ROOT I/O | ||
| StoredProcessBlockHelper(); | ||
|
|
||
| StoredProcessBlockHelper(std::vector<std::string> const& processesWithProcessBlockProducts); |
There was a problem hiding this comment.
| StoredProcessBlockHelper(std::vector<std::string> const& processesWithProcessBlockProducts); | |
| explicit StoredProcessBlockHelper(std::vector<std::string> const& processesWithProcessBlockProducts); |
| return processesWithProcessBlockProducts_; | ||
| } | ||
|
|
||
| std::vector<std::string>& processesWithProcessBlockProducts() { return processesWithProcessBlockProducts_; } |
There was a problem hiding this comment.
Why this instead of our regular set method?
There was a problem hiding this comment.
Done. I changed it to use set methods which are preferred in the CMS style guide.
| std::string const eventHistory = "EventHistory"; | ||
| std::string const eventBranchMapper = "EventBranchMapper"; | ||
|
|
||
| std::string const eventSelections = "EventSelections"; | ||
| std::string const branchListIndexes = "BranchListIndexes"; | ||
| std::string const eventToProcessBlockIndexes = "EventToProcessBlockIndexes"; |
There was a problem hiding this comment.
Thought for the future. Given we now have std::string_view which can be constexpr we should probably change all of these and the accessor methods to use std::string_view just to avoid the runtime intialization.
There was a problem hiding this comment.
I see that for C++20 std::string constructor is also constexpr capable so maybe the idea is moot.
| ProcessBlockHelperBase const* processBlockHelper() const { return processBlockHelper_; } | ||
|
|
||
| // These are intended to be used only for testing purposes | ||
| std::vector<unsigned int> const& translateFromStoredIndex() const { return translateFromStoredIndex_; } |
There was a problem hiding this comment.
Using a friend could allow you to enforce this.
There was a problem hiding this comment.
Done. I think I like it. A little strange to declare a class defined in FWCore/Integration as a friend in FWCore/Common, but I suppose there is not really any real dependence...
| std::vector<unsigned int> const& storedCacheIndices, | ||
| std::vector<unsigned int>&& nEntries); | ||
|
|
||
| void fillEntriesFromPrimaryInput(std::vector<unsigned int>&& nEntries); |
There was a problem hiding this comment.
Why use of &&? Value semantics is more in line with present C++ best practices.
There was a problem hiding this comment.
Done. I fixed three functions in this file with && arguments. One is now by value and the other two I changed to const&.
|
|
||
| class SubProcessBlockHelper : public ProcessBlockHelperBase { | ||
| public: | ||
| ProcessBlockHelperBase const* topProcessBlockHelper() const override; |
There was a problem hiding this comment.
Is final more appropriate?
There was a problem hiding this comment.
Done. I changed override to final. This is the other class that inherits from ProcessBlockHelperBase.
| std::vector<std::vector<unsigned int>> const& nEntries, | ||
| std::vector<unsigned int>& storedProcessOffset) const { | ||
| for (unsigned int iStored = 0; iStored < nInputProcesses + 1; ++iStored) { | ||
| storedProcessOffset[iStored] = 0; |
There was a problem hiding this comment.
What guarantees do you have that storedProcessOffset.size() >= nInputProcesses?
There was a problem hiding this comment.
I changed the loop to a ranged-for loop.
Maybe it is a little less error prone and more readable this way. Definitely the more modern style. To answer your question, the vector was initialized as follows and the size of it never changes:
std::vector<unsigned int> storedProcessOffset(nInputProcesses + 1, 0);
Originally, it seemed kind of intuitive to use an integer loop since this code is all about manipulating integer indexes and the index needs to be calculated even if it is not part of the loop. Usually, I am a big fan of ranged-for loops. I like them.
| std::vector<std::vector<unsigned int>> const& nEntries, | ||
| std::vector<unsigned int>& processOffset) const { | ||
| for (unsigned int iStored = 0; iStored < nInputProcesses; ++iStored) { | ||
| processOffset[iStored] = 0; |
There was a problem hiding this comment.
I changed the loop to a ranged for loop. Same comments as above.
| std::vector<std::vector<unsigned int>> const& nEntries, | ||
| std::vector<unsigned int>& storedFileInProcessOffset) const { | ||
| for (unsigned int iStored = 0; iStored < nInputProcesses; ++iStored) { | ||
| storedFileInProcessOffset[iStored] = 0; |
There was a problem hiding this comment.
I changed the loop to a ranged for loop. Same comments as above.
| @@ -131,9 +130,6 @@ namespace edm { | |||
| template <module::Abilities ABILITY, typename T, typename... VArgs> | |||
| struct CheckAbility<ABILITY, T, VArgs...> { | |||
| static constexpr bool kHasIt = (T::kAbilities == ABILITY) | CheckAbility<ABILITY, VArgs...>::kHasIt; | |||
| typedef std:: | |||
| conditional_t<(T::kAbilities == ABILITY), typename T::Type, typename CheckAbility<ABILITY, VArgs...>::Type> | |||
| Type; | |||
There was a problem hiding this comment.
Did this type alias turn out to be unnecessary?
There was a problem hiding this comment.
Yes. The typedef is not used anywhere in CMSSW so I deleted it. As I was double checking this, I noticed this TYPE typedef appears again 6 lines below, so I deleted that one also.
There was a problem hiding this comment.
Move this deletion to a separate PR. #34406
|
I was hoping the dependent tests would not run. I can believe this might be necessary, but... In the case of this test (see last commit), tests after a failure will likely all fail. It will be similar to the way compilation failures show up. We will need to look for the first failure and the rest of the failures will probably be a consequence of the first failure. I was hoping the cleanup step would not run when a failure occurred. As it is written now that cleanup would remove all the the log files until it hit a file that was never created. I could change the "rm" to "rm -rf", then it would be consistent. I am still thinking about this, if someone has a better idea... |
|
@wddgit , I can update buildrules so that dependent test still fails but with error message that one of its dependency failed. |
|
That would be better. One thought, I am not sure if this is a good idea. Could an environment variable be set so the test could know something it depends on failed? |
|
@wddgit , I was thinking of not even starting the dependent test and only write the error message with the failed dependencies name in the log of dependent test. |
|
Even better. That sounds good. |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6e4464/16663/summary.html Comparison SummarySummary:
|
1 similar comment
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6e4464/16663/summary.html Comparison SummarySummary:
|
|
Tests passed and I've replied to all comments received so far and fixed everything I know that needed fixing. I am just waiting for more comments from Chris now and after that for approvals. |
|
On these specific points (from #33969 (comment))
I'd say not for now.
I'd say not worth it.
As you wrote, this is really a wider question and really beyond this PR. Not worth of preventing this PR being merged. |
|
+1 |
|
+1 |
|
@civanch The simulation parts (in Mixing/Base) of this have not changed since you approved this about a month ago (thanks). Just letting you know that recently all the other review/changes were completed and all the other approvals are in, so if you could take a look again... @makortel I actually changed the test to redirect the output to log files. If the test completes successfully, they all get deleted in a cleanup "test". If not, I think the cleanup step does not run and they will be available to look at in the tmp subdirectory (assuming my understanding of what scram will do is correct). |
|
+1 |
|
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. @silviodonato, @dpiparo, @qliphy, @perrotta (and backports should be raised in the release meeting by the corresponding L2) |
cmsbuild
added
fully-signed
simulation-approved
and removed
pending-signatures
simulation-pending
labels
|
+1 |
PR description:
Implements persistence for ProcessBlock products.
This continues the work started in #30117, which implemented the non-persistent part of the ProcessBlock feature of the Framework.
I am currently working on updating the TWIKI documentation related to ProcessBlock. At the moment this PR is being submitted, the documentation only covers the non-persistent parts of the ProcessBlock feature. My plan is that within a few days the content of this PR will be described in more detail there. Currently, the TWIKI page is located here:
https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideProcessBlockData
PR validation:
This PR adds many unit tests that cover all the features added by the PR.
This PR should have almost no effect at all on existing workflows. There is nothing outside of the new unit tests that creates persistent ProcessBlock products. A file written without any kept ProcessBlock products will be identical to a file written by a version of CMSSW that predates this PR. If no ProcessBlock products are read from input or kept in the output the added overheads in terms of CPU and memory should be negligible. And the behavior and output of a cmsRun job should not be affected at all.
Nothing should change in the output of existing workflows until future code changes are merged that cause ProcessBlock products to be saved to output files.