-
Notifications
You must be signed in to change notification settings - Fork 60
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
Add a hook to inject datamodel version information into podio internals #651
Add a hook to inject datamodel version information into podio internals #651
Conversation
Wouldn't it be more straightforward to just add a simple field for version in the yaml and then relay on a build system to populate it somehow (e.g. Or inject the version information with |
I would like to avoid having to Another possibility I thought about was to configure a smaller YAML file with the version information (but then we might be configuring quite a few files in the end).
Hadn't thought about that, but also a possibility. Would also make it a bit more language agnostic. In the end how the information is injected exactly is rather flexible in this PR, so it should be fairly straight forward to do what we want to do. |
b49d7be
to
2c0831d
Compare
I have change the injection of the datamodel version info to be a commond line argument to the class generator instead of being part of the YAML file. I have also added a new |
cc55fd0
to
18a1129
Compare
I am not sure if it's an argument in any direction, but I would at least like to point it out (and write it down for posteriority): If the version information is not part of the YAML configuration, the "roundtripped generated code" might be different because the generated code now depends on some pretty much ephemeral version input. On the other hand if we pull in a c++ variable (like we did in the first approach) we have the same code, but potentially different contents, because the value of that variable can change without visible changes in the generated code. |
18a1129
to
1b81ea4
Compare
1b81ea4
to
b225083
Compare
09aa2b8
to
709ca85
Compare
As discussed during todays EDM4hep meeting, this PR effectively imposes that packages that use podio follow SemVer as we use that format to store the information. There is no real technical blocker to store the version as e.g. a string. However, we will defer implementing that until someone actually requests that feature. |
@@ -505,12 +514,14 @@ def _write_all_collections_header(self): | |||
def _write_edm_def_file(self): | |||
"""Write the edm definition to a compile time string""" | |||
model_encoder = DataModelJSONEncoder() | |||
print(f"{self.datamodel_version=}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
print(f"{self.datamodel_version=}") |
Debugging print? Otherwise it's going to be a bit weird like that whenever it appears with self.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, few seconds too late. This is indeed a debug print that I missed while having another look.
@tmadlener I am a bit confused. What is the "datamodel version"? I can see in the generated files the PODIO version and the EDM4hep schema version but not datamodel. See what I print using the Julia interface:
|
The datamodel version would be the tag for EDM4hep. With the current |
Thanks. What will be the relation between the schema version and the datamodel version? Will the major version of datamodel be the schema version? What about compatibility rules (at least for I/O) ? |
The one thing that should always be true is: If the schema version increases, the datamodel version also has to increase (but not necessarily vice versa). How this increase happens is in principle up for the actual datamodel authors to define any relation they want there. IMHO the minimal set of rules are (if we focus more on the API side of things and assume that we have schema evolution for every change we make to the datamodel):
One aspect that could also be considered, although I am not sure we will ever hit it in reality is the following: It would be possible to reserve major version changes to cases where we drop backwards compatibility mainly on the I/O side (i.e. we explicitly exclude the API side from SemVer). This would happen if we don't want to (or can't) provide a schema evolution mechanism for some changes to the datamodel. But I am not sure if we should consider this as a "normal use case". |
BEGINRELEASENOTES
ENDRELEASENOTES
@peremato this would make e.g. the EDM4hep version appear in the output files (once the few lines are added to the
edm4hep.yaml
file and its version header).Conceptually, this relies on the fact that a datamodel will have a version header of some form and will just try to reuse that. The main requirements are that the version class of the datamodel is convertible to a
podio::version::Version
.Fixes #630