Here is a first attempt to move the AAF Adapter to separate git repo. This started out as a test but turned out to be pretty straightforward, since I already did most of the work with my AVB adapter.

The extracted AAF Adapter plugin is currently here https://github.com/OpenTimelineIO/otio-aaf-adapter I did some git magic to preserve the original commit history of the adapter files.

You can use the external adapter without applying this pull request. Unfortunately using the pkg_resources method doesn't seem to override the contrib plugin. The OTIO_PLUGIN_MANIFEST_PATH env variable is needed in that case.

The adapter also uses features that aren't in the 0.14 release, and doesn't work with a pypi wheel.

Jul 09 '22 21:07 markreidvfx

Codecov Report

Merging #1348 (2f3d0de) into extract_adapters (d1608c7) will decrease coverage by 1.11%. The diff coverage is n/a.

@@                 Coverage Diff                  @@
##           extract_adapters    #1348      +/-   ##
====================================================
- Coverage             79.35%   78.24%   -1.12%     
====================================================
  Files                   193      190       -3     
  Lines                 20617    18816    -1801     
  Branches               4152     3846     -306     
====================================================
- Hits                  16361    14722    -1639     
+ Misses                 2145     2031     -114     
+ Partials               2111     2063      -48     

Jul 09 '22 21:07 codecov-commenter

It's nice to see that it was easily separable! Thanks for getting the ball rolling.

Perhaps it would make sense to also add a line to Readme.md in the "Adapters" section, that tells where to find the relocated AAF adapter. When this is ready to land, we could update the Readme.md to point to a fork of your adapter work into the OpenTimelineIO org, as my working assumption is that this adapter will fall in the "officially supported by the OTIO project" umbrella.

Jul 10 '22 00:07 meshula

Good idea. The api docs for adapter aren't being generated anywhere yet either. I'm looking into next too.

Jul 10 '22 02:07 markreidvfx

It looks like the coverage decreases with this PR. For example https://app.codecov.io/gh/AcademySoftwareFoundation/OpenTimelineIO/compare/1348?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr%20comments&utm_term=None#D8L191 isn't covered anymore. https://app.codecov.io/gh/AcademySoftwareFoundation/OpenTimelineIO/compare/1348?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr%20comments&utm_term=None#D1L172 too.

When moving adapters, wi'll have to be carefull to keep the code coverage at a good level since some code seems to be only covered by adapter tests.

Jul 10 '22 02:07 JeanChristopheMorinPerso

Every decrease noted indicates a missing unit test on the C/C++ side of things. Some strategies are to (1) add unit tests as part of the change, (2) start an Issue that lists with checkboxes missing unit tests corresponding to items identified by the CI, and start a separate effort to improve test coverage, (3) leave test coverage for future us.

I'm personally inclined to (2), but would be interested in hearing other people's thoughts about it. The reason I'm inclined to (2) is that my preference would be that good unit tests are written, rather than letter of the law unit tests that simply satisfy a bot, and good unit tests could stall things indefinitely until someone has time to devote to it.

Jul 10 '22 19:07 meshula

It looks pretty straight forward to add a few tests to restore coverage. I will send a pull request.

Jul 11 '22 17:07 markreidvfx

This (and the forked repo you created) is really great! On the unit test front, I think we should emphasize good tests being written and minimize the burden on these kinds of PRs getting through, so somehow noting the changes for future and letting this get through feels like the best approach (another vote for (2) in the suggestions @meshula made). I think it probably should fall on us maintainers to own doing the work associated with that.

On the adapter precedence front - this is an issue I've run into as well. In my case I was able to work around it by giving the adapter a different name and using otio.adapters.from_name - but I don't think this is the right approach for this case. My instinct is that we should at least make the pkg_resources plugins take precedence. This does open the question of what happens if you have multiple pkg_resources plugins registered for the same extension. However I think that case will be a bit rarer, is currently an issue anyway, and can be managed by users when choosing which python packages to install.

Jul 13 '22 06:07 reinecke

I did add the unit tests that restore the coverage lost from the adapter removal. I was hoping to remove it from this pull and create separate pull request, but first wanted to demonstrate it fixes the issue.

Jul 13 '22 06:07 markreidvfx

I'm unsure why the EDL adapter has shown up in the Codecov report? I think its something to do with the recent EDL commits. The Codecov website can be pretty frustrating to use :p. By my account though, the added unit tests should restore the coverage lost from the adapters removal and even add a bit more.

Jul 13 '22 22:07 markreidvfx

I think codecov is just confused. Codecov is unfortunately quite easy to confuse.

Jul 13 '22 22:07 JeanChristopheMorinPerso

Tests are awesome :) Why don't we go ahead and PR/land them separately, per your suggestion? I think they're ready.

Jul 14 '22 22:07 meshula

Will do

Jul 15 '22 02:07 markreidvfx

The separated AAF Adapter is now located here https://github.com/OpenTimelineIO/otio-aaf-adapter

I created a task list of things that need to happen before this pull request could safely be applied https://github.com/OpenTimelineIO/otio-aaf-adapter/issues/1

Please let me know if there are any other blocking issues to add to this task list.

Aug 04 '22 22:08 markreidvfx

@markreidvfx could you aim this PR at the newly created extract_adapters branch? I think there should be an "Edit" button at the top of the PR page for this which will let you change the target branch.

Nov 30 '22 16:11 jminor

@jminor the pull has be change to target that branch. There were some conflicts which I've resolved. f7123d0bb1d1a621180f5606844919216ec0eeac made some changes to the aaf adapter, I'll port them to the adapter repo.

Dec 01 '22 20:12 markreidvfx

@markreidvfx, would you mind rebasing on top of or merging the latest changes in the "extract_adapters"? It might kick off the CI so we can see that everything is still good to go.

Dec 13 '22 23:12 apetrynet

@apetrynet rebased and all tests pass!

Dec 14 '22 00:12 markreidvfx

Thanks @markreidvfx!

Dec 14 '22 07:12 apetrynet