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

[KED-3023] Enable plugins to extend starter-aliases default list #1592

Merged
merged 87 commits into from
Jun 29, 2022

Conversation

noklam
Copy link
Contributor

@noklam noklam commented Jun 6, 2022

Signed-off-by: noklam [email protected]

Description

Taking over the original PR from #1265, would fix #1040

Development notes

  • modified the _STARTERS_ALIASES to make it a dict with the format discussed in above issue.
  • created a kedro.starter entrypoint which can is iterated over to update the _STARTERS_ALIASES constant.
  • updated the kedro new and kedro starter list commands to use the updated _STARTERS_ALIASES constant (with new aliases discovered in the entrypoints).
  • Refactor with KedroStarterSpec, the location is temp, it probably doesn't belong kedro.framework.cli.starters

The code can be run with

conda create -n fps python=3.8 -y
conda activate fps
pip install -e .  # Assume have the latest commit 
pip install git+https://github.com/noklam/fake-plugin-starter.git
kedro starter list

Todo

  • Unit test + e2etest?
  • Documentions

Screenshots

image

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

@noklam noklam requested a review from idanov as a code owner June 6, 2022 14:10
@noklam noklam marked this pull request as draft June 6, 2022 14:11
@noklam noklam removed the request for review from idanov June 6, 2022 21:46
@noklam
Copy link
Contributor Author

noklam commented Jun 8, 2022

It's in a good shape now and the PR should be functioning as it intended. Big thanks to @Galileo-Galilei to draft this PR initially and make it very easy for me to pick up.

I plan to do a bit more refactoring, so this 56ee39c will be my checkpoint in case I fail to :).

@noklam
Copy link
Contributor Author

noklam commented Jun 8, 2022

I am quite happy with the KedroStarterSpec class, I have been able to remove ~100 lines of code which come from the validation logic, some tests are removed too since it is impossible to have fields that are not defined.

@noklam noklam marked this pull request as ready for review June 8, 2022 16:08
@noklam noklam requested a review from antonymilne June 8, 2022 16:08
@noklam
Copy link
Contributor Author

noklam commented Jun 20, 2022

The Window e2etest is failing with a very subtle error. When test_plugin is installed and run with ParallelRunner, the first few lines of log disappear in info.log but stdout did capture them. I doubt this is a bug somewhere in ParallelRunner but not related to this PR particularly.

@noklam
Copy link
Contributor Author

noklam commented Jun 22, 2022

The CI was failing because of a bug, once we have the fix the PR should pass again.

The bug was caused by the spawn process (Windows or MacOS >=3.8), because it will re-import everything.

Thanks @AntonyMilneQB for getting to the bottom of this!
The story roughly goes like this:

  1. so basically the import from kedro.framework.cli.starters imports kedro.framework.cli en route
  2. in __init__.py, it does from .cli import main
  3. when cli is imported, it runs this: import kedro.config.default_logger, which reset the logger to the framework defaults.

@noklam noklam merged commit 2ce618c into main Jun 29, 2022
@noklam noklam deleted the feat/enable-plugin-starter-alias branch June 29, 2022 22:41
@noklam
Copy link
Contributor Author

noklam commented Jun 29, 2022

@AntonyMilneQB Thanks a lot for your reviews!

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.

[KED-3023] Enable plugins to extend starter-aliases default list
4 participants