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

Make kedro micropkg package use pyproject.toml #2761

Merged
merged 1 commit into from Jul 10, 2023

Conversation

astrojuanlu
Copy link
Member

Description

Fix gh-2414.

Initially, I tried to be smart and also rework how requirements.txt files are handled. But in the end I opted for this simple solution (just replacing setup.py with pyproject.toml) and it looks like it's simple and it works.

Thanks to gh-2614, not many code changes were required because essentially the pull code is now packaging-agnostic. The only part I had to touch was the on-the-fly generation of the file. Not even the tests needed to be touched.

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

Copy link
Contributor

@noklam noklam left a comment

Choose a reason for hiding this comment

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

I don't have many experience with micropkg before, I have few comments but not necessary related to this PR.

First, I expect kedro pipeline create has symmetry to kedro micropkg package. i.e. kedro pipeline create nok then kedro micropkg package nok should work, but it doesn't because I need to do kedro micropkg pipelines.nok which isn't trivial to find in the doc. I end up have to look at the e2e test. The documentation doesn't tell me I need to do this https://docs.kedro.org/en/latest/nodes_and_pipelines/micro_packaging.html

  1. Error message is not very helpful for --all, I am pretty lost when it talks about manifest and

(kedro_core) pattern main % kedro micropkg package
Please specify a micro-package name or add '--all' to package all micro-packages in the 'pyproject.toml' package manifest section.
(kedro_core) pattern main % kedro micropkg package --all
Nothing to package. Please update the 'pyproject.toml' package manifest section.

I also did a manual test to create micropkg and install it

kedro micropkg pipelines.nok
pip install dist/nok-0.1.tar.gz

This works fine but when I try to uninstall it I notice it is also in test folder which I didn't expect.

(kedro_core) pattern main % pip uninstall nok
Found existing installation: nok 0.1
Uninstalling nok-0.1:
Would remove:
/Users/Nok_Lam_Chan/miniconda3/envs/kedro_core/lib/python3.8/site-packages/nok-0.1.dist-info/*
/Users/Nok_Lam_Chan/miniconda3/envs/kedro_core/lib/python3.8/site-packages/nok/*
/Users/Nok_Lam_Chan/miniconda3/envs/kedro_core/lib/python3.8/site-packages/tests/* <- what is this?

@astrojuanlu
Copy link
Member Author

Thanks a lot @noklam !

First, I expect kedro pipeline create has symmetry to kedro micropkg package. i.e. kedro pipeline create nok then kedro micropkg package nok should work, but it doesn't because I need to do kedro micropkg pipelines.nok which isn't trivial to find in the doc.

This is good feedback, we should add that to #1536

Error message is not very helpful for --all, I am pretty lost when it talks about manifest and

This works on main as well right?

I also did a manual test to create micropkg and install it

Couldn't reproduce this 🤔 but I have an idea of how it could have happened.

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.

Nice and simple solution 👍

I think @noklam raised some very good points, which we need to address. If no tickets exist yet for these issues, let's add them to: https://github.com/kedro-org/kedro/milestone/21

@astrojuanlu
Copy link
Member Author

Spread these comments over gh-1536, gh-2123, gh-2766 👍🏽

@noklam
Copy link
Contributor

noklam commented Jul 5, 2023

Error message is not very helpful for --all, I am pretty lost when it talks about manifest and

This works on main as well right?

I test it with main, the behavior is the same so it is not introduced by this PR, it's just a minor documentation improvement suggestion.

@astrojuanlu astrojuanlu force-pushed the feat/micropkg-pyproject-toml branch from eb08378 to 7723112 Compare July 5, 2023 14:19
@SajidAlamQB
Copy link
Contributor

Will this still work for users who are still using setup.py in their existing Kedro projects? Do we need to make any considerations for them?

@astrojuanlu
Copy link
Member Author

Yes indeed - in fact, currently the templates still use setup.py (because #2280 isn't closed yet) and this still works.

That's for kedro micropkg package with older projects. For kedro micropkg pull that pulls older micropackages, it will work because after #2614 the machinery is packaging-agnostic. ✨

@astrojuanlu astrojuanlu force-pushed the feat/micropkg-pyproject-toml branch from 7723112 to 37526f2 Compare July 7, 2023 09:39
Fix gh-2414.

Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
@astrojuanlu astrojuanlu enabled auto-merge (squash) July 10, 2023 10:00
@astrojuanlu astrojuanlu merged commit 58167a8 into main Jul 10, 2023
34 checks passed
@astrojuanlu astrojuanlu deleted the feat/micropkg-pyproject-toml branch July 10, 2023 10:45
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.

Use pyproject.toml instead of setup.py for kedro micropkg
4 participants