Use Gaudi::Functional, add examples and adapt PodioInput to use Gaudi::Functional #129

jmcarcell · 2023-07-27T13:34:16Z

BEGINRELEASENOTES

Make changes in DataWrapper.h so that algorithms using Gaudi::Functional can work
Add examples and tests of a consumer, producer and transformer and their versions with multiple inputs and/or outputs
Adapt PodioInput to use Gaudi::Functional

ENDRELEASENOTES

This PR keeps the old tests (that still work) that we can remove or adapt later. There should be no breaking changes and so far the changes to PodioInput seem to work fine.

test/k4FWCoreTest/CMakeLists.txt

Zehvogel · 2023-08-02T06:50:49Z

Nice that the tests work now! :)

I am still a bit confused about the semantics, specifically using a consumer for reading.
My Impression was that this would be done by a producer in the functional approach, but this is completely side-stepped by the DataSvc? PodioInput now behaves like a constant (if there is such a thing in Gaudi functional) as you are not taking any inputs and don't produce any outputs (directly).

hegner · 2023-08-03T13:22:43Z

test/k4FWCoreTest/src/components/ExampleFunctionalConsumer.cpp

+// Which type of collection we are reading
+// this will always be podio::CollectionBase
+// Has to be wrapped in DataWrapper
+using colltype = DataWrapper<podio::CollectionBase>;


As just discussed in person you can avoid these using statements and the user side visibility of handles and wrappers by changing BaseClass_t.
We are using our special wrappers because of what is needed for PODIO here.
To make Gaudi Functional use these, we have to make them visible to [Consumer/Producer/Transformer]. which internally use DataHandleMixin. The latter uses this template magic to know which Handles to use. To override the default we need to subclass of Gaudi::Algorithm that adds something like

template<typename T> using InputHandle_t = DataObjectReadHandle<DataWrapper<T>>;

and a similar one for the writing part.

Doing this we will not be able to use the legacy read/write any more though. If we want to preserve that, we have to use the key4hep specific data handle which we wanted to deprecate.

I was able to do this for the producers. When trying to do the same thing for the consumers (or transformers since they also have inputs) I get the following error message:

ExampleFunction... ERROR Wrong DataObjectType : The type expected for /Event/MCParticles is AnyDataWrapper<edm4hep::MCParticleCollection> and is different from the one of the object in the store which is DataWrapper<podio::CollectionBase>.

that is coming from Gaudi performing a check with a dynamic_cast: https://gitlab.cern.ch/gaudi/Gaudi/-/blob/master/GaudiKernel/include/GaudiKernel/DataObjectHandle.h#L56. Options:

Templated registering of objects, not trivial to do

Using a wrapper around Gaudi::Functional::Consumer (and transformer) that retrieves the collection as podio::CollectionBase and then casts to the right type, but I like that our algorithms and the ones from Gaudi look the same

Maybe it's also possible to have a wrapper that we can use for registering and then wrappers of concrete types can inherit from so that the dynamic_cast check would pass.

Ideally I would like it to be like it is in the example producers right now, where we inherit from the corresponding Gaudi::Functional and we just have to specify the type of collection that we want.

Ideally I would like it to be like it is in the example producers right now, where we inherit from the corresponding Gaudi::Functional and we just have to specify the type of collection that we want.

I agree with this. In case it is not possible the option I would like the second most after this one is to have a k4FWCore::Functional (naming up for discussion) that is effectively a template specialization of the corresponding Gaudi::Functional that injects the necessary types for us. I don't know if this is something that is possible, or if would always need a wrapper.

Do I understand correctly that the whole maybeRead machinery is necessary to work around the AnyDataWrapper vs DataWrapper issue on the consuming side?

The maybeRead is there to simplify the interface and not have to use podio::CollectionBase as type and then do a dynamic cast by pushing directly, for example, an edm4ep::MCParticleCollection to the TES. There is maybe an option to create a custom Functional base class we can inherit from in k4FWCore that somehow gets around this so that it is possible to retrieve a podio::CollectionBase and do the relevant dynamic casting inside the class and not have the maybeRead stuff, but I don't think this is trivial to do

