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

Decouple the collection buffer creation from the collection #394

Merged
merged 14 commits into from
May 9, 2023

Conversation

tmadlener
Copy link
Collaborator

@tmadlener tmadlener commented Mar 21, 2023

BEGINRELEASENOTES

  • Introduce a CollectionBufferFactory that can create the necessary buffers from a collection type, a schema version and a subset collection flag.
    • Use this factory throughout all existing Readers
    • Remove createBuffers and createSchemaEvolvableBuffers from podio::CollectionBase interface
  • Make the minimum allowed schema_version 1 in the yaml definition files. Default to 1 if no schema_version is provided
  • Add a schemaVersion to the DatamodelDefinition.h header that is generated and that can be accessed via {{ package_name }}::meta::schemaVersion. Use this to propagate schema information to the necessary places.
  • Make SIOBlocks write the current schema version, such that on reading they can generate the appropriate buffers for the version on file.

ENDRELEASENOTES

This PR continues the cleanup of internals and the introduction of full schema evolution capabilities. The most important bit is that it completely disentangles the buffer creation from the collection interface, so that it is now possible to create buffers without having to create a collection first. This is achieved via a CollectionBufferFactory (singleton) that is populated during datamodel library loading. The rest of this PR is mainly shuffling some parts of existing code around and removing some other bits that were previously necessary to "steal" the buffers from the collection they were created from.

This does not yet address any schema evolution concerns other than providing the possibility to generate different buffers for different schema versions and the basics for populating the factory with versioned creation function. For actual schema evolution functionality this has to be populated accordingly (either by users or via automatic code generation).

@hegner this introduces an other de-facto registry, but I don't see an easy way around that. We could hide the fact that it is a singleton registry behind an interface of global functions if we wanted to.

This should solve the issue that is currently blocking #257.

TODO:

  • Proper writing of schema version for SIO backend (should be possible to use the version mechanism of SIO here)
  • Documentation

@tmadlener tmadlener requested a review from hegner May 3, 2023 07:39
@tmadlener
Copy link
Collaborator Author

Updated the docstring for the CollectionBufferFactory to include the basics and some of the assumptions. Since this is a class that is mostly targeted at internal use, I have not created additional documentation in the user facing markdown files.

@tmadlener tmadlener changed the title [WIP] Decouple the collection buffer creation from the collection Decouple the collection buffer creation from the collection May 3, 2023
@hegner
Copy link
Collaborator

hegner commented May 5, 2023

@tmadlener - did you see the cling error of the IBs before?

Copy link
Collaborator

@hegner hegner left a comment

Choose a reason for hiding this comment

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

Thanks. To me it looks fine.
There is only a nitpicking one about underscore vs camel case for variable names. But that we should discuss for coding conventions anyhow.

@tmadlener tmadlener merged commit d294ae6 into AIDASoft:master May 9, 2023
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.

2 participants