Fix accessing input file metadata in MetadataSvc

[m-fila]

BEGINRELEASENOTES

• Added tests reading metadata from input file
• Fixed accessing input file metadata in MetadataSvc

ENDRELEASENOTES

In the MetadataSvc added in #215 the matadata was split into two frames:

• the first one in theIMetadataSvc collecting metadata put during current data processing,
• the second one in theMetaDataSvc (shadowing the variable from interface) holding metadata from an input file. This frame was not accessible with get and put.

In this PR I propose to have one metadata frame containing both metadata from input and anything set in current processing. The frame will be not saved if it doesn't exist:

• there was no metadata in input,
• and no metadata was added with put.

Reading parameters from not existing frame gives empty optional

[andresailer]

Looks good to me!
Thanks for the fix!

[tmadlener]

One small thing I just realized as well, we have the MetadataSvc but we have MetaDataHandles. IIUC both of them will stay around right? Should we try to homogenize the naming of the two things?

Another question: Does the MetadataSvc still work with podio < 1.0? I.e. before the Frame::getParameter started to return std::optional?

[m-fila]

I feel like these are questions for @jmcarcell as he was the original author of the service

From my understanding:

• the MetaDataHandle stays not to break already existing algorithms using them and for compatibility with PodioDataSvc.
• I'd prefer to use MetadataWhatever rather than MetaDataWhatever as "metadata" is a legit word, but probably it'd be easier to do the opposite and rename toMetaDataSvc
• I didn't check but I think it should work with podio<1, since return can be implicitly converted from value to optional as long as optional return type is declared. So return frame->getParameter<T>(name); should work for frame->getParameter<T> returning either T or optional<T>

[tmadlener]

I'd prefer to use MetadataWhatever rather than MetaDataWhatever as "metadata" is a legit word, but probably it'd be easier to do the opposite and rename toMetaDataSvc

I also like the Metadata better than MetaData, but I also agree with your assessment. Let's see whether the MetaDataHandle is supposed to stay before we discuss this further.

[jmcarcell]

Having a look at this, it's possible to fix the issue of the two frames by removing the redeclaration in

k4FWCore/k4FWCore/components/MetadataSvc.h

Line 39 in 941da76

std::unique_ptr<podio::Frame> m_frame;

, then there will only be one frame, all the tests work and empty optional is returned when there isn't a value. The rest of the changes are interface changes to use getFrame and setFrame instead of accessing directly m_frame (which would be possible after deleting the redeclaration and having only one). For simplicity I prefer accessing m_frame directly, since there are only two points of contact with the metadata service, one in IOSvc to set the frame if it's read from a file and another one from the Writer algorithm to write the metadata frame. Then the metadata interface and service are very simple and small. With the setters and getters I feel it's too complicated for what it does. First of all, there is a unique pointer owning the frame but the getters give you back raw pointers. There is also a check for whether the frame is valid or not and then give you an std::nullopt but this is already implemented in getParameter.

[m-fila]

tl;dr I really don't like having and exposing public unique_ptr member as part of the interface

Having a look at this, it's possible to fix the issue of the two frames by removing the redeclaration in

k4FWCore/k4FWCore/components/MetadataSvc.h

Line 39 in 941da76

std::unique_ptr<podio::Frame> m_frame;

, then there will only be one frame, all the tests work and empty optional is returned when there isn't a value.

I'd argue that the frame should go from IMetaDataSvc rather than from MetaDataSvc. The IMetaDataSvc is supposed to be an interface and shouldn't contain implementation. Having implementation of set and put is already pushing it, but since they are supposed to be templates then this is the easiest way

The rest of the changes are interface changes to use getFrame and setFrame instead of accessing directly m_frame (which would be possible after deleting the redeclaration and having only one). For simplicity I prefer accessing m_frame directly,

I agree that having the setter and getter on this service isn't pretty. But I see them as lesser evil:

• The setter was already there due to coupling with IOSvc.
• The protected getter is to give access for set and put and is a workaround for having templated methods in an interface that otherwise have everything virtual
• The public const getter is due to the coupling with Writer. I think that giving access to a const raw pointer is safer than exposing a std::unique_ptr. The getter gives raw pointer becasue this is an idiomatic way for not giving an ownership - there is no need to transfer ownership to the reader as it accepts a const Frame&

I see there is a strong coupling between IOSvc and MetadataSvc and I'm not sure if overall it wouldn't be better to have a single concrete class that implements bothIIOSvc and IMetadataSvc rather than having two separate implementations that depend on each other (IOSvc even creates the MetadataSvc)

There is also a check for whether the frame is valid or not and then give you an std::nullopt but this is already implemented in getParameter

I'm not sure to which getParameter you are referring to. The service creates a frame on the first put or if there is a frame from the input data given to it by IOSvc. So there is a state in which there is no frame in the service

• podio::Frame::getParameter can't be even used if the frame wasn't created
• k4FWCore::getParameter checks service validity not whether the service holds a valid frame.