k4FWCore/include/k4FWCore/FunctionalUtils.h

hegner · 2023-09-11T12:00:20Z

k4FWCore/components/PodioInput.cpp

@@ -21,21 +21,172 @@

 #include "k4FWCore/PodioDataSvc.h"

+#include "edm4hep/MCParticleCollection.h"


IMHO doing all this explicitly is the wrong approach and not really composable. Since the Input is always on top of the scheduling graph - can't we just push things into the event store with AnyData handles?

The problem is that we have to register objects with the type we want to get, otherwise there is the check I have mentioned in one of the comments above that will try to dynamic_cast whatever is in the store (DataWrapper<collectionType>) to what we are asking for. Pushing a DataWrapper<podio::CollectionBase> like before without templating the registration and getting back a AnyDataWrapper<class> doesn't work since you can't dynamic_cast one into the other

jmcarcell · 2023-09-11T12:21:57Z

The latest commit 15e9202 (#129) simplifies the input interfaces allowing not to have to do dynamic casts and not having to use DataWrappers, the price to pay is having to template the registering of objects and having to have explicitly the EDM4hep classes. For classes that are not in EDM4hep they are registered as podio::CollectionBase so using DataWrapper and dynamic casts will be needed. This is one way of doing this, there are others possibly but this is probably from the user interface the least invasive being it almost identical to a non-k4FWCore Gaudi functional algorithm.

hegner

Thanks. Very nice examples you put. I left a few more comments to address

hegner · 2023-09-29T07:20:42Z

README.md

 cmake ..
 make install
 ```

-
+## Implementing algorithms
+k4FWCore uses (or will use) `Gaudi::Functional` for executing algorithms. In


I'd remove the "or will use". And then: "There are three different types of algorithms, depending on your use case:" And then the enumeration... if you open a ticket on me I can make the following text a bit more explicit - the complicated part is not in the 'operator' part, but in the templating. Which I would add.

I have added a few more comments and a link to a more complete list that I found in the LHCb starterkit. If you want to change it I think you should be able to push to this branch

hegner · 2023-09-29T07:20:57Z

README.md

+outputs (like `ExampleFunctionalProducerMultiple`) that can be used as a
+template for the more typical case when working with multiple inputs or outputs.
+
+`GaudiAlg` is deprecated and will be removed in future versions of Gaudi.


emphasize it visually

Do you mean add an example here? Or at least the structure with the class definition and template arguments and operator()?

hegner · 2023-09-29T07:23:08Z

k4FWCore/include/k4FWCore/BaseClass.h

+
+// Base class used for the Traits template argument of the
+// Gaudi::Functional algorithms
+struct BaseClass_t {


This is a badly chosen name - and not even name spaced. Do you have something more prescriptive what the purpose is?

Here the idea is that the Gaudi examples always use a BaseClass_t that is defined to be typically Gaudi::Algorithm so by having this the changes are minimal. See for example https://gitlab.cern.ch/gaudi/Gaudi/-/blob/master/GaudiExamples/src/FunctionalAlgorithms/MakeAndConsume.cpp#L60 and a few lines below; In k4FWCore we don't need line 60 but we need instead #include "BaseClass.h" and the rest is exactly the same as in the Gaudi examples. We could also name it differently and have it namespaced but I thought for consistency it would be simpler this way.

k4FWCore/src/PodioDataSvc.cpp

test/k4FWCoreTest/src/components/ExampleFunctionalConsumerMultiple.cpp

test/k4FWCoreTest/src/components/ExampleFunctionalTransformer.cpp

…rMultiple

…ction

… algorithms

jmcarcell · 2023-10-04T13:08:52Z

As discussed in a private meeting, and for the sake of time for a coming conference and tutorial, this PR will be merged by the end of today if there aren't any complains. pre-commit and the builds with the nightlies both are happy, so it should be ready to go

tmadlener

I had a brief look at the examples and I do have some "style" comments.

tmadlener · 2023-10-04T13:52:12Z

test/k4FWCoreTest/src/components/ExampleFunctionalConsumer.cpp

+#include <string>
+
+// Which type of collection we are reading
+using colltype = edm4hep::MCParticleCollection;


Can we remove this typedef? It's effectively used twice (as far as I can see), and it reduces readability IMHO. Additionally, there are other examples with multiple collections that show the usage with a typedef which are much clearer, I think.

If not removing it, at least we should give it a more descriptive name.

tmadlener · 2023-10-04T13:54:11Z

test/k4FWCoreTest/src/components/ExampleFunctionalProducer.cpp

+#include <string>
+
+// Which collection we are producing
+using colltype = edm4hep::MCParticleCollection;


In a similar fashion as above, I would remove this typedef.

tmadlener · 2023-10-04T13:55:34Z

test/k4FWCoreTest/src/components/ExampleFunctionalProducerMultiple.cpp

+using Float_t         = podio::UserDataCollection<float>;
+using Particle_t      = edm4hep::MCParticleCollection;
+using SimTrackerHit_t = edm4hep::SimTrackerHitCollection;
+using TrackerHit_t    = edm4hep::TrackerHitCollection;
+using Track_t         = edm4hep::TrackCollection;


Nitpicking here: These typedefs use _t, but the example above doesn't. I think at least in the examples we should have consistency.

tmadlener · 2023-10-04T13:56:47Z

test/k4FWCoreTest/src/components/ExampleFunctionalTransformerMultiple.cpp

+using FloatColl         = podio::UserDataCollection<float>;
+using ParticleColl      = edm4hep::MCParticleCollection;
+using SimTrackerHitColl = edm4hep::SimTrackerHitCollection;
+using TrackerHitColl    = edm4hep::TrackerHitCollection;
+using TrackColl         = edm4hep::TrackCollection;
+
+// As a simple example, we'll write an integer and a collection of MCParticles
+using Counter_t  = podio::UserDataCollection<int>;
+using Particle_t = edm4hep::MCParticleCollection;


Same here as above, unless making the in- and outputs follow different conventions was a conscious choice?

jmcarcell · 2023-10-04T18:18:30Z

I had a brief look at the examples and I do have some "style" comments.

All fixed. They have been changed through many iterations and at some point I became so used to them that I stopped noticing they were around

jmcarcell changed the title ~~Use Gaudi::Functional, add examples and move PodioInput~~ Use Gaudi::Functional, add examples and adapt PodioInput to use Gaudi::Functional Jul 27, 2023

jmcarcell force-pushed the functional branch from a1a96cf to a0c9bcd Compare July 27, 2023 20:26

Zehvogel reviewed Aug 1, 2023

View reviewed changes

test/k4FWCoreTest/CMakeLists.txt Show resolved Hide resolved

jmcarcell requested a review from hegner August 1, 2023 15:35

jmcarcell force-pushed the functional branch from 7e86b11 to eac9363 Compare August 1, 2023 15:37

hegner reviewed Aug 3, 2023

View reviewed changes

tmadlener reviewed Aug 17, 2023

View reviewed changes

k4FWCore/include/k4FWCore/FunctionalUtils.h Outdated Show resolved Hide resolved

jmcarcell force-pushed the functional branch 2 times, most recently from f0ab0a2 to 3914c0b Compare September 11, 2023 11:51

hegner reviewed Sep 11, 2023

View reviewed changes

jmcarcell force-pushed the functional branch 2 times, most recently from 0ddc479 to 3b09d32 Compare September 11, 2023 12:17

jmcarcell force-pushed the functional branch 2 times, most recently from 7799902 to 614efba Compare September 15, 2023 07:32

jmcarcell mentioned this pull request Sep 20, 2023

Add support for reading any number of collections with Gaudi::Functional #145

Closed

jmcarcell force-pushed the functional branch from 8cd5a87 to 050cff3 Compare September 28, 2023 11:58

hegner requested changes Sep 29, 2023

View reviewed changes

jmcarcell mentioned this pull request Oct 3, 2023

Add a planar digitisation algorithm key4hep/k4Reco#1

Merged

jmcarcell force-pushed the functional branch from 0e8a8bf to 8baed4b Compare October 3, 2023 17:26

jmcarcell added 9 commits October 3, 2023 19:27

Add example producer

dbc4808

Up

dbfe999

Up

3c40628

Up

f74c865

Update the example producer

28a35c1

Change to Functional Producer

bdbcf73

Add a consumer, producer and transformer

ae4f679

Remove deleted file

3d88081

Add some information in the README about Gaudi::Functional

26e08e8

jmcarcell added 23 commits October 3, 2023 19:27

Fix name

638f7e5

Fix the producers

cb6139f

Fix tests

57f7c2b

Fix the consumer tests

da2107e

Fix tests

5294aab

Add registering template, simplify input interfaces for functional algs

51890e0

Rename FunctionalUtils.h to BaseClass.h

f58bc21

Don't include the datawrappers

5b0d022

Reorganize imports

9e79c83

Remove CollName from the arguments of maybeRead

8ae87a3

Produce an empty MCParticleCollection in the exampleFunctionalProduce…

b5840e8

…rMultiple

Fix a maybeRead call since type was removed in a previous commit

89b29ee

Fix the tests after the producer multiple outputs two MCParticleColle…

f5a5040

…ction

Add a comment explaining a test dependency

f185ca3

Remove the podio/CollectionBase.h include

4e3ee36

Adress comments from PR

cbf9987

Add header explaining each functional test

716c5c4

Add tests chaining functional algorithms and mixing them with the old…

6849e50

… algorithms

Remove some unneeded includes

8baed4b

Fix the wrong name of a test

39d560c

Run clang-format

b80a6cb

Fix licenses

bda0c3c

Fix remaining licenses

86750a4

tmadlener reviewed Oct 4, 2023

View reviewed changes

Fix typedef style

550d7c8

jmcarcell merged commit 4eb5cd8 into main Oct 4, 2023
4 of 9 checks passed

jmcarcell deleted the functional branch October 4, 2023 18:26

Emphasize message in the README

4d03256

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Gaudi::Functional, add examples and adapt PodioInput to use Gaudi::Functional #129

Use Gaudi::Functional, add examples and adapt PodioInput to use Gaudi::Functional #129

jmcarcell commented Jul 27, 2023 •

edited

Loading

Zehvogel commented Aug 2, 2023

hegner Aug 3, 2023 •

edited

Loading

jmcarcell Aug 22, 2023

tmadlener Aug 22, 2023

tmadlener Oct 4, 2023

jmcarcell Oct 4, 2023

hegner Sep 11, 2023

jmcarcell Sep 11, 2023

jmcarcell commented Sep 11, 2023 •

edited

Loading

hegner left a comment

hegner Sep 29, 2023

jmcarcell Sep 30, 2023

hegner Sep 29, 2023

jmcarcell Sep 30, 2023

hegner Sep 29, 2023

jmcarcell Sep 30, 2023

jmcarcell commented Oct 4, 2023

tmadlener left a comment

tmadlener Oct 4, 2023

tmadlener Oct 4, 2023

tmadlener Oct 4, 2023

tmadlener Oct 4, 2023

jmcarcell commented Oct 4, 2023

		@@ -21,21 +21,172 @@

		#include "k4FWCore/PodioDataSvc.h"

		#include "edm4hep/MCParticleCollection.h"

Use Gaudi::Functional, add examples and adapt PodioInput to use Gaudi::Functional #129

Use Gaudi::Functional, add examples and adapt PodioInput to use Gaudi::Functional #129

Conversation

jmcarcell commented Jul 27, 2023 • edited Loading

Zehvogel commented Aug 2, 2023

hegner Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmcarcell commented Sep 11, 2023 • edited Loading

hegner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmcarcell commented Oct 4, 2023

tmadlener left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmcarcell commented Oct 4, 2023

jmcarcell commented Jul 27, 2023 •

edited

Loading

hegner Aug 3, 2023 •

edited

Loading

jmcarcell commented Sep 11, 2023 •

edited

Loading