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 documentation of hooks LIFO order #3013

Merged
merged 27 commits into from
Sep 18, 2023

Conversation

noklam
Copy link
Contributor

@noklam noklam commented Sep 6, 2023

NOTE: Kedro datasets are moving from kedro.extras.datasets to a separate kedro-datasets package in
kedro-plugins repository. Any changes to the dataset implementations
should be done by opening a pull request in that repository.

Description

fix #1940, the scope of this ticket is limited to the LIFO order, #1718 will be the ticket to summarise the order of kedro run

(Opening this PR to draft my idea and I will add the docs accordingly and update the title)
Few things I plan to do:

Development notes

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the RELEASE.md file
  • Added tests to cover my changes
  • Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

@noklam noklam mentioned this pull request Sep 6, 2023
6 tasks
@noklam
Copy link
Contributor Author

noklam commented Sep 6, 2023

https://docs.kedro.org/en/stable/extend_kedro/plugins.html#community-developed-plugins And while I am looking at this, we have awesome-kedro now, maybe it make sense to shuffle the order accordingly or at least move the more stable one to the top? (kedro-mlflow and GetInDatas)

cc @stichbury

@stichbury
Copy link
Contributor

stichbury commented Sep 7, 2023

https://docs.kedro.org/en/stable/extend_kedro/plugins.html#community-developed-plugins And while I am looking at this, we have awesome-kedro now, maybe it make sense to shuffle the order accordingly or at least move the more stable one to the top? (kedro-mlflow and GetInDatas)

cc @stichbury

That sounds OK to me, although it's hard to rank them by stability once we get past the most obvious. We did have a discussion a while ago that @idanov initiated about using a badge on individual plugin README pages to indicate when they were last updated. But anyway, let's remove the list from the docs and point instead to awesome-kedro where we list the plugins with GetInData starting the list and a rough order once we've included the most obviously stable.\

EDIT: See comment to the parent task, which should really be a response here

@noklam
Copy link
Contributor Author

noklam commented Sep 7, 2023

After some testing, I found out the LIFO order is quite tricky, and I don't recommend we document it as is.

The "registration order" is a combination of these factors:

  1. Project hooks
  2. hooks from entrypoints (which follow alphabetical order, and is a 2-level for loop

The execution order is the reverse of registration order (LIFO), except that user may use hook_impl "try_first" argument. Optionally, you can also import a registered entrypoints into project to change the order (i.e. importing kedro_viz hook in settings.py will force it to be registered earlier)

In general, order shouldn't be matter (99% of the time). If user need to control this, we should recommend using pluggy built-in functionality "try_first" argument. We shouldn't recommend to control the order via entrypoints order

@noklam noklam linked an issue Sep 7, 2023 that may be closed by this pull request
2 tasks
@noklam noklam marked this pull request as ready for review September 7, 2023 18:07
@stichbury stichbury changed the title Plan to update doumentations of hooks LIFO order Plan to update documentation of hooks LIFO order Sep 8, 2023
@@ -29,8 +29,15 @@ def to_json(metadata):
pipeline = pipelines["__default__"]
print(pipeline.to_json())
```
Starting from 0.18.14, Kedro switch to replace `setup.py` with `pyproject.toml`. The plugin need to provide the entry points in either file.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Starting from 0.18.14, Kedro switch to replace `setup.py` with `pyproject.toml`. The plugin need to provide the entry points in either file.
From version 0.18.14, Kedro replaced `setup.py` with `pyproject.toml`. The plugin needs to provide entry points in either file.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused by this -- can you add the entry point in either file? Should you, or shouldn't we make a recommendation not to use setup.py since we've replaced it with pyproject.toml?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stichbury We have moved to pyproject.toml if you created starter with the new version, but there are projects created with older version of Kedro and they only upgrade kedro but not the project template. Maybe its's too confusing and I should just keep the new one?

Copy link
Contributor

@stichbury stichbury left a comment

Choose a reason for hiding this comment

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

Few changes suggested and I'll make a commit direct onto hooks.md on your branch if that's OK as I want to make a few changes there.

@stichbury
Copy link
Contributor

@ankatiyar I know I've asked this before but @noklam has touched a few pages of markdown and there were some issues with his text but Vale didn't flag anything. I'm going to deliberately introduce an issue into hooks.md to see if it shouts...I'm not convinced it's running as it should be.

@noklam noklam requested a review from merelcht September 8, 2023 16:59
Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

Thanks for also updating the docs about plugins!

@@ -73,6 +73,7 @@ def _register_hooks_setuptools(
plugin_names = set()
disabled_plugin_names = set()
for plugin, dist in plugininfo:
print("DEBUG!!!", plugin, dist)
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing you only added this for debugging? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! 😅 I was working between different branches must have left this accidentally.

RELEASE.md Outdated Show resolved Hide resolved
Copy link
Contributor

@stichbury stichbury left a comment

Choose a reason for hiding this comment

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

LGTM, just a few changes which are mostly capitalisation of Hook 😆

@noklam noklam enabled auto-merge (squash) September 18, 2023 21:03
@noklam noklam merged commit 40ebad4 into main Sep 18, 2023
40 of 42 checks passed
@noklam noklam deleted the noklam/document-the-lifo-order-1940 branch September 18, 2023 21:33
To add the entry point to `pyproject.toml`, the plugin needs to provide the following `entry_points` configuration:
```toml
[project.entry-points."kedro.project_commands"]
kedrojson = kedrojson.plugin.commands
Copy link
Member

Choose a reason for hiding this comment

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

oops, typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did test it with the plugin hooks🥲 maybe I mess it up when I copy-paste it back to the doc. Thanks for spotting this.

)
```toml
[project.entry-points."kedro.starters"]
starter = plugin.starters
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be plugin:starters? (Same typo as above)

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.

Document the LIFO order in which hooks are executed in settings.py
5 participants