Safe and efficient use of tensorflow in reco

[jpata]

I'm checking the various producers to understand if the tensorflow::GraphDef and tensorflow::Session are used correctly, efficiently and coherently across reco modules:

• TrackTfClassifier: GraphDef and Session owned by an EventSetup object. The Session is closed when the EventSetup object is deleted. This seems to be the most efficient solution, allowing the same model to be shared across multiple EDProducers, but also the most complicated.
• DeepTau: GraphDef and Session in cache. The Session is closed when the cache is deleted. This seems to be an efficient solution when the model does not need to be shared across multiple EDProducers.
• Egamma PFID: the current proposed implementation has GraphDef in cache, Session in producer. It looks like most of the memory is in fact used in creating the Session, not the GraphDef (igprof). The session is closed when the EDProducer is deleted.
• DeepCore: GraphDef in cache, Session in producer. The Session is not closed.
• DeepMET: GraphDef in cache, Session in producer. The Session is not closed.

Questions:

1. Egamma PFID, DeepCore, DeepMET: would it be more efficient to put the Session as well in the cache object that is shared across threads? I believe it's related to https://github.com/cms-sw/cmssw/issues/32529
2. DeepCore, DeepMET: do we have a memory leak when the EDProducer is deleted but the Session is not closed? Does it matter? Is std::atomic<tensorflow::GraphDef*> in the global cache object actually required?

[jpata]

assign reconstruction

[cmsbuild]

New categories assigned: reconstruction

@slava77,@jpata you have been requested to review this Pull request/Issue and eventually sign? Thanks

[cmsbuild]

A new Issue was created by @jpata Joosep Pata.

@Dr15Jones, @perrotta, @dpiparo, @makortel, @smuzaffar, @qliphy can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

[slava77]

there is also a variant with edm::global module as in #35237

[jpata]

@makortel @Dr15Jones

could you help us understand if std::atomic is actually required here: https://github.com/cms-sw/cmssw/blob/6d2f66057131baacc2fcbdd203588c41c885b42c/RecoMET/METPUSubtraction/plugins/DeepMETProducer.cc#L12 ?

[makortel]

I don't see any reason why that should be std::atomic. The graph_def is written to exactly once (initializeGlobalCache()), then read once per stream (in edm::stream module constructor, that are guaranteed to be called serially), and finally destructed once (globalEndJob()).

The variable could easily be std::unique_ptr<tensorflow::GraphDef>, and given that tensorflow::createSession() takes const GraphDef*, that could even be edm::propagate_const<std::unique_ptr<tensorflow::GraphDef>> (i.e. the DeepMETProducer would see "constant pointer to constant GraphDef").

[Dr15Jones]

If the GraphDef doesn't need to be changed after it is created, then just a std::unique_ptr<const tensorflow::GraphDef> would work.