Print ES module dependences from Tracer #46929
Conversation
|
cms-bot internal usage |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46929/42995
|
|
A new Pull Request was created by @wddgit for master. It involves the following packages:
@Dr15Jones, @cmsbuild, @makortel, @smuzaffar can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
+1 Size: This PR adds an extra 196KB to repository
Comparison SummarySummary:
|
| ComponentDescription const* getComponentDescription(ESResolverIndex iResolverIndex) const; | ||
| unsigned int getTransitionID(ESResolverIndex iResolverIndex) const; |
There was a problem hiding this comment.
What would you think about dropping the get prefix?
| ComponentDescription const* getComponentDescription(ESResolverIndex iResolverIndex) const; | |
| unsigned int getTransitionID(ESResolverIndex iResolverIndex) const; | |
| ComponentDescription const* componentDescription(ESResolverIndex iResolverIndex) const; | |
| unsigned int transitionID(ESResolverIndex iResolverIndex) const; |
There was a problem hiding this comment.
Done. That is better. Thanks. (I didn't push the new code yet, but hopefully later today I will do that after I finish some last checks)
| namespace edm { | ||
| class EventSetupConsumesInfo { | ||
| public: | ||
| EventSetupConsumesInfo(std::string_view const& iEventSetupRecordType, |
There was a problem hiding this comment.
string_view can be considered as a lightweight type that would be better passed by value
| EventSetupConsumesInfo(std::string_view const& iEventSetupRecordType, | |
| EventSetupConsumesInfo(std::string_view iEventSetupRecordType, |
There was a problem hiding this comment.
I changed this class into a struct and removed that function entirely. CMS coding guidelines recommend this for things like this. It seemed cleaner and less error prone than trying to get the order of the arguments correct every time I called the constructor. Although I agree with the comment in general, string_view should be passed by value not by reference.
| Produces new WhatsIt products for the EventSetup by copying | ||
| the products it read. |
There was a problem hiding this comment.
To me it would be more clear to describe already here that the input ES data products are not actually copied.
There was a problem hiding this comment.
I renamed the class ConsumeWhatsIt and there is also a new MayConsumeWhatsIt. I also edited the comment along the lines of your suggestion.
| @@ -75,6 +75,8 @@ namespace edm::eventsetup { | |||
| CallbackProductResolver(const CallbackProductResolver&) = delete; | |||
| const CallbackProductResolver& operator=(const CallbackProductResolver&) = delete; | |||
|
|
|||
| unsigned int transitionID() const final { return callback_->transitionID(); } | |||
There was a problem hiding this comment.
This is the "counter" for the setWhatProduced() calls in the ESProducer, right? I was going to suggest if we could think of a better word (I can see potential confusion with the framework transitions), but I see we already use the transitionID() term for this purpose.
There was a problem hiding this comment.
Yes. It is the counter for the setWhatProduced calls in ESProducer. Adding this function to the resolver interface does make the name more visible, but we were already using this name. The transitionID function in CallbackBase was already there and this PR didn't modify that. It also was present in several other files already.
I could change it. We could also leave that for a separate PR. But I can't think of a better name and in some places it is convenient to use the same name as we use in EDConsumerBase. But I agree it leads to some confusion. It has been that way a while.
| @@ -197,6 +197,16 @@ namespace edm { | |||
| void watchPostEndJob(PostEndJob::slot_type const& iSlot) { postEndJobSignal_.connect_front(iSlot); } | |||
| AR_WATCH_USING_METHOD_0(watchPostEndJob) | |||
|
|
|||
| typedef signalslot::Signal<void(PathsAndConsumesOfModulesBase const&, ProcessContext const&)> | |||
There was a problem hiding this comment.
What is the state of PathsAndConsumesOfModuleBase object in the existing PreBeginJob signal?
There was a problem hiding this comment.
The PathsAndConsumes object is not completely filled. Most of the information related to EventSetup isn't filled yet.
I was trying to minimize perturbations to the existing code and that led me to the current implementation. I didn't really want to change the interface of the preBeginJob signal or figure out all the consequences of moving the sequence in existing code around (afraid of the time it would take and possible errors I could inadvertently introduce and it is not obvious what to move).
I'm not completely happy with this.
The current sequence is (most of this is in EventProcessor::beginJob):
- Initialize info related to ED modules
- Use that for EDModule deletion
- The EventSetup modules go through finalizeConfiguration (updateLookup can't be called yet)
- eventSetupConfiguration signal (a couple things watch this)
- preBeginJob signal (partially filled PathsAndConsumes passed as argument, more things watch this)
- inside WorkerManager::beginJob there is a loop, for each worker updateLookup is called then beginJob
- looper calls updateLookup
- initializeForEventSetup (uses results of updateLookup)
- new signal used by the Tracer with all the EventSetup information filled
I could try to improve this. I'm nearing completion on the rest of the comments and the may consumes stuff. Mostly that seemed straightforward. I'm not sure if it is worth the effort to change this. I don't like giving access to a partially filled object in preBeginJob, but... What do you think?
There was a problem hiding this comment.
Thanks for explaining the details.
I'm not sure if it is worth the effort to change this. I don't like giving access to a partially filled object in preBeginJob, but... What do you think?
I share your dislike, more specifically I'm concerned partially filled object would lead to confusion. Also code using PathsAndConsumesOfModulesBase would potentially want to watch the new signal. Ideally, I think we should eventually give only a fully filled PathsAndConsumesOfModulesBase object.
On a quick look I noticed only a few Services that really use it, in addition to Tracer (NVProfilerService, ProcessCallGraph, FastTimerService, DependencyGraph), so I'd expect migrating those Services to use the new signal would be straightforward. But in order to avoid feature creep, it would probably be better to migrate those in subsequent PR(s), given that this PR doesn't impact their present functionality.
There was a problem hiding this comment.
I could easily add a boolean data member to the class PathsAndConsumesOfModules that indicates whether the EventSetup part has been initialized or not. If a member function using EventSetup info is called too early, it could throw an exception. Later we could remove that boolean if we migrate everything and remove the PathsAndConsumes argument from the preBeginJob transition signal function interface. Would that help?
There was a problem hiding this comment.
I think the boolean and exception would be useful to add some safety on the interface during the transition period.
There was a problem hiding this comment.
Done. I implemented the boolean and exception.
| // that is held in memory throughout the job, while the following | ||
| // function returns a newly created object each time. We do not | ||
| // expect this to be called during a normal production job where | ||
| std::vector<eventsetup::ComponentDescription const*> const& esModulesWhoseProductsAreConsumedBy( |
There was a problem hiding this comment.
Could you add a comment describing the return value in similar way to the modulesWhoseProductsAreConsumedBy() above?
There was a problem hiding this comment.
I added a comment for that function and all the other similar functions in that class. Better. Thanks.
Do you mean whatever |
|
I noticed that I neglected to implement code to handle the "may consumes" case even though I had thought about it and had a plan for how to do it. I am working on that now. It is mostly just additions to what is already here, not much significant changes in the existing PR code. I'll put it into a separate commit. This version just silently ignores those cases. I need to add a test module using "may consumes". Our current test relies on a lower level unit test. |
I am going to edit the comment at the top and remove those sentences. They do not make much sense. PoolDBESSource and its data products will show up in the Tracer output. The dump will not give any insight into the internal workings of PoolDBESSource and PoolDBESSource will not consume or depend on anything, but that is expected. |
wddgit
force-pushed
the
printoutEventSetupDependences
branch
from
66bc924 to
d5fbf7d
Compare
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46929/43163
|
|
Pull request #46929 was updated. @Dr15Jones, @cmsbuild, @makortel, @smuzaffar can you please check and sign again. |
|
I just force pushed all the updates. This should complete all of the following:
Let me know if there are any more review comments or questions. |
|
please test |
|
I also rebased the working area and squashed the commits. (Although there is a branch from before I rebased and before I squashed |
|
+1 Size: This PR adds an extra 100KB to repository Comparison SummarySummary:
|
|
This one is done and just waiting for more comments or approval. I probably won't get to responding to any new comments until Monday so no rush. Just don't want it to be forgotten because of the holidays. |
PR description:
The Tracer service will print out information about the dependences between modules when the dumpPathsAndConsumes configuration parameter is set true. Before this PR, this information only covered dependences between EDFilters, EDAnalyzers and EDProducers. This PR extends that to also include EventSetup modules. The dependency information it prints is based upon the consumes calls in the C++ code.
Issue #12126 requests this capability. This is an old request, originally from 2015.
For the most part, this PR adds and doesn't modify existing code. It shouldn't modify the behavior of the EventSetup. It shouldn't affect output of the program. Mostly it is additional code to get data out of existing data structures that are somewhat difficult to access.
When reviewing this PR, one might benefit by first looking at the following file that contains reference output of a unit test:
There are 5 main parts in the output.
Parts 3 and 5 are new. Parts 2 and 4 have EventSetup dependences added. Note that parts 4 and 5 have detail that includes both consumer and producer transitionID. The transitionID in the EventSetup is the count of the associated setWhatProduced function call in the ESProducer. This might be important because prefetching depends on this and is not done at the level of an entire ESProducer module.
PR validation:
The existing unit test for this dump of information is extended to cover the various EventSetup cases and the output of the dump is compared to a saved reference file known to be correct by manual inspection.