-
Notifications
You must be signed in to change notification settings - Fork 289
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
Schema Downgrading System #1387
Schema Downgrading System #1387
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1387 +/- ##
==========================================
- Coverage 86.27% 86.08% -0.20%
==========================================
Files 196 199 +3
Lines 19871 20252 +381
Branches 2309 2333 +24
==========================================
+ Hits 17144 17434 +290
- Misses 2161 2246 +85
- Partials 566 572 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
I'm not qualified to review the schema logic, just left a couple notes on the C++ code. From your list of questions:
Is it bad? Have you tried profiling it?
I like that idea. |
Yes, currently performance is about a 10x slowdown when downgrading. I have some leads on improving it though, so stay tuned for later in the week! |
Signed-off-by: ssteinbach <[email protected]>
Signed-off-by: ssteinbach <[email protected]>
Signed-off-by: ssteinbach <[email protected]>
Signed-off-by: ssteinbach <[email protected]>
Signed-off-by: ssteinbach <[email protected]>
- get_default, set_default, has_key Signed-off-by: ssteinbach <[email protected]>
Signed-off-by: ssteinbach <[email protected]>
c9e89d6
to
6685d41
Compare
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.
I like the approach! A few c++ comments here and there, mostly minor.
- cache the child cloning encoder across the entire serialization - const & sprinkled in - remove dead code
I added a performance scaling section to the pr text, just noting here in case folks following it are interested. |
* refactor versioning tests into their own class * DRY cleanup in the serializer before other stuff * DRY reduction in the json FILE serializer * Add io_perf_test to repo * Add a call w/ downgrade manifest to io_perf_test * add anydictionary convenience functions * add .cache to gitignore * add override tags * add perf tests for no-downgrade scenarios * autogen version info struct * add exceptions for overwriting up/downgrade fn * add exception when double registering a type * move schema version types into typeRegistry * add version manifest plugin * lint pass * comment formatting for RTD * add upgrade_downgrade_example in C++ * Add notes to environment variables markdown. * Improve error handling and text for env var errors - the OTIO_DEFAULT_TARGET_VERSION_FAMILY_LABEL has checking to make sure the format is correct and that the version/label requested are present. - Adds a custom exception that gets raised if there is a problem - Adds a unit test for testing this behavior * Performance tuning (cache the child cloning encoder across the entire serialization, use const & as much as possible) Signed-off-by: ssteinbach <[email protected]> Co-authored-by: meshula <[email protected]> Signed-off-by: Michele Spina <[email protected]>
Overview
As OpenTimelineIO makes its way into vendor tools, it is likely that multiple versions of OTIO will exist in different applications that would like to interoperate bidirectionally within a workflow. Therefore, we are adding a system that enables a newer version of the OpenTimelineIO library to write files with schemas that are compatible with older versions of the library.
See also: #1295.
C++
schema_version_map
, a mapping of schema names to desired schema versionsCORE_VERSION_MAP
, a compiled in constant mapping of "label" toschema_version_map
. Labels are currently associated with releases of the OpenTImelineIO library (ie. "0.15.0"). Intended to describe sets of schema versions that are compatible with specific releases of OpenTimelineIO.register_downgrade_function
to pair withregister_upgrade_function
. Like upgrade functions, downgrade functions take an AnyDictionary and operate on it in place.schema_version_targets
arguments toto_json_string
andto_json_file
family of functions/methods. These areoptional<schema_version_map>
and allow the user to optionally provide a set of schema version targets for downgrading during serializationCloningEncoder
to build anAnyDictionary
of the object, which is then passed through the downgrade functions and into the serializer.type_version_map
method toTypeRegistry
for querying the schema names and versions of all currently registered typesio_perf_test
andupgrade_downgrade_example
example C++ programsAnyDictionary
:get_default
,set_default
andhas_key
std::unordered_map
instead ofstd::map
inside the writer for a number of internal data structures where order is not relevantAnyDictionary
, likeRationalTime
, don't store them as concrete types but store them asAnyDictionary
. Otherwise they can't be downgraded.override
tagging to Encoder/Writer virtual methodsconst&
etc. 20mb OTIO serializes with no downgrading in 0.03s (was 0.04s).Python
downgrade_function_from
decorator, to echo theupgrade_function_for
function.version_manifest
field to the plugin manifest system, allowing you to define custom family/label/schema version sets to targetOTIO_DEFAULT_TARGET_VERSION_FAMILY_LABEL
environment variable for telling the python API that you want to use that as a default downgrade target.autogen_version_map
python script that generates theCORE_VERSION_MAP.cpp
file for the c++ apiPerformance
There is a performance cost to using the downgrading system. On a 20mb OTIO file where all clips are downgraded, the difference in serialization time was a slowdown of about 4x (0.03s -> 0.12s with a release build on an M1 Mac). Deserialization is not impacted at all.
As noted above, the baseline serialization performance was also improved, even when not downgrading.
Performance Scaling With File Size
Follow up issues
Further Design Discussions
enabled
flag). For downgrading, its true that the parser ignores extra keys, so we could leave enabled in objects and read them in old versions, however if someone goes in and edits the value ofenabled
the old APIs won't know what to do with them and it is possible to create data that would be interpreted differently in the old system (which neither models nor reads the .enabled flag and therefore assumes everything is enabled) and the newer system, which would read that as being enabled false.TypeRegistry
architecture