Improve usability of LinkNavigator from python

[tmadlener]

Attempt for checking whether this is enough to get some parts of the LinkNavigator work more easily from the python side

BEGINRELEASENOTES

• Add dedicated overloads for Mutable handles to LinkNavigator::getLinked to make python bindings also work with Mutable handles.

ENDRELEASENOTES

See #918 for some more discussion and insights.

@dudarboh this currently adds the "easy" part of more overloads that should hopefully help with calling getLinked with Mutable handles. It doesn't yet add any other python wrapping for improving the other cumbersome parts that you found.

[dudarboh]

@tmadlener, I quickly tested, seems to work with mutables now 👀

[tmadlener]

Thanks for checking.

[tmadlener]

We should add also the python wrapping parts here to make it really easy to use.

[tmadlener]

I made Claude come up with some "python bindings" and this looks like a reasonable solution to me. The one thing that doesn't yet transpire through the whole cppyy machinery is the capabilities of structured bindings for the WeightedObject, i.e. in c++ I can do

podio/tests/unittests/links.cpp

Line 665 in 2a69481

const auto& [cluster, weight] = linkedClusters[0];

But trying to do the same in python currently fails with

Traceback (most recent call last):
  File "/home/madlener/work/AIDASoft/podio/python/podio/test_LinkNavigator.py", line 85, in test_multiple_links
    for c, w in linked:
        ^^^^
TypeError: WeightedObject<ExampleCluster> object does not support indexing

[tmadlener]

Many CI jobs are failing due to the strace test. It looks like adding new python bindings also increases the calls to open shared libraries.

@jmcarcell do you think adding the workarounds that were added to resolve #770 (#808 and #775) should also be added to the LinkCollectionImpl.h?

[jmcarcell]

I have pushed a fix, as usual it is not trivial and it's in a different place from the other cases. I'll think about this tomorrow.

[tmadlener]

Thanks :)

@dudarboh do you want to give this another go before we merge to see if it indeed fixes the issues you observed?

[dudarboh]

I quickly tested everything looks good, @tmadlener .

Could we maybe also add LinkCollection to the podio __init__.py? Because

import edm4hep
import ROOT
ROOT.podio.LinkCollection[edm4hep.ReconstructedParticle, edm4hep.MCParticle]

works, but

import edm4hep
import podio
podio.LinkCollection[edm4hep.ReconstructedParticle, edm4hep.MCParticle]

doesn't

[tmadlener]

Would the use case for that be to write LinkCollections? Or do you still need it for using them when reading them from file? In principle, I assume it's fairly straight forward to do.

[dudarboh]

I think only for writing. Reading works fine now without providing explicit temlplate arguments:

nav = podio.LinkNavigator( link_col )

[tmadlener]

Merging this tomorrow unless there are more comments.

[jmcarcell]

There is a problem with this, if included in several translation units, then it won't link since the symbol will be defined several times. Both inline or constexpr trigger the issue of opening all the libraries (just checked).

[tmadlener]

Can we do something like #if !defined(__CLING__) to conditionally turn this off when going through cling while keeping the desired inline constexpr behavior for "normal" usage?

[jmcarcell]

Using checking for __CLING__ works and I have pushed it. Having a look at
this, the purpose of these tags is simply to select an overload for getLinked. There could be instead getFrom and getTo and we wouldn't need to pass these empty structs around, in addition to having to type less and not defining tags in the global podio:: namespace. And also a bit more explicit. getLinked with a tag argument would be the obvious choice. The automatic deduction of To From depending on types when the type is different would be lost but is it really needed that much?

[tmadlener]

Thanks for having a look and pushing the changes. Seems like they are working.

One of the key use cases for using the LinkNavigator is (IMO) precisely the fact that people don't have to remember which type is FromT and which type is ToT, so having a getLinked that just does the right thing is what we want, I think.

However, looking at the code, it looks like we could still get rid of the tag types and just introduce the getLinkedTo and getLinkedFrom and keep all the existing functionality because that is what is currently there in effect in any case. I will put that into another PR with a proper deprecation warning.