OpenTimelineIO icon indicating copy to clipboard operation
OpenTimelineIO copied to clipboard

Add default fallback value to serializable field properties in Python

Open timlehr opened this issue 2 years ago • 6 comments
trafficstars

Feature Request

Add a default fallback value to all Python properties.

Description

Currently, serializable fields are plain properties in Python. If they do not have any data written to them, or get explicitly initialized in __init__, they will fail on access, as their field does not yet have an respective entry in _dynamic_fields. That entry will only be created once the field setter function has been called at least once.

  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/site-packages/opentimelineio/core/__init__.py", line 283, in getter
    return self._dynamic_fields[name]
KeyError: 'source_url'

This requires all the fields to be specified during __init__ at all times or the code will fail. If a schema has been serialized sparsely (or was handcrafted in an unsupported environment without access to the schemadef), replacing the repetitive init code with something that iterates over **kwargs won't work, as those field values won't even be passed / are unknown to the system. OTIO currently doesn't keep a list of all serializable fields unless they have a value already. As the field objects are just plain properties it's also not possible to determine all field names programatically without subclassing / monkey patching.

  def __init__(self, **fields):
      super(StudioOtioBaseSchema, self).__init__()

      # iterate over all provided field values and set them
      for field_name, field_value in fields.items():
          setattr(self, field_name, field_value)

While these loosely defined fields are not ideal in the first place, I feel like there is an argument to be made for having a transient fallback value that gets specified alongside the defined field, in case the field gets accessed, but doesn't have a value assigned / hasn't been serialized yet.

# current
metadata = otio.core.serializable_field(
    "source_url", required_type=str
)

# proposal
metadata = otio.core.serializable_field(
    "source_url", required_type=str, default_value=""
)

timlehr avatar Aug 23 '23 22:08 timlehr

Can i have a look at the code please

Ashray-1501 avatar Aug 24 '23 13:08 Ashray-1501

We considered something like this in the early designs for OTIO, but we were concerned about the integrity of archival data being jeopardized by changes in default values across versions of OTIO. Take for example the scenario where sparse JSON for a Timeline is written out without a global_start_time. If later a default value for global_start_time is introduced, that might retroactively change the timing of an old OTIO file where the missing value was assumed to be something else. In order to mitigate this risk, we decided that OTIO files should not be sparse (e.g. they should have all fields filled in with values) and that there would be no default value. We can certainly reconsider this choice, and in fact there is some support for reading sparse JSON files since some people "thinned" their OTIO files against the guidance above. (Not their fault, since I don't know if we documented that advice clearly anywhere.) Tim, can you elaborate on the use case you have in mind for this feature request?

jminor avatar Aug 24 '23 16:08 jminor

@jminor I thought this might be the design decision behind it. I think with the schema versioning / upgrade / downgrade capabilities we have now, it could be feasible / safe again.

Our use case is not really critical at all - we didn't manage to write out a nil value (source_url: nil) to a metadata dictionary (seems to be pruned by Swift according to Dale) for a property using the Swift Bindings (handcrafting a schema by building the JSON data manually) so we decided to omit it. We can certainly work around that or add that capability to the Swift bindings.

I guess a better use case for a sparse data set might be a realtime application that communicates via OTIO JSON and tries to keep it's packages small? I can see something like that in a virtual production environment.

timlehr avatar Aug 24 '23 18:08 timlehr

@jminor It would be great if we could at least have a list of registered fields (e.g. _registered_field_names) without having to have a value assigned. This way, one could still implement a default value system at their own risk, by tweaking my init code a little bit:

  def __init__(self, **fields):
      super(StudioOtioBaseSchema, self).__init__()

      # iterate over all provided field values and set them
      for field_name, field_value in fields.items():
          setattr(self, field_name, field_value)
          
      # init missing fields with None
      for field_name in self._registered_field_names:
          if field_name not in self._registered_fields:
               setattr(self, field_name, None)

timlehr avatar Aug 24 '23 18:08 timlehr

I like Tim's suggestion that the recent addition of versioning introduces an opportunity to revisit this behavior. Having to write default values is IMO surprising and a slight headwind when I'm writing new OTIO using code.

meshula avatar Aug 24 '23 19:08 meshula

One place we might look for inspiration/precedence in the python standard library is fields in dataclasses.

@timlehr can you look into how this works in the C++ a bit so we can better understand the implications of adding a default value?

reinecke avatar Aug 31 '23 17:08 reinecke