Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow easier extension of data models (e.g. EIC builds on edm4hep) #263

Closed
wdconinc opened this issue Mar 15, 2022 · 2 comments · Fixed by #317
Closed

Allow easier extension of data models (e.g. EIC builds on edm4hep) #263

wdconinc opened this issue Mar 15, 2022 · 2 comments · Fixed by #317

Comments

@wdconinc
Copy link
Contributor

In EIC we have found ourselves unable to build on edm4hep, and instead necessitated to reimplement edm4hep in an eicd namespace. While we can create new data types and combine them in the same file with edm4hep data types, this ultimately does not allow that much freedom since we wish to maintian a 'chain of custody' with relations to the originating objects. Pretty quickly that means we cannot extricate ourselves from a web of edm4hep data types.

While it is to some extent intentional that inheritance of podio data models is restricted to avoid fragmentation with incompatible extensions of data models, this is now leading to duplication of the same data model in a different namespace where there is no connection any more to the originating data model. That would appear on the face of it to be a worse situation. As we understand this is not limited to EIC vis-a-vis edm4hep.

We wonder if a more flexible structure that allows some inheriting of data model components can be allowed.

@tmadlener
Copy link
Collaborator

We have had a similar discussion already in #168, so this is something that is on our list, but maybe can be bumped in priority now that we have another request for this.

Just to add a bit of technical context to this. From the c++ side there is in principle nothing that prevents this, as long as you make sure that you have your includes straight. This might necessitate to tweak some target_include_directories and target_link_libraries for the dependent data model as well, because the current podio cmake macros are not designed to handle dependencies to other data model definitions at the moment. What is actually preventing this from working at the moment is a bit of validation code that we do on the python side, before we start code generation:

def _check_relations(self, classname, definition):
"""Check the relations of a class"""
many_relations = definition.get("OneToManyRelations", [])
for relation in many_relations:
if relation.full_type not in self.datatypes:
raise DefinitionError("'{}' declares a non-allowed many-relation to '{}'"
.format(classname, relation.full_type))
one_relations = definition.get("OneToOneRelations", [])
for relation in one_relations:
if relation.full_type not in self.datatypes:
raise DefinitionError("'{}' declares a non-allowed single-relation to '{}'"
.format(classname, relation.full_type))

As you can see, we specifically check that relations are inside the datamodel, but patching out that requirement should in principle not break the code generation, if you want a quick fix already now. However, we have never tested this, so there might be some other hidden issues lurking.

@hegner
Copy link
Collaborator

hegner commented Mar 15, 2022

Conceptually it is not too complicated to do. Thinking about it, so far I think we need this:

  • Having the initial data model pre-compiled and accessible
  • Access to the yaml file of the base data model (whether we install it alongside the bins or not is another discussion)
  • an explicit declaration of a dependency on a different data model
    • whether we want to support a multi-layer approach is to be discussed
  • separating the lists for internal consistency check from the lists of data types to be created
  • fixing the linking and include directory business. That requires to check and fix the CMake.

With this we should be able to do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants