Skip to content
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

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

JeanChristopheMorinPerso
Copy link
Member

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
Copy link

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

Impacted file tree graph

@@            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.

Copy link
Collaborator

@ssteinbach ssteinbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +22 to +29
.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;
}))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this copying the data?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"]')

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ssteinbach requested a review from meshula April 13, 2023 05:43
@ssteinbach
Copy link
Collaborator

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

@meshula
Copy link
Collaborator

meshula commented Apr 13, 2023

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
Copy link
Contributor

davidbaraff commented Apr 13, 2023 via email

@JeanChristopheMorinPerso
Copy link
Member Author

JeanChristopheMorinPerso commented Apr 13, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants