WIP: Schema backwards compatibility #272
Conversation
This is not meant to be the last word for backwards compatibility. In fact, it's really only the first word. There's a lot more work to be done and more thought to be given to the mechanisms that are required to support this kind of behavior. Also, this particular case isn't even all that critical--it just serves as a potentially useful example.
|
Here's a fairly unstructured list of thoughts, questions, considerations, and the like (mostly for my own benefit):
|
|
I think I will make comments in reverse order.
[Added comment] I guess one case that may happen frequently, a new version of the schema supports some special case. One encounters a file made under the new schema, but it actually doesn't use the new features of that schema. In that case, one can try to read that element with the schema the library supports, and if it reads without error, then that's ok (or is it? I wonder if there are schemas that permit arbitrary attributes, and then a new attribute is added and it ignores it when that actually changes the meaning; is that possible?)]
|
This was necessary since the obsgeovel and obsgeoloc fields were affected by the schema version change but are not used in Galactocentric frames, so were not previously being tested.
|
I will respond in pseudorandom order:
|
Yes, I agree that this isn't the best long-term approach and that we will need to do work to avoid multiple tag classes.
I think we should never have a hard failure (i.e., untrapped exception) reading an ASDF file so long as the yaml is well formed. In such a case we issue a warning (at most) and read the unhandled schema tag as a basic python data structure. If backward compatibility is always adhered to, then the main issue is how to handle versions ahead of what the library supports (right?). What I'm suggesting (I don't know if it is practical) is to try reading it with the latest schema that it supports, and if it works, fine (warning about the version), and if not, read it in as a python structure (though if nested items have supported tags, e.g., numpy arrays, they do get handled). I'm not even sure I would agree that a major version change breaks backward compatibility. The basic principle here is that a tag means something that has an invariant aspect to it. Future versions may change how it may be represented, or enhanced. But if we can't represent the old version in the new (by conversion), it's a different entity and should be named differently (and not just by version numbers). I'm open to see good counterexamples (these may have to come from a wider circle) |
After some more investigation, it actually looks like this much is already supported (assuming it does not try and fail to map it to another version of the same type). I will add some unit tests to confirm.
This seems like a good strategy in all cases. I'll do some more digging to see whether this works yet or not (I suspect it doesn't yet but I might be wrong). Still tbd is the best way to accommodate explicit implementations of multiple versions of the same schema. I'll play around more with the ideas proposed above. |
ASDF should not fail to read a file, even if the file makes reference to schema tags that are not defined. In these cases, ASDF should simply provide a raw Python object that represents the deserialized object.
This is an important case that is not currently handled properly. Currently if an exact version match is not found for a schema version, the "best match" is used. However, the tag implementation for this "best match" may not be compatible with the given schema version, which will result in an unhandled error. This needs to be handled properly.
This will allow us to isolate the error that is caused when an incompatible tag class is used to implement a mismatched schema version and reflects the correct behavior in the long run.
All classes that inherit AsdfType are automatically added to the internal list of built-in types. In this case, adding a type that was defined within a unit test to the list of built-ins caused problems for the schema validation tests. This commit is really just a work-around for now. In the long term, there should be another class that can be inherited by both built-in and user-defined types.
This seems like the most logical place to make a decision about whether to write a custom data structure or possible or just return a raw Python object. Added a new method to AsdfTypes `version_is_supported` to determine if a particular tag version can be handled by the class. It's mostly just a stub for now but logic will be fleshed out soon.
This update allows extension types to be differentiated between those that ASDF provides as part of the library, and those that are user-defined. The ASDF types are automatically added to the list of built-in extensions used by the library, whereas user-defined types are not.
|
The latest commit addresses issue #276 by updating the extension type hierarchy. This will be a prerequisite for a clean and general way of determining schema versions that are supported by a particular tag class. |
This update allows a single extension class to support multiple versions of the same schema tag. There might be a better way to do it in the long run, but this way appears to work, at least for built-in classes. It would require a different mechanism to be used for user-defined classes, which is problematic.
In order to correctly implement multiple supported versions of tag classes for all extensions and not just ASDF built-ins, the logic for adding versioned extension classes was moved from the AsdfType metaclass constructor to the constructor for AsdfExtensionList. This allows both AsdfType and UserType sibling version classes to be properly handled. The sibling version classes themselves are now created within the ExtensionType constructor and are stored as property of the main version class.
| @@ -417,6 +454,11 @@ class AsdfType(object): | |||
| version : 3-tuple of int | |||
| The version of the standard the type is defined in. | |||
|
|
|||
| supported_versions : set | |||
There was a problem hiding this comment.
Is it possible to indicate a range of versions supported? I suspect this may be a common case and it would be more painful to have to explicitly list each version that is supported. Also, it may be good to outline version synchronization issues within the library. I.e., is it possible that schemas get updated versions before the library is updated to deal with them? If so, what happens? Is there a way to indicate that we presume that the library supports future versions without out listing them explicitly (e.g., perhaps indicating a range of version where the end range is indefinite). By default the behavior may be that it tries to handle these newer versions and warns that it failed. I wonder if we have considered all the timing issues, but I may be all wet.
There was a problem hiding this comment.
The idea of supporting version ranges occurred to me as well, and I think it's probably the right thing to do. I also think it makes sense to address it in a separate issue after getting this PR merged.
Likewise, I think it makes sense to allow for indefinite ranges. None of this should be too difficult to support; it will just require some modifications to the way these things are represented internally. Again, I'd like to make a separate PR for these.
There was a problem hiding this comment.
I agree it should be in a different PR
As it stands, this is a fairly naive implementation of backwards compatibility for
CelestialFrames. More than anything it's meant to demonstrate how backwards compatibility might be implemented for future tag classes where it is actually required. It's also meant to expose functionality that needs to be added/modified in order to support backwards compatibility. I will continue to update this PR with improvements until it seems mature enough to merge.This is an attempt to address issue #249, and also may be related to #252.