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

Improve types of childen, tracks, effects and markers parameters in function signatures #1448

Conversation

JeanChristopheMorinPerso
Copy link
Member

@JeanChristopheMorinPerso JeanChristopheMorinPerso commented Oct 7, 2022

Summarize your change.

Continuation of the work I've been doing to improve the parameter types in our bindings.

In this PR, I'm changing the types of the children, tracks, effects and markers parameters. A nice side effect is that we no more need the Python > C++ > Python > C++ > Python dance to convert the inputs from a py::object and AnyVectors.

Previously, there was an hand-coded bridging to AnyVector while now it's using facilities in pybind11 to accomplish the same thing. Part of the reason that's possible, is that py::object has been replaced in the interfaces with explicit types that pybind11 knows how to deal with.

Here is an example of why it's important to expose the correct types in constructors (and functions/methods):

The previous behavior was something like this:

>>> opentimelineio._otio.Track(name='asd', children=[1])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/jcmorin/jcmenv/aswf/OpenTimelineIO/.venv/lib/python3.10/site-packages/opentimelineio/core/_core_utils.py", line 126, in _value_to_so_vector
    raise TypeError(
TypeError: Expected list/sequence of SerializableObjects; found element of type '<class 'int'>'

In this example there is two things to note. First, the error doesn't tell us which argument is of wrong type. Second, it doesn't tell us which specific types should be in the list/sequence. From the code, the list/sequence passed to the children parameter is not all subclasses of SerializableObject. It only accepts a list/sequence of type Composable and its sublcasses.

Compare this to the new behavior:

>>> opentimelineio._otio.Track(name='asd', children=[1])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: __init__(): incompatible constructor arguments. The following argument types are supported:
    1. opentimelineio._otio.Track(name: object = '', children: Optional[List[opentimelineio._otio.Composable]] = None, source_range: Optional[opentimelineio._opentime.TimeRange] = None, kind: str = 'Video', metadata: object = None)

Invoked with: kwargs: name='asd', children=[1]

Now we can at least see all the arguments and the list of all accepted signatures. The signature is clear about that fact that it accepts a list of Composable and its subclasses. Pybind11 doesn't tell us which argument is wrong, but that could be contributed to Pybind11 if we feel like it would improve the experience.


There is one unanswered question though, at least for me, and it's what about the crashes mentioned in otio_utils.cpp? Andl also, what about Every element has to be a SerializableObject::Retainer<>? Was all this needed/casued by the fact that we were round tripping to Python?

Signed-off-by: Jean-Christophe Morin <jean_christophe_morin@hotmail.com>
@JeanChristopheMorinPerso
Copy link
Member Author

Rebased to fix conflicts.

@codecov-commenter
Copy link

codecov-commenter commented Oct 7, 2022

Codecov Report

Merging #1448 (8a8cea6) into main (5faf124) will increase coverage by 0.05%.
The diff coverage is 21.73%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1448      +/-   ##
==========================================
+ Coverage   78.85%   78.90%   +0.05%     
==========================================
  Files         203      202       -1     
  Lines       22246    22214      -32     
  Branches     4517     4502      -15     
==========================================
- Hits        17541    17529      -12     
+ Misses       2437     2430       -7     
+ Partials     2268     2255      -13     
Flag Coverage Δ
py-unittests 78.90% <21.73%> (+0.05%) ⬆️

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

Impacted Files Coverage Δ
...ntimelineio/opentimelineio-bindings/otio_utils.cpp 51.08% <ø> (+2.51%) ⬆️
...pentimelineio/opentimelineio-bindings/otio_utils.h 52.94% <ø> (+1.04%) ⬆️
.../py-opentimelineio/opentimelineio/core/__init__.py 83.33% <ø> (ø)
...-opentimelineio/opentimelineio/core/_core_utils.py 85.26% <ø> (+1.14%) ⬆️
...entimelineio-bindings/otio_serializableObjects.cpp 24.77% <19.04%> (+0.22%) ⬆️
...melineio/opentimelineio-bindings/otio_bindings.cpp 51.58% <50.00%> (ø)
src/opentimelineio/stringUtils.h

Continue to review full report at Codecov.

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

Signed-off-by: Jean-Christophe Morin <jean_christophe_morin@hotmail.com>
Signed-off-by: Jean-Christophe Morin <jean_christophe_morin@hotmail.com>
@ssteinbach ssteinbach merged commit 3e5d941 into AcademySoftwareFoundation:main Oct 12, 2022
@ssteinbach ssteinbach added this to the Public Beta 16 milestone Oct 12, 2022
@JeanChristopheMorinPerso JeanChristopheMorinPerso deleted the remove_py_to_vector branch October 13, 2022 01:00
MichaelPlug pushed a commit to MichaelPlug/OpenTimelineIO that referenced this pull request Aug 5, 2023
…unction signatures (AcademySoftwareFoundation#1448)

* remove py_to_vector

Signed-off-by: Jean-Christophe Morin <jean_christophe_morin@hotmail.com>
Signed-off-by: Michele Spina <michelespina96@gmail.com>
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.

None yet

4 participants