[tmadlener]

The public const getter is due to the coupling with Writer. I think that giving access to a const raw pointer is safer than exposing a std::unique_ptr. The getter gives raw pointer becasue this is an idiomatic way for not giving an ownership - there is no need to transfer ownership to the reader as it accepts a const Frame&

I suppose we can't make that return references, because it's possible that getFrame is called, when we don't have a valid m_frame yet, and we need the fact that we can do the nullptr check? I agree that the raw pointer already signifies that the caller doesn't get ownership.

[m-fila]

I suppose we can't make that return references, because it's possible that getFrame is called, when we don't have a valid m_frame yet, and we need the fact that we can do the nullptr check? I agree that the raw pointer already signifies that the caller doesn't get ownership.

Yes, getFrame should return either a valid frame or signalize invalid state, so a reference is not good for that. Optional also doesn't work since we need reference semantic and there is no std optional reference

[tmadlener]

Looks good to me. Do we have documentation for this somewhere already? Since this should be the preferred way of using things, I think we should make sure this is what we advertise for these use cases (ideally, the only thing).

One potential improvement that I am not sure if it's worth putting in yet: If someone tries to put some parameter that podio doesn't support yet, they will get a rather cryptic compiler error, because they will hit an enable_if at some point inside podio. We could catch those cases a bit earlier with a static_assert (and a human readable message) in the MetadataSvc interfaces if we wanted to.

[jmcarcell]

There is also a check for whether the frame is valid or not and then give you an std::nullopt but this is already implemented in getParameter

I'm not sure to which getParameter you are referring to. The service creates a frame on the first put or if there is a frame from the input data given to it by IOSvc. So there is a state in which there is no frame in the service

* `podio::Frame::getParameter` can't be even used if the frame wasn't created

* `k4FWCore::getParameter` checks service validity not whether the service holds a valid frame.

I was comparing before to now: now whether a valid frame is there has to be checked and if not then empty optional is returned; before there was always a frame and whatever podio returned was returned. I guess from the interface it doesn't make sense to assume the frame pointer will be not a nullptr. These are the lines: https://github.com/key4hep/k4FWCore/pull/223/files#diff-9efba907d64eafc80d74009969525571f65e8fc21b43175a0a7279a795c11826R41-R44.

For joining the services I think in the future it's more likely the case of splitting them since they do different things even if the MetadataSvc depends on the IOSvc (but creating the MetadataSvc from the IOSvc is done for convenience and is not necessary). I think it makes sense, they are parts that do different things.

[m-fila]

Looks good to me. Do we have documentation for this somewhere already? Since this should be the preferred way of using things, I think we should make sure this is what we advertise for these use cases (ideally, the only thing).

We have an issue #212 for documenting the new IOSvc, I guess MetadataSvc should be included there. I'll be happy to take this later on

One potential improvement that I am not sure if it's worth putting in yet: If someone tries to put some parameter that podio doesn't support yet, they will get a rather cryptic compiler error, because they will hit an enable_if at some point inside podio. We could catch those cases a bit earlier with a static_assert (and a human readable message) in the MetadataSvc interfaces if we wanted to.

Something like:

  template <typename T> void put(const std::string& name, const T& obj) {
    static_assert(podio::isSupportedGenericDataType<T>, "Argument type not supported as podio frame parameter");

still gives ~60 lines of errors but at least there is something readable at the top

I was comparing before to now: now whether a valid frame is there has to be checked and if not then empty optional is returned; before there was always a frame and whatever podio returned was returned

The frame from IMetadataSvc was created only on the first put

[jmcarcell]

Something like:

  template <typename T> void put(const std::string& name, const T& obj) {
    static_assert(podio::isSupportedGenericDataType<T>, "Argument type not supported as podio frame parameter");

still gives ~60 lines of errors but at least there is something readable at the top

Do you want to do it or shall we merge?

The frame from IMetadataSvc was created only on the first put

But the one from MetadataSvc was always there, anyway it doesn't matter.

[andresailer]

Make it private and the IOSvc a friend of the MetadataSvc or something to hide from other users.

[m-fila]

Moved getFrame in IMetadataSvc (interface) to protected and made friends with Writer

[andresailer]

Something like:

  template <typename T> void put(const std::string& name, const T& obj) {
    static_assert(podio::isSupportedGenericDataType<T>, "Argument type not supported as podio frame parameter");

still gives ~60 lines of errors but at least there is something readable at the top

Do you want to do it or shall we merge?

The frame from IMetadataSvc was created only on the first put

But the one from MetadataSvc was always there, anyway it doesn't matter.

@jmcarcell dixit: Alternatively, the enable if could be removed in putParameter in podio and a static_assert added instead? @tmadlener to check if SFINAE allows this

[jmcarcell]

Merging assuming the static_assert discussed above is not needed since it will be implemented in: AIDASoft/podio#676