Allow to construct AnyVector and AnyDictionary with values from Python #1574

JeanChristopheMorinPerso · 2023-04-07T15:30:15Z

Allow to construct AnyVector and AnyDictionary with values from Python.

Note that it only useful to OTIO developers who have to play with the Python bindings internals. For example I often have to create these objects when I work on the bindings, and it's quite annoying to have to do

import opentimelineio as otio
d = otio._otio.AnyDictionary()
d['a'] = 'a'

while I could simply do d = otio._otio.AnyDictionary({'a': 'a'}).

This was extracted from #1490.

…n. This is mainly meant to facilitate developing OTIO itself. Signed-off-by: Jean-Christophe Morin <[email protected]>

Signed-off-by: Jean-Christophe Morin <[email protected]>

codecov-commenter · 2023-04-07T15:58:41Z

Codecov Report

Merging #1574 (51d62b5) into main (6f4ed5e) will increase coverage by 0.01%.
The diff coverage is 81.57%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1574      +/-   ##
==========================================
+ Coverage   80.04%   80.06%   +0.01%     
==========================================
  Files         197      197              
  Lines       21720    21757      +37     
  Branches     4309     4316       +7     
==========================================
+ Hits        17386    17419      +33     
+ Misses       2182     2180       -2     
- Partials     2152     2158       +6

Flag	Coverage Δ
py-unittests	`80.06% <81.57%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...pentimelineio/opentimelineio-bindings/otio_utils.h	`62.85% <ø> (ø)`
...eio/opentimelineio-bindings/otio_anyDictionary.cpp	`38.88% <50.00%> (+5.55%)`	⬆️
...elineio/opentimelineio-bindings/otio_anyVector.cpp	`38.09% <50.00%> (+7.32%)`	⬆️
...ineio/opentimelineio-bindings/otio_anyDictionary.h	`78.43% <100.00%> (+0.43%)`	⬆️
...imelineio/opentimelineio-bindings/otio_anyVector.h	`86.79% <100.00%> (+0.25%)`	⬆️
...ntimelineio/opentimelineio-bindings/otio_utils.cpp	`54.73% <100.00%> (ø)`
tests/test_core_utils.py	`98.90% <100.00%> (+0.14%)`	⬆️

... and 2 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f4ed5e...51d62b5. Read the comment docs.

ssteinbach

This looks good to me. My only question is about copying and interacting with the reference system.. I wonder if there are any tests that could be added for that behavior.

ssteinbach · 2023-04-13T05:37:51Z

src/py-opentimelineio/opentimelineio-bindings/otio_anyVector.cpp

+        .def(py::init([](const py::iterable &it) {
+            any a;
+            py_to_any(it, &a);
+            AnyVector v = safely_cast_any_vector_any(a);
+            auto proxy = new AnyVectorProxy;
+            proxy->fetch_any_vector().swap(*v.get_or_create_mutation_stamp()->any_vector);
+            return proxy;
+        }))


is this copying the data?

If by data you mean everything (like a deep copy in Python), no. If you mean the top level list/vector, yes. See https://github.com/AcademySoftwareFoundation/OpenTimelineIO/pull/1574/files#r1165693980.

What should be the behavior?

ssteinbach · 2023-04-13T05:38:58Z

tests/test_core_utils.py

+                'list': [1, 2.5, 'asd'],
+                'dict': {'map1': [345]},
+                'AnyVector': v,
+                'AnyDictionary': d1,


related to the question about copies, can you edit d1 in place and have d2["AnyDictionary"] change?

Items removed or added in d1 in place won't be added/removed in d2, but values are shared. As far as I see, this is an existing behavior. For example, this passes:

def test_construct_with_values(self): # Contruct with values d1 = opentimelineio.core._core_utils.AnyDictionary( {'key1': 1234, 'key_2': {'asdasdasd': 5.6}} ) # Construct without values and only add a value after instantiation. d3 = opentimelineio.core._core_utils.AnyDictionary() d3['key4'] = 'value4' # Create an SO and add it to d1 and d3. so = opentimelineio._otio.SerializableObjectWithMetadata('initial name') d1['so'] = so d3['so'] = so d2 = opentimelineio.core._core_utils.AnyDictionary( { 'd1': d1, 'd3': d3, 'so': so } ) self.assertEqual(d2['d1'], d1) self.assertEqual(d2['d3'], d3) # Edit d1 in place d1['key3'] = 'value3' d1['so'].name = 'new name set on d1["so"]' # Confirm that d2['d1'] is not updated self.assertEqual(d2['d1'], {'key1': 1234, 'key_2': {'asdasdasd': 5.6}, 'so': so}) self.assertEqual(d1, {'key1': 1234, 'key_2': {'asdasdasd': 5.6}, 'key3': 'value3', 'so': so}) # But values that were already there are shared. # We previously updated the name of so by doing d1['so'].name = '...' # and d2['so'] got updated. self.assertEqual(d2['so'].name, 'new name set on d1["so"]') # Same behavior here. d3['key5'] = 'value5' d3['so'].name = 'new name set on d3["so"]' self.assertEqual(d2['d3'], {'key4': 'value4', 'so': so}) self.assertEqual(d3, {'key4': 'value4', 'key5': 'value5', 'so': so}) self.assertEqual(d2['so'].name, 'new name set on d3["so"]')

But just to be sure, I'll do some A/B testing with the main branch.

Same result on the main branch (with slight modification in the code example to not construct with values):

def test_construct_with_values(self): d1 = opentimelineio.core._core_utils.AnyDictionary() d1['key1'] = 1234 d1['key_2'] = {'asdasdasd': 5.6} # Construct without values and only add a value after instantiation. d3 = opentimelineio.core._core_utils.AnyDictionary() d3['key4'] = 'value4' # Create an SO and add it to d1 and d3. so = opentimelineio._otio.SerializableObjectWithMetadata('initial name') d1['so'] = so d3['so'] = so d2 = opentimelineio.core._core_utils.AnyDictionary() d2['d1'] = d1 d2['d3'] = d3 d2['so'] = so self.assertEqual(d2['d1'], d1) self.assertEqual(d2['d3'], d3) # Edit d1 in place d1['key3'] = 'value3' d1['so'].name = 'new name set on d1["so"]' # Confirm that d2['d1'] is not updated self.assertEqual(d2['d1'], {'key1': 1234, 'key_2': {'asdasdasd': 5.6}, 'so': so}) self.assertEqual(d1, {'key1': 1234, 'key_2': {'asdasdasd': 5.6}, 'key3': 'value3', 'so': so}) # But values that were already there are shared. # We previously updated the name of so by doing d1['so'].name = '...' # and d2['so'] got updated. self.assertEqual(d2['so'].name, 'new name set on d1["so"]') # Same behavior here. d3['key5'] = 'value5' d3['so'].name = 'new name set on d3["so"]' self.assertEqual(d2['d3'], {'key4': 'value4', 'so': so}) self.assertEqual(d3, {'key4': 'value4', 'key5': 'value5', 'so': so}) self.assertEqual(d2['so'].name, 'new name set on d3["so"]')

ssteinbach · 2023-04-13T05:44:34Z

C++ folks, @darbyjohnston, @davidbaraff, or @meshula -- can anyone else scan this? Thanks @JeanChristopheMorinPerso!

meshula · 2023-04-13T16:37:05Z

My question is whether or not this opens a path to creating malformed constructions? (By opens a path, I mean, can you do something now to build something malformed, that could not be built before?)

davidbaraff · 2023-04-13T16:55:55Z

An important invariant is not being able to build something in Python that causes C++ to crash. :) I’m fine with programming in C++ and doing illegal things in OTIO that cause a core dump. That’s the nature of C++.But the goal is that if you type something in in Python that can cause a core dump, it’s a bug on the implementors of the bridge (i.e., primary me) that should be fixed. The corollary is never to open up any new capabilities that permit this to happen. The bridging regarding dictionaries/vectors is extremely complicated because of things like modifying these structures during traversal, which could invalidate a C++ iterator. A lot of care was taken to get to where we are now (I don’t know that it’s perfect right now of course but I don’t know of any existing mechanisms that can cause a crash).So just saying that any new capabilities have to keep this front of mind. This stuff is NOT easy.Sent from my iPadOn Apr 13, 2023, at 9:37 AM, Nick Porcino ***@***.***> wrote: My question is whether or not this opens a path to creating malformed constructions? (By opens a path, I mean, can you do something now to build something malformed, that could not be built before?) —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

JeanChristopheMorinPerso · 2023-04-13T18:21:22Z

My question is whether or not this opens a path to creating malformed constructions? (By opens a path, I mean, can you do something now to build something malformed, that could not be built before?)

@meshula By that you mean will accept values/types that couldn't used before?

Basically, when we do d['asd'] = 'value' in python (where d is an AnyDictionary instance), it will call d.__setitem__, d.__setitem__ will use otio.core._core_utils._value_to_any to convert value into a PyAny which will then be use to do d.__internal_setitem__(key, PyAny_value). With the changes in this PR, the code path is almost the same when constructing with values. There is just an additional step which is that the Pybind11 class will call py_to_any_dictionary(value) to convert the value to an AnyDictionary. py_to_any_dictionary (on the C++ side) calls py_to_any (also C++), which roundtrips in Python to call otio.core._core_utils._value_to_any. The return value from that roundtrip is casted to an AnyDictionary. To me it's quite similar.

Also, I'm not aware of anyone actually using the AnyDictionary class in Python directly to construct metadata. As far as I know, in Python it's more an implementation detail in the sense that it behaves and looks like a normal dict to users. And users pass and use dicts with the Python API. For example, there is no easy way to instantiate AnyVector or AnyDictionary. They are only accessible from opentimelineio._otio or opentimelineio.core._core_utils, which are both "private".

Also, thanks @davidbaraff! There is one known crash with AnyDictionary, see #1474. But that's existing behavior. And there is also a known exception (at least to me):

>>> opentimelineio.core.SerializableObjectWithMetadata().metadata
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/jcmorin/jcmenv/aswf/OpenTimelineIO/build/lib.linux-x86_64-cpython-310/opentimelineio/core/_core_utils.py", line 127, in __repr__
    return repr(dict(self))
  File "/usr/lib/python3.10/_collections_abc.py", line 886, in __iter__
    yield from self._mapping
ValueError: Underlying C++ AnyDictionary has been destroyed

This particular wasn't reported by anyone, except me. I tend to poke around quite a bit with the OTIO internals and I sometimes find these gems. But that's all existing behaviour.

Allow to construct AnyVector and AnyDictionary with values from Pytho…

b1f4bc8

…n. This is mainly meant to facilitate developing OTIO itself. Signed-off-by: Jean-Christophe Morin <[email protected]>

JeanChristopheMorinPerso mentioned this pull request Apr 7, 2023

Improve typing of the metadata parameter #1490

Draft

Fix formatting

51d62b5

Signed-off-by: Jean-Christophe Morin <[email protected]>

ssteinbach reviewed Apr 13, 2023

View reviewed changes

ssteinbach requested a review from meshula April 13, 2023 05:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to construct AnyVector and AnyDictionary with values from Python #1574

Allow to construct AnyVector and AnyDictionary with values from Python #1574

JeanChristopheMorinPerso commented Apr 7, 2023

codecov-commenter commented Apr 7, 2023

ssteinbach left a comment

ssteinbach Apr 13, 2023

JeanChristopheMorinPerso Apr 13, 2023

ssteinbach Apr 13, 2023

JeanChristopheMorinPerso Apr 13, 2023

JeanChristopheMorinPerso Apr 13, 2023

JeanChristopheMorinPerso Apr 13, 2023

ssteinbach commented Apr 13, 2023

meshula commented Apr 13, 2023

davidbaraff commented Apr 13, 2023 via email

JeanChristopheMorinPerso commented Apr 13, 2023 •

edited

Loading

Allow to construct AnyVector and AnyDictionary with values from Python #1574

Are you sure you want to change the base?

Allow to construct AnyVector and AnyDictionary with values from Python #1574

Conversation

JeanChristopheMorinPerso commented Apr 7, 2023

codecov-commenter commented Apr 7, 2023

Codecov Report

ssteinbach left a comment

Choose a reason for hiding this comment

ssteinbach Apr 13, 2023

Choose a reason for hiding this comment

JeanChristopheMorinPerso Apr 13, 2023

Choose a reason for hiding this comment

ssteinbach Apr 13, 2023

Choose a reason for hiding this comment

JeanChristopheMorinPerso Apr 13, 2023

Choose a reason for hiding this comment

JeanChristopheMorinPerso Apr 13, 2023

Choose a reason for hiding this comment

JeanChristopheMorinPerso Apr 13, 2023

Choose a reason for hiding this comment

ssteinbach commented Apr 13, 2023

meshula commented Apr 13, 2023

davidbaraff commented Apr 13, 2023 via email

JeanChristopheMorinPerso commented Apr 13, 2023 • edited Loading

JeanChristopheMorinPerso commented Apr 13, 2023 •

edited

Loading