Bindings: UnknownSchema added data property #1799
Conversation
|
|
Signed-off-by: Natchar Ratanasirigulchai <[email protected]>
natchar
force-pushed
the
otio_unknownSchema_data
branch
from
cabe1df
to
a8974b3
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1799 +/- ##
==========================================
- Coverage 84.11% 81.61% -2.51%
==========================================
Files 198 176 -22
Lines 22241 12344 -9897
Branches 4687 3024 -1663
==========================================
- Hits 18709 10075 -8634
+ Misses 2610 1732 -878
+ Partials 922 537 -385
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 66 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
|
This looks good to me. Once this lands, then this feature in Raven should be easy to implement: OpenTimelineIO/raven#26 @ssteinbach do you have any concerns with this? |
| @@ -31,6 +31,11 @@ class UnknownSchema : public SerializableObject | |||
| return _original_schema_version; | |||
| } | |||
|
|
|||
| AnyDictionary& data() noexcept | |||
There was a problem hiding this comment.
is the intention to provide mutable access? Reading the original issue it talks about accessing dictionaries presumably for reading. A reference opens the door to lifetime issues where the reference to the AnyDictionary can outlive the UnknownSchema object.
I think it would be better to return a copy of the AnyDictionary, even though that incurs overhead, and if we want mutability provide a method to supply a new dictionary.
Alternatively, we could change _data to be a shared pointer, but treating the AnyDictionary as a value type would give safer concurrency, if we also added a mutex during the copy and replace.
We don't have much consideration for concurrency in otio at the moment, but it would make sense to me to think about it moving forward.
Does anyone (@jminor ?) have a sense of whether these dictionaries might tend to be massive (dozens or hundreds of entries), or are they more on the order of a few entries?
If the former, I think I'd bias to a shared_ptr solution and ignore concurrency inside the object; if the latter, return/set by value pattern with a mutex in anticipation of concurrency.
There was a problem hiding this comment.
@meshula Would a const & do the trick? Or you want a copy? And if its a copy should we call it data_copy() or something to disambiguate that you're not getting direct access to the child dictionary?
There was a problem hiding this comment.
Regarding size: a typical OTIO object should be small, with possibly dozens of JSON key/values in metadata. However, given that this class, UnknownSchema, is meant for user defined schema types, they really could be anything. This is the place where we're encouraging developers to add interesting new stuff, so we should be open-ended about what might be in there.
That leads me to support your suggestion that the API be defensive about UnknownSchema by providing read-only access. That will address the desired workflow (making UnknownSchema introspectable) without opening the door to meddling with the contents.
There was a problem hiding this comment.
Cool :) @ssteinbach const& doesn't protect against object lifetime issues, or concurrent modification, which I'm worried about. It would work if we had a notion of a holder on the container that also blocked writing while the container is held, but we haven't got that :]
There was a problem hiding this comment.
I don't think there's a way to modify the contents of an UnknownSchema object via our API, except via read_from which fully replaces the contents, so I suspect concurrency is not an issue. If a developer wants to modify these things, then they can register a schema and get a real object instead of UnknownSchema.
Would lifetime issues be handled by returning a copy as suggested earlier in this thread?
There was a problem hiding this comment.
Yes, a copy solves lifetime issues :)
There was a problem hiding this comment.
Ok, cool. @natchar could you alter data() to return a copy of _data?
There was a problem hiding this comment.
If it is returning a copy of data, perhaps instead of it being a property, it should be a function definition so that the call is unknownSchema.data().
Maybe I'm missing something, but if I do change the function signature to AnyDictionary data() const noexcept that should return a copy of _data. However, in the python bindings we use AnyDictionaryProxy for AnyDictionary and running the test will yield Underlying C++ AnyDictionary has been destroyed. Is there another change I need to make to AnyDictionaryProxy?
There was a problem hiding this comment.
It's reporting that the copy has been destroyed, right? So I guess we need to look at the binding, and make sure that's it's doing what we expect, and then remove the warning... Any other thoughts?
Fixes #1234
Python bindings return dictionary-type python object for UnknownSchema data.