Skip to content

Import _opentime before actually creating the bindings for _otio #1396

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

Conversation

JeanChristopheMorinPerso
Copy link
Member

I noticed that doing python -c 'import opentimelineio._otio' was not working. It was resulting in an import error:

ImportError: arg(): could not convert default argument 'marked_range: opentime::v1_0::TimeRange' in method '<class 'opentimelineio._otio.Marker'>.__init__' into a Python object (type not registered yet?)

This error doesn't normally happen because we usually go through opentimelineio/__init__.py which imports _opentime before importing _otio.

After some searching, I found https://pybind11.readthedocs.io/en/stable/advanced/misc.html#partitioning-code-over-multiple-extension-modules which describes some options to fix this. This PR implements the simplest option out there, which is to import _opentime before actually creating the _otio bindings.

This should be harmless because it is going to be a no-op when importing OTIO normally (via import opentimelineio). It will be a no-op in this case bacause Python doesn't re-import a package if imported twice.

…s allows to import _otio without manually importing _opentime before.

Signed-off-by: Jean-Christophe Morin <jean_christophe_morin@hotmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Sep 17, 2022

Codecov Report

Merging #1396 (f6671b8) into main (8b430a6) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1396   +/-   ##
=======================================
  Coverage   86.12%   86.12%           
=======================================
  Files         199      199           
  Lines       20522    20523    +1     
  Branches     2375     2375           
=======================================
+ Hits        17674    17675    +1     
  Misses       2267     2267           
  Partials      581      581           
Flag Coverage Δ
py-unittests 86.12% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...melineio/opentimelineio-bindings/otio_bindings.cpp 98.29% <100.00%> (+0.01%) ⬆️

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 8b430a6...f6671b8. Read the comment docs.

Copy link
Collaborator

@meshula meshula left a comment

Choose a reason for hiding this comment

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

could you make a small grammatical tweak please?

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

@meshula meshula left a comment

Choose a reason for hiding this comment

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

looks good!

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.

nice catch. LGTM

@ssteinbach ssteinbach added this to the Public Beta 15 milestone Sep 18, 2022
@meshula meshula merged commit 4d136df into AcademySoftwareFoundation:main Sep 18, 2022
@JeanChristopheMorinPerso JeanChristopheMorinPerso deleted the import_opentime_before_binding_otio branch September 18, 2022 23:08
MichaelPlug pushed a commit to MichaelPlug/OpenTimelineIO that referenced this pull request Aug 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants