What would you think about dropping the get prefix?

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)

string_view can be considered as a lightweight type that would be better passed by value

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.

To me it would be more clear to describe already here that the input ES data products are not actually copied.

I renamed the class ConsumeWhatsIt and there is also a new MayConsumeWhatsIt. I also edited the comment along the lines of your suggestion.

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.

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.

Since this is nominally the order of the setWhatProduced method calls, how about we change it to producedID? If we keep using the transitionID name for this one it is very confusing with the use of transition already in the ED system.

I am OK with changing this to producedID. I can't think of anything better. @makortel Are you in agreement also? Just checking before I start spending time implementing that change.

Just to repeat what I said earlier, this name was in use before this PR. I'll have to search for all existing uses and change them. But I agree that the name transitionID is confusing in this context.

Actually, what about produceMethodID instead.

produceMethodID does seem better to me.

I'm good with produceMethodID().

What is the state of PathsAndConsumesOfModuleBase object in the existing PreBeginJob signal?

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):

1. Initialize info related to ED modules
2. Use that for EDModule deletion
3. The EventSetup modules go through finalizeConfiguration (updateLookup can't be called yet)
4. eventSetupConfiguration signal (a couple things watch this)
5. preBeginJob signal (partially filled PathsAndConsumes passed as argument, more things watch this)
6. inside WorkerManager::beginJob there is a loop, for each worker updateLookup is called then beginJob
7. looper calls updateLookup
8. initializeForEventSetup (uses results of updateLookup)
9. 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?

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.

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?

I think the boolean and exception would be useful to add some safety on the interface during the transition period.

Done. I implemented the boolean and exception.

Could you add a comment describing the return value in similar way to the modulesWhoseProductsAreConsumedBy() above?

I added a comment for that function and all the other similar functions in that class. Better. Thanks.

Is this TODO idea addressed in this PR, or you think it is not worth it anymore? (I see you added the comment in 2019)

Given the class being struct, this line is redundant

If the public interface of this class are the public data members, I'd be inclined to drop the trailing underscore (like in ProductLabels)

But thinking out loud a bit

• Event system ConsumesInfo has a member-function interface
• With public data members it is possible to accidentally write to any of the data members
  • Not sure how big risk this point really is
• Adding trivial getters/setters like in ConsumesInfo doesn't seem good either (also C++ Code Guidelines C.131), and adding all of these via constructor would just beg for a mistake

So probably a struct would indeed be the least bad option.

Should we consider making the interface patterns of EventSetupConsumesInfo and ConsumesInfo similar (some time in the future, if ever)?

I have a little bit uneasy feeling of adding a dependence on ESProducer type here (although it is only forward-declared).

With EDModules I suppose we deal with these dependencies by passing in the module ID (that is just and integer) in this abstract interface, whereas the implementation (in FWCore/Framework) gets access to the actual worker/module objects via Schedule. I wonder if something similar would be feasible for ESProducers? (even for possible subsequent work if would be too much for this PR)

Given how often static_cast<unsigned int>(Transition::NumberOfEventSetupTransitions) appears in the code, maybe a constexpr should be defined maybe in the file holding Transition?

constexpr unsigned int kNumberOfEventSetupTransitions = static_cast<unsigned int>(Transition::NumberOfEventSetupTransitions);

Given the full type is used above in the function signature, how about moving the using to before the function and then make use of the using there as well?

Is this only used during the filling of the information for the Service callback? If so, I'm not a fan of it hanging around long term.

I do not like everything suddenly becoming dependent upon EventSetupProvider. What appears to be needed is just a way to lookup for each EventSetup Record index what ComponentDescriptions are associated with a given index. I'd prefer a dedicated structure to lookup such information (could be a virtual interface which just goes back to the EventSetupProvider hidden inside of it).

In fact, maybe just returning a std::map<ESRecordIndex, std::vector<ESProductResolverIndex>> and then have the call which setups the data for the Service callback use that might be sufficient.

This should probably be ESResolverIndexContainer

Since this is nominally the order of the setWhatProduced method calls, how about we change it to producedID? If we keep using the transitionID name for this one it is very confusing with the use of transition already in the ED system.

Should there be one function which returns both (rather than 2 functions)?

Should we change doModulesWhoseProductsAreConsumedBy so it also uses Transition instead of BranchType?

I don't like exposing even via forward reference eventsetup::ESProductResolverProvider to everything that deals with a PathsAndConsumesOfModulesBase.

I think we should use a different data structure and avoid exposing the inner details of the EventSetup system.

@Dr15Jones We are agreed that I need to remove the dependence on EventSetupProvider and ESProductResolverProvider from the interface available to Services. There are a couple of options I can think of to accomplish this.

• I could remove the dependence on EventSetupProvider and ESProductResolverProvider from PathsAndConsumesOfModules entirely. The downside to this that we would need to calculate all elements of the std::vector<std::vector<EventSetupConsumesInfo>> always when initializing PathsAndConsumesOfModules and it just saves the result. Currently, most of the time we never calculate this at all, only when Tracer is printing out the dependences. So this involves extra CPU work, memory churn, and temporarily some extra memory to calculate this.
• I could allow PathsAndConsumesOfModules to contain pointers to EventSetupProvider and ESProductResolverProvider, but only as private members that are used but not accessible. The Services would not have access to them. Note that PathsAndConsumesOfModules is a Framework class so its dependence on EventSetupProvider and ESProductResolverProvider is less of a problem than allowing Services to depend on them.

I can implement this either of those ways, just let me know what you prefer. Or maybe you have a better idea?

How about an abstract virtual class which has the job to return that info? Then Services would only depend on that interface and not on any parts of the ES system itself.

PathsAndConsumesOfModulesBase is an abstract virtual base class and that is what the Services currently have access to (before this PR). Other than the constructor/destructor, all the base class functions have the pattern non-virtual function calls pure virtual function which is defined in PathsAndConsumesOfModules. PathsAndConsumesOfModules is the derived class. The only things with access to the derived class are EventProcessor and SubProcess. Is that good enough?

Sure.