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

Update the adapter.py to pass Argument Maps to Hooks #450

Merged
merged 5 commits into from Mar 12, 2019

Conversation

Projects
None yet
3 participants
@mikekoetter
Copy link
Contributor

mikekoetter commented Mar 6, 2019

This MR passes through the argument maps in the adapter.py read/write systems, passing them through to the hooks.

The existing #TODO items suggested that we perhaps should be doing this...
I can't find any reason not to.

An example use-case would be to create a pre-write adapter hook that checks the argument map for style being identified as nucoda and then preforms a path replacement on the reference url

def hook_function(in_timeline,argument_map=None):
    if argument_map.get('style') == 'nucoda':
        for in_clip in in_timeline.each_clip():
            ''' Change the Path to use windows drive letters ( Nucoda is not otherwise forward slash sensative ) '''
            if in_clip.media_reference:
                in_clip.media_reference.target_url = in_clip.media_reference.target_url.replace(r'/linux/media/path','S:')

mikekoetter added some commits Mar 6, 2019

test(hooks): Add test to run the hook through `otio.adapters`
Updates the manifest to unclude the post_media_linker hook, and asserts
that the extra data is being passed through the hook
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 6, 2019

Codecov Report

Merging #450 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #450      +/-   ##
==========================================
+ Coverage   87.15%   87.17%   +0.02%     
==========================================
  Files          67       67              
  Lines        6734     6794      +60     
==========================================
+ Hits         5869     5923      +54     
- Misses        865      871       +6
Impacted Files Coverage Δ
opentimelineio/adapters/adapter.py 94.36% <100%> (+0.52%) ⬆️
...neio_contrib/adapters/advanced_authoring_format.py 90.62% <0%> (-1.21%) ⬇️
...elineio_contrib/adapters/tests/test_aaf_adapter.py 98.08% <0%> (+0.16%) ⬆️

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 88089b1...4c6fc6a. Read the comment docs.

@ssteinbach ssteinbach added this to the Public Beta 10 milestone Mar 6, 2019

@ssteinbach

This comment has been minimized.

Copy link
Member

ssteinbach commented Mar 6, 2019

@mikekoetter Your example is great - if it isn't too much to ask, do you think you could add an "Example Hooks" or "Example Use Case" section to this doc:
https://opentimelineio.readthedocs.io/en/latest/tutorials/write-a-hookscript.html

(docs/tutorials/write-a-hookscript.md) at the bottom and include that snippet?

@ssteinbach

This comment has been minimized.

Copy link
Member

ssteinbach commented Mar 6, 2019

I think we had originally envisioned having a separate set of arguments for hook functions, but its clever to have them receive the adapter arguments as well. My only concern is that if you pass an argument that isn't defined by either the adapter or the hook function you'll get a TypeError, right?

@mikekoetter

This comment has been minimized.

Copy link
Contributor Author

mikekoetter commented Mar 7, 2019

That is a concern I had as well, however I think that is what happens now in the adapter functions.

If we try and pass style='nucoca' to anything other than the cmx_3600 adapter we will get a TypeError

>>> otio.adapters.write_to_string(mytimeline,style='nucoda')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python2.7/site-packages/OpenTimelineIO-0.10.0.dev1-py2.7.egg/opentimelineio/adapters/__init__.py", line 212, in write_to_string
    **adapter_argument_map
  File "/usr/lib/python2.7/site-packages/OpenTimelineIO-0.10.0.dev1-py2.7.egg/opentimelineio/adapters/adapter.py", line 223, in write_to_string
    **adapter_argument_map
  File "/usr/lib/python2.7/site-packages/OpenTimelineIO-0.10.0.dev1-py2.7.egg/opentimelineio/plugins/python_plugin.py", line 128, in _execute_function
    return (getattr(self.module(), func_name)(**kwargs))
TypeError: write_to_string() got an unexpected keyword argument 'style

--

For the hooks, it was a little unclear to me from the docs what the argument list should be.
So I followed the calling structure in hooks.py and the hook used in the test example.py

def hook_function(in_timeline,argument_map=None):

If we are counting on that as being the structure of a hook ( in which case the doc example needs updating ) then I would not expect to see a TypeError in the hooks section. Or rather it would be incumbent upon a hook's author to catch KeyErrors from the incoming argument_map dict

docs(hooks): Update initial example, add a use-case example
Update the hook_function defenition in the example to use the kwarg
`argument_map`, and add a use case example that uses said argument map
@ssteinbach

This comment has been minimized.

Copy link
Member

ssteinbach commented Mar 8, 2019

I see, so this works as long as the signature of a hook function is always:
(in_timeline, argument_map)

I've been chewing on this and I think that a slightly deeper (potentially breaking) fix might be better.

The adapters take arguments expanded out, but the media linkers take them packed into a dictionary. I feel like we should probably make that consistent...
Adapters:

**adapter_argument_map

Media Linkers:
media_linker_argument_map=media_linker_argument_map

It looks like the hook functions were modeled after the media linkers, with an argument dictionary instead of expanded arguments.

I think that is a design mistake on our part - it makes more sense to me that arguments would be passed to EVERYTHING the adapter way - expanded out. That way in the default case, the interface to the adapter is externally inspectable and python throws meaningful errors.

I actually think the better solve is to go in and fix both hooks and media linkers to take arguments in the adapter way.

Furthermore, I think each plugin (adapter, media linker, hooks) should get their own argument dictionaries. If you think its valuable context for hook functions to also get the arguments that were passed to adapters, I could totally see also providing a 'context' dictionary as an argument to the hook functions, but I think the best thing would be for us to change hook functions and media linkers to match the way that adapters work.

@ssteinbach

This comment has been minimized.

Copy link
Member

ssteinbach commented Mar 11, 2019

Ok, after thinking about it some more and looking through the code, I actually am backtracking on my previous comment. Because hook functions typically fire off as adapters run, any arguments would need to be passed to read_from_file etc directly, like media linker arguments currently are. I think the solve is to add a hook_function_argument_map to this:

And pass it through the same way the media linker arguments are being passed through:

result = hooks.run("post_adapter_read", result)

Your change is really close to this, in other words - if you add hook_function_argument_map to the function signature of the adapters read/write functions, then pass that into the hook function instead of the adapter argument map I think its what we want.

Sorry about the flip flop on this! Hopefully this feedback makes sense.

@ssteinbach

This comment has been minimized.

Copy link
Member

ssteinbach commented Mar 11, 2019

Oh and I think it could make sense to pass the adapter arguments in to the hook function in the way you note, but I would just wrap it in the dictionary:

hook_function_argument_map['adapter_arguments'] = adapter_args
hook_function_argument_map['media_linker_argument_map'] = media_linker_argument_map

Then we should also note that in the docs.

fix(hooks): Add hooks_argument_map to adapter signature
Add `hooks_argument_map` to the adapter signature and pass the adapter
and media linker maps through as values of the hooks map
@mikekoetter

This comment has been minimized.

Copy link
Contributor Author

mikekoetter commented Mar 11, 2019

ok, @ssteinbach I think I follow.

I took a swing at what I thought you were describing. let me know how close to the mark I got

@ssteinbach
Copy link
Member

ssteinbach left a comment

Yeah, this is looking good to me. Thanks!

@ssteinbach ssteinbach requested a review from jminor Mar 11, 2019

@ssteinbach ssteinbach merged commit a216cc4 into PixarAnimationStudios:master Mar 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.