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

Initial port of aaf adapter from using pyaaf to pyaaf2 #396

Merged
merged 9 commits into from Dec 13, 2018

Conversation

Projects
None yet
5 participants
@freesonluxo
Copy link
Collaborator

freesonluxo commented Dec 4, 2018

This is an initial (and somewhat messy, but working!) port in advanced_authoring_formay.py of pyaaf to pyaaf2. All tests pass locally.

Otio 236 pyaaf2 port (#1)
* Fixing misspelling typo in docs (#385)

* WIP: Initial port of aaf from pyaaf to pyaaf2

* WIP: Got all tests except muted clip working

* WIP: Adding necessary helper file. All a WIP!

* WIP: Got all tests working

* WIP: Final cleanup to push to master

* WIP: check for none name

@jminor jminor requested review from jminor and stefanschulze Dec 4, 2018

@jminor

This comment has been minimized.

Copy link
Collaborator

jminor commented Dec 4, 2018

After this lands into OTIO, lets try to get the _walk_item function pushed upstream into pyaaf2.

@jminor
Copy link
Collaborator

jminor left a comment

If you rebase this onto the latest master, the merge conflicts should go away.

Also, to get the CI to run successfully, you will likely have to add a pip install pyaaf2 in the tox.ini somewhere. Look for "If we could get this from PyPI" in that file and you'll see the odd stuff we had to do to get the right pyaaf1 version installed. That should be much simpler with pyaaf2.

freesonluxo added some commits Dec 4, 2018

Otio 236 pyaaf2 port (#1)
* Fixing misspelling typo in docs (#385)

* WIP: Initial port of aaf from pyaaf to pyaaf2

* WIP: Got all tests except muted clip working

* WIP: Adding necessary helper file. All a WIP!

* WIP: Got all tests working

* WIP: Final cleanup to push to master

* WIP: check for none name
@ssteinbach
Copy link
Member

ssteinbach left a comment

Looks really good! Really excited for this change. I made some small notes.

Show resolved Hide resolved opentimelineio_contrib/adapters/advanced_authoring_format.py Outdated
Show resolved Hide resolved opentimelineio_contrib/adapters/advanced_authoring_format.py Outdated
Show resolved Hide resolved opentimelineio_contrib/adapters/advanced_authoring_format.py Outdated
Show resolved Hide resolved opentimelineio_contrib/adapters/advanced_authoring_format.py Outdated
Show resolved Hide resolved tox.ini Outdated

ssteinbach and others added some commits Dec 5, 2018

Update opentimelineio_contrib/adapters/advanced_authoring_format.py
Co-Authored-By: freesonluxo <45015871+freesonluxo@users.noreply.github.com>
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 5, 2018

Codecov Report

Merging #396 into master will decrease coverage by 10.13%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #396       +/-   ##
===========================================
- Coverage   87.05%   76.91%   -10.14%     
===========================================
  Files          63       63               
  Lines        5569     5580       +11     
===========================================
- Hits         4848     4292      -556     
- Misses        721     1288      +567
Impacted Files Coverage Δ
...neio_contrib/adapters/advanced_authoring_format.py 0% <0%> (-92.07%) ⬇️
...elineio_contrib/adapters/tests/test_aaf_adapter.py 14.64% <0%> (-82.5%) ⬇️

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 cc762bc...187ae9d. Read the comment docs.

@jminor

This comment has been minimized.

Copy link
Collaborator

jminor commented Dec 5, 2018

Something isn't quite right about the pyaaf2 install. The AAF tests are being skipped - which is why the coverage dropped so much. If you click through to the travis logs, you'll see a bunch of this:
test_aaf_flatten_tracks (test_aaf_adapter.AAFAdapterTest) ... skipped 'AAF module not found. You might need to set OTIO_AAF_PYTHON_LIB'

@jminor

This comment has been minimized.

Copy link
Collaborator

jminor commented Dec 5, 2018

Actually, it is just one mention of the old pyaaf in the test itself. See line 89 here:
https://github.com/PixarAnimationStudios/OpenTimelineIO/blob/master/opentimelineio_contrib/adapters/tests/test_aaf_adapter.py#L89

@jminor

jminor approved these changes Dec 5, 2018

import opentimelineio as otio

lib_path = os.environ.get("OTIO_AAF_PYTHON_LIB")
if lib_path and lib_path not in sys.path:
sys.path += [lib_path]
sys.path.insert(0, lib_path)

This comment has been minimized.

@swallitsch

swallitsch Dec 6, 2018

Contributor

If we're moving to pyaaf2 with pure python, then we should put the pip library in the setup.py, and trust that it will be installed. We should no longer need lines ~35-39 at all.


commands =
git clone https://github.com/markreidvfx/pyaaf2

This comment has been minimized.

@swallitsch

swallitsch Dec 6, 2018

Contributor

Unless we're trying to get a specific branch here, we should rely on the new line 26 below to pull pyaaf2 from pypi. Right now this git clone line will float as new commits come in on master, which is probably not desirable for testing.

This comment has been minimized.

@jminor

jminor Dec 6, 2018

Collaborator

At the moment we need one commit in pyaaf2 that isn't in a release yet: markreidvfx/pyaaf2@2bd48a9

Also, we're proposing a small change to pyaaf2 here: markreidvfx/pyaaf2#21

Once those land into a release of pyaaf2, we can switch to just adding pyaaf2 as a normal dependency. We might still keep it optional, since many people won't need AAF support at all.

This comment has been minimized.

@jminor

jminor Dec 6, 2018

Collaborator

Our intent is to not make a release of OTIO until that stuff is smoothed out.

This comment has been minimized.

@swallitsch

swallitsch Dec 6, 2018

Contributor

Sounds good!

@@ -85,8 +85,8 @@
try:
lib_path = os.environ.get("OTIO_AAF_PYTHON_LIB")
if lib_path and lib_path not in sys.path:
sys.path += [lib_path]
import aaf # noqa
sys.path.insert(0, lib_path)

This comment has been minimized.

@swallitsch

swallitsch Dec 6, 2018

Contributor

Same comment as above, trust that pyaaf2 has been installed as part of setup.py and we can skip all of this.

@ssteinbach ssteinbach added this to the Public Beta 10 milestone Dec 6, 2018

@jminor

jminor approved these changes Dec 13, 2018

@jminor jminor merged commit 74d45be into PixarAnimationStudios:master Dec 13, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jminor jminor referenced this pull request Dec 14, 2018

Closed

Switch from pyaaf to pyaaf2 #383

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment