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

Switch to templated links and remove explicit declarations from yaml file #373

Merged
merged 8 commits into from
Dec 18, 2024

Conversation

tmadlener
Copy link
Contributor

@tmadlener tmadlener commented Sep 19, 2024

BEGINRELEASENOTES

  • Switch to templated podio::Links for the Link datatypes
  • Bump the schema version to 3
  • Remove the legacy Association headers that were put in place for a smooth transition to Links

ENDRELEASENOTES

This PR introduces all the existing Link datatypes as typedefs (keeping the same header file names for the collections as before) of podio::LinkCollection<ToT, FromT> and removes their declarations from the yaml file. It also includes the necessary schema evolution code for podio to be able to read back data files that have been written with previous versions of EDM4hep (or at least they will once suitable files have been uploaded for testing).

  • The typedef headers is in include/edm4hep
  • The schema evolution code is in edm4hep/schema_evolution

Both of these can obviously be changed to something more appropriate if necessary.

@tmadlener tmadlener changed the title [WIP] Switch to templated links and remove explicit declarations from yaml file Switch to templated links and remove explicit declarations from yaml file Dec 3, 2024
@tmadlener
Copy link
Contributor Author

The Key4hep release workflows are failing because they don't come with a recent enough version of podio.

@tmadlener
Copy link
Contributor Author

The downstream-build workflow seems broken and it looks like it stumbles during parsing of the PR message? I will try to build this locally to check if there are any leftover Associations still in the stack. Otherwise I would like to get this merged rather soon.

@tmadlener
Copy link
Contributor Author

Nothing breaks for me locally, so I think this can be merged. Waiting another day to give everyone a chance to get a comment or question in.

@jmcarcell
Copy link
Member

The downstream build should be fixed, parsing was almost removed but not quite because of some {{ }} expansion that was messing with the script.

I was not sure about schema evolution being necessary here but 0.99 was already in July so it's all the files produced in the last 6 months with the nightlies or in the last months with the latest release 🤔

Comment on lines 31 to 34
target_include_directories(edm4hep_v2 PRIVATE
$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/edm4hep/schema_evolution/include>
)
target_link_libraries(edm4hep_v2 PUBLIC podio::podio EDM4HEP::edm4hep)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not entirely sure about the naming of this library here. Maybe v2 gives the wrong impression? Ideally this would simply be merged into edm4hep but the problem for that is that we still need a dedicated dictionary at the moment, because the selection.xml for EDM4hep is generated and we cannot simply inject the few datatypes that would be necessary into that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe edm4hepOLE (OldLinkEvolution)? or edm4hepOLD (OldLinkData)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:D that would be some nice backronym in this case. Would be OK for me, but I am not sure we can find one for all future changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed it to edm4hepOldSchemas which should hopefully be a somewhat future proof catch-all.

@tmadlener
Copy link
Contributor Author

Thanks for the fix. Schema evolution is currently here to keep backwards compatibility, since we kind of advertised that this will be a transparent switch (and also to show that it is technically possible). I can have another look to see if it can squeeze it a bit more. In principle it is also possible to just drop that part some time in the future, when everyone is definitely on >= 1.0.

@tmadlener tmadlener merged commit d651cd6 into key4hep:main Dec 18, 2024
8 of 10 checks passed
@tmadlener tmadlener deleted the proper-links branch December 18, 2024 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants