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

Load entrypoint plugins before builtin and contrib. #1389

Merged

Conversation

markreidvfx
Copy link
Contributor

@markreidvfx markreidvfx commented Aug 29, 2022

This pull request is meant to assist the moving contrib adapters to external repos.
You can use $OTIO_PLUGIN_MANIFEST_PATH to override a contrib adapter, however this can be inflexible and difficult to test your adapter when you install it.
Being able to use a stock install of OTIO also means you don't have to manage pull requests for 2 repos at the same time when starting to move a contrib adapter.

This issue has been mentioned in #1348

@codecov-commenter
Copy link

codecov-commenter commented Aug 29, 2022

Codecov Report

Merging #1389 (7e513ec) into main (bb49bed) will increase coverage by 0.00%.
The diff coverage is 79.16%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1389   +/-   ##
=======================================
  Coverage   86.27%   86.27%           
=======================================
  Files         196      196           
  Lines       19871    19883   +12     
  Branches     2309     2312    +3     
=======================================
+ Hits        17144    17155   +11     
  Misses       2161     2161           
- Partials      566      567    +1     
Flag Coverage Δ
py-unittests 86.27% <79.16%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...-opentimelineio/opentimelineio/plugins/manifest.py 86.56% <66.66%> (ø)
tests/test_plugin_detection.py 82.95% <91.66%> (+1.37%) ⬆️

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 bb49bed...7e513ec. Read the comment docs.

@markreidvfx markreidvfx changed the title Load pkg_resources plugins before builtin and contrib. Load entrypoint plugins before builtin and contrib. Aug 29, 2022
Copy link
Collaborator

@reinecke reinecke 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! One question, is there a way we could add a unittest to check the precedence? This file seems to have a lot of tests around this sort of thing.

If the unittesting gets a bit too difficult or hacky to implement, I think we can just call it a CBB since we already weren't testing it.

@markreidvfx
Copy link
Contributor Author

I'll take a look, I think I can use part of those existing tests and come up with something that isn't too hacky.

Allows for overriding builtin plugins via entrypoint method.

Signed-off-by: Mark Reid <[email protected]>
@markreidvfx
Copy link
Contributor Author

I added a unit test, please take a look when you have the time

@jminor
Copy link
Collaborator

jminor commented Sep 9, 2022

@ssteinbach does this conflict with the downgrader PR at all? Do you care if this lands before/after that one?

Copy link
Collaborator

@reinecke reinecke left a comment

Choose a reason for hiding this comment

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

Great! Thanks for adding the test!

@ssteinbach
Copy link
Collaborator

@ssteinbach does this conflict with the downgrader PR at all? Do you care if this lands before/after that one?

Not an issue. The only file in common is in manifest.py and it isn't a conflicting change. Go for it!

@ssteinbach
Copy link
Collaborator

@jminor do you want to land this before or after the next release? (IE do you want to land it at the beginning of the next cycle or at the end of this one)

@markreidvfx
Copy link
Contributor Author

It would be especially helpful for people working on extracting contrib adapters if it was in the next wheel release. 🙂 It would allow running unit test against a wheel, at least for me, instead of having to compile OTIO everytime i run CI. saves a lot time/cycles.

@jminor jminor merged commit dcf3033 into AcademySoftwareFoundation:main Sep 12, 2022
@ssteinbach ssteinbach added this to the Public Beta 15 milestone Sep 12, 2022
MichaelPlug pushed a commit to MichaelPlug/OpenTimelineIO that referenced this pull request Aug 5, 2023
…areFoundation#1389)

* Load entrypoint plugins before builtin and contrib in order to allow overriding builtin plugins.

Signed-off-by: Mark Reid <[email protected]>
Signed-off-by: Michele Spina <[email protected]>
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.

6 participants