-
Notifications
You must be signed in to change notification settings - Fork 49
Pre and post loader hooks #1222
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
base: develop
Are you sure you want to change the base?
Pre and post loader hooks #1222
Conversation
Added functions to register/deregister them.
Task linked: OP-6870 Plugin hooks - Loader and Scene Inventory |
I wonder if new hook plug-ins here make more sense than just an |
Had the same question, but they should be blocking, I dont think you can achieve that with events. |
It is assumed that utils could be used in some kind 'load API', `actions` are tightly bound to UI loading.
The Event system in AYON is blocking by default, right @iLLiCiTiT ? |
Co-authored-by: Jakub Trllo <[email protected]>
Co-authored-by: Jakub Trllo <[email protected]>
Co-authored-by: Jakub Trllo <[email protected]>
@@ -460,7 +520,7 @@ def remove_container(container): | |||
return Loader().remove(container) | |||
|
|||
|
|||
def update_container(container, version=-1): | |||
def update_container(container, version=-1, hooks_by_identifier=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the behavior here is wrong.
It's odd that you can pass this argument and "override" the hooks behavior.. but what's worse is that it doesn't default to using the registered hooks. The API should also adhere to all the registered hooks.
So I'd say:
- It should not take this as an argument for someone to populate manually and allow overriding it.
- But at the very least, it should default to including the registered hooks if not overridden by the user in the API call.
Also, note that current code would fail if called with default arguments, because None..get(loader_identifier, {})
would not work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think its a responsibility of 'update_container' to find hooks by itself, and it would be terrible slow. Passing in this argument is made from controller
where it is cached.
hooks_by_identifier
wouldnt be None in current implementation, it would be always {}
in worst case. But I agree this might be confusing for new occurences of update_container
call. I could change default type to {}
, but isn't that not recommended, to pass []
or {}
as keyword arguments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant more - that if I have code calling update_container
I would expect any custom events or hooks to also trigger. It should happen in all cases, so that it's consistent. If the behavior is slow otherwise, we may need to rethink how these custom hooks are implemented instead. It would e.g. run absolutely fine if these were registered with an event system instead so they are registered once, instead of dynamically discovered each time.
E.g.
- Maya look assigner uses
load_container
- Maya Load Layout uses
load_container
- Maya Setdress uses
load_container
- OpenRV startup logic allows the use of
load_container
- Ayon core inventory plug-in "Remove and Load" action uses
load_container
- Ayon core build workfile uses
load_container
Each of these now must be adjusted to correctly run the hooks.
I'd say that each of these should all behave the same, out of the box.
@@ -264,6 +266,43 @@ class ProductLoaderPlugin(LoaderPlugin): | |||
""" | |||
|
|||
|
|||
class PreLoaderHookPlugin: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it'd make more sense to make a single PrePostLoaderHookPlugin
instead.
That way, one could make a Plugin that could act like a context manager by storing attributes on the plugin itself, e.g.
def pre_process(self):
self.handle = do_whatever()
def post_process(self):
cleanup(self.handle)
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, how should the hooks behave if the loading failed? Should post hooks still trigger, or should those not?
Also, what should happen if a pre hook errors, should it continue loading, or should it stop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats probably question of who is making pre/post hook. If they want to loading to fail/be skipped, they need to raise an exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree with the error handling that it is up to the developer . Loader itself should be mostly decoupled from the logic in per-load hook (otherwiser you could put everytthing to the loader). Bur the context manager idea is interesting, maybe something for the future when we rewrite the loading API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we will really do this, then I agree with @BigRoy, we should have one hook being able to handle pre and post (less classes, less functions). And I think it should have is_compatible
method instead of loader_identifiers
and avoid the mapping by loader identifiers (which makes it even more complicated).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be also removed from pipeline.__init__
to make it "less visible" as this should be removed/made obsolete with impeding Loader API.
I wanted to ask very similar questions to @BigRoy 's but I wanted to wait for this first #1222 (comment) . We really need to know what is the use-case. It's all related to current crap api. Loader plugins are also "actions" (delivery etc.), so I guess the pre/post hook needs to know "some" context, but because current api does not actually tell you anything about "if", "where" or "what" actually happened it's just a random classes with functions that do get triggered in various occations that probably won't be important for the hooks. |
Summoning @antirotor to describe more details about #928 |
…der-and-Scene-Inventory' into feature/AY-2218_Plugin-hooks-Loader-and-Scene-Inventory
…ture/AY-2218_Plugin-hooks-Loader-and-Scene-Inventory
Clients are changing some behaviour of some loaders. This should help them stay "vanilla" and still add some custom behaviour. For example - you load layout into your scene and you want to immediately lock all transforms so artist cannot accidentally move stuff around. All logic for the loader is the same, you just want to execute few lines of code after the load is complete. There are also often customization about hierarchy where loaded stuff should end up - if you want to customize this - to add loaded model into very specific structure, again - the whole loader logic is the same, you just need to move loaded items somewhere. |
So how will these changes help if you don't know what happened in |
Also important note is that this PR is adding 2 new classes and at about 10 new functions just to register and discover them, changing api functions for load. All of that will make backwards compatibility even harder when we begin with new load api. |
@@ -264,6 +266,43 @@ class ProductLoaderPlugin(LoaderPlugin): | |||
""" | |||
|
|||
|
|||
class PreLoaderHookPlugin: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree with the error handling that it is up to the developer . Loader itself should be mostly decoupled from the logic in per-load hook (otherwiser you could put everytthing to the loader). Bur the context manager idea is interesting, maybe something for the future when we rewrite the loading API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I vote for using type hints in methods arguments and return values - since annotations from __future__
is imported. If you use mypy later on, it will really help.
"""Plugin that should be run before any Loaders in 'loaders' | ||
|
||
Should be used as non-invasive method to enrich core loading process. | ||
Any external studio might want to modify loaded data before or after |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any external studio might want to modify loaded data before or after | |
Any studio might want to modify loaded data before or after |
I'd say almost everyone is "external studio" as there is no internal one 😜
tuple( | ||
list[ProductLoaderPlugin], | ||
list[LoaderPlugin], | ||
): Discovered loader plugins. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this changed?
Changelog Description
This PR introduces new concept of
Pre/Post Loader Hooks
which could be used by to enhance or modify loading processes without need of modifying official loaders.The idea is for custom addons to be able to implement their own
PreLoadHookPlugin | PostLoadHookPlugin
which would be triggered before/after Loader action.Additional info
Each addon must register its folder with pre/post hooks like this:
I am testing it with this dummy custom addon
my_custom_addon-1.0.0.zip
Testing notes: