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

Hooks execution order is unclear when multiple hooks exists #2493

Closed
Tracked by #3014
noklam opened this issue Apr 4, 2023 · 8 comments
Closed
Tracked by #3014

Hooks execution order is unclear when multiple hooks exists #2493

noklam opened this issue Apr 4, 2023 · 8 comments

Comments

@noklam
Copy link
Contributor

noklam commented Apr 4, 2023

Description

Short description of the problem here.
A user report this in https://kedro-org.slack.com/archives/C03RKP2LW64/p1680617722865809?thread_ts=1680605927.073699&cid=C03RKP2LW64

In this case, the user's hook is interfering with kedro-telemetry, we use after_context_hook , catalog = context.catalog which collect some summary statistics about the project stats.

Context

How has this bug affected you? What were you trying to accomplish?
It is confusing as the execution order is not linear, it's difficult to reason & debug once you have more than one hook.

We don't encourage hooks to communicate with each other, but they can via storing state in self.xxx, if it's not managed properly they can interfere with each other and cause obscure bug.

If users need to have finer control the order of hooks, currently user can define it explicitly in settings.py, but this doesn't solve the problem completely

Expected Result

Tell us what should happen.
Normally you expected after_context_created hook is the earliest hook to be triggered.

Actual Result

Tell us what happens instead.
When you have more than one hooks. In this case, this is what happened

  1. telemetry's after_context_created triggered -> it create a catalog within the hook
  2. after_catalog_created is trigger for the 1st time
  3. User's custom hook after_context_created triggered
  4. after_catalog_created is trigger for the 2nd time.
-- If you received an error, place it here.
-- Separate them if you have more than one.

Your Environment

Include as many relevant details about the environment in which you experienced the bug:

  • Kedro version used (pip show kedro or kedro -V): 0.18.7
  • Python version used (python -V): 3.8.5
  • Operating system and version: MacOS
@merelcht
Copy link
Member

merelcht commented May 9, 2023

Do some testing to find out what's really going on here and decide if it can be fixed. If not, write it up in docs so at least users know it's expected behaviour.

@merelcht
Copy link
Member

See also #2492 (the same user who raised the issue on Slack)

@astrojuanlu
Copy link
Member

Potentially useful:

The call time order is LIFO relative to registration as mentioned in the docs.

pytest-dev/pluggy#250 (comment)

@noklam
Copy link
Contributor Author

noklam commented May 17, 2023

@astrojuanlu I believe that's true - HOOKS is also LIFO, however, it becomes less clear when the hook is a pip-installable one. i.e. kedro-telemetry.

@astrojuanlu
Copy link
Member

If I understand correctly, there are no guarantees about run order for plugin (entry points) hooks. That order can be influenced (but not forced) passing tryfirst=True or trylast=True to the HookimplMarker:

setuptools plugins (in arbitrary order, except those marked with try_first and try_last);

pytest-dev/pluggy#64 (comment)

(docs: https://pluggy.readthedocs.io/en/stable/index.html#call-time-order)

I don't think it's properly documented upstream, but we could document it ourselves.

@noklam
Copy link
Contributor Author

noklam commented May 31, 2023

https://kedro-org.slack.com/archives/C03RKP2LW64/p1685454486286959

Another question came up recently, maybe it's a strong enough signal that we should fix the docs. The thread also mention 2 issues that included some advance examples of creating stateful hooks. We should consider adding that too.

@noklam
Copy link
Contributor Author

noklam commented Dec 18, 2023

Maybe we can close this because kedro-org/kedro-plugins#422 is merged.

@astrojuanlu
Copy link
Member

Indeed: getindata/kedro-azureml#79

Closing! Hope we can tackle #1718 soon

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

No branches or pull requests

4 participants