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

Added support for custom adapter hooks #1801

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

timlehr
Copy link
Contributor

@timlehr timlehr commented Oct 8, 2024

Summarize your change.

Rebased PR #1711

Required by OpenTimelineIO/otio-aaf-adapter#43

This adds support for attributing custom hooks to adapters and executing them with hook_function_argument_map being passed along through the adapter IO functions.

Describe the reason for the change.

I added two custom hooks to the OTIO AAF adapter (OpenTimelineIO/otio-aaf-adapter#43), allowing for embedding of media essence into the resulting AAF. This was needed to facilitate just-in-time DNXHR transcoding of the media for the AAF creation and adding a certain level of control and flexibility to the feature.

These features to the core are required in order to properly pass the hook argument map along to potential custom hooks run by the adapter. I tried to work with _FEATURE_MAP instead of creating a new version of the Adapter schema in order to minimize the impact of this change, while adding the necessary changes to facilitate custom hooks for adapters.

Reference associated tests.

tests/test_adapter_plugin.py
tests/test_hooks_plugins.py

@timlehr
Copy link
Contributor Author

timlehr commented Oct 8, 2024

@ssteinbach I finally had some time to revive #1711. Please have a look when you find some time. Thank you! 🙏

@timlehr timlehr force-pushed the tlehr/adapterHooks branch from 3a5ff33 to 23b28a6 Compare March 13, 2025 02:41
@codecov-commenter
Copy link

codecov-commenter commented Mar 13, 2025

Codecov Report

Attention: Patch coverage is 90.69767% with 4 lines in your changes missing coverage. Please review.

Project coverage is 84.81%. Comparing base (c0e97b0) to head (87f0c74).
Report is 41 commits behind head on main.

Files with missing lines Patch % Lines
...-opentimelineio/opentimelineio/adapters/adapter.py 73.33% 2 Missing and 2 partials ⚠️

❌ Your changes status has failed because you have indirect coverage changes. Learn more about Unexpected Coverage Changes and reasons for indirect coverage changes.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1801      +/-   ##
==========================================
+ Coverage   84.11%   84.81%   +0.69%     
==========================================
  Files         198      177      -21     
  Lines       22241    12779    -9462     
  Branches     4687     1186    -3501     
==========================================
- Hits        18709    10839    -7870     
+ Misses       2610     1760     -850     
+ Partials      922      180     -742     
Flag Coverage Δ
py-unittests 84.81% <90.69%> (+0.69%) ⬆️

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

Files with missing lines Coverage Δ
...sts/baselines/custom_adapter_hookscript_example.py 100.00% <100.00%> (ø)
tests/baselines/example.py 100.00% <100.00%> (ø)
tests/test_adapter_plugin.py 91.27% <100.00%> (+3.43%) ⬆️
tests/test_hooks_plugins.py 98.01% <100.00%> (+0.29%) ⬆️
...-opentimelineio/opentimelineio/adapters/adapter.py 78.18% <73.33%> (-10.82%) ⬇️

... and 126 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

This adds support for attributing custom hooks to adapters and executing them with `hook_function_argument_map` being passed along through the adapter IO functions.

Signed-off-by: Tim Lehr <tim.lehr@disneyanimation.com>
@timlehr timlehr force-pushed the tlehr/adapterHooks branch from 23b28a6 to 87f0c74 Compare March 13, 2025 02:45
@timlehr
Copy link
Contributor Author

timlehr commented Mar 13, 2025

@ssteinbach @jminor Finally rebased this again 👍

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.

The code looks good. This could use a note in the hook scripts documentation:

https://opentimelineio.readthedocs.io/en/latest/tutorials/write-a-hookscript.html

and adding it to the pluginfo script as well would be nice. I think all you need to do is add a "hooks" field to the plugin feature map:

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

Successfully merging this pull request may close these issues.

3 participants