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-2970] Improve micropackaging CLI experience #1224

Merged
merged 41 commits into from
Feb 17, 2022

Conversation

SajidAlamQB
Copy link
Contributor

@SajidAlamQB SajidAlamQB commented Feb 8, 2022

Description

Current problems: * The kedro pipeline command group from 0.18.0 will also package things that are not pipelines and this current naming will lead users to think the kedro pipeline package and kedro pipeline pull commands only work on pipelines

The kedro pipeline command group from 0.18.0 also has commands that don't work on things that are not pipeline modules e.g. kedro pipeline delete only works on pipelines

Solution: * Create a new command group called kedro micropkg to capture being able to package and pull things that are not pipelines.

@yetudada agreed to go with kedro micropkg as the new command group on the cli. kedro micropkg will live alongside kedro pipeline pull/package for 0.17.7 with deprecation warnings but will be removed in 0.18.0.

related to: #1194

https://jira.quantumblack.com/browse/KED-2970

Development notes

Documentation and refactoring code will be done in a separate PR.

Imported several methods from pipeline.py to micropkg.py including _install_files, _generate_wheel_file etc..

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

@SajidAlamQB SajidAlamQB marked this pull request as ready for review February 9, 2022 15:03
Copy link
Contributor

@lorenabalan lorenabalan left a comment

Choose a reason for hiding this comment

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

LGTM, happy to approve when all the tests builds pass. Don't forget to add this in the release notes, as it directly affects the user. We need to also update documentation to make sure this cli group is included and that our tutorials refer to this one instead of pipeline. But I think that should be a separate PR, to make it easier to review. You can make it branch off this branch, or off main directly if you prefer.

I really appreciate that you copy-pasted only and did minimal changes to make it work for the new name - thus not mixing refactoring and copied changes in one go, which would make it hard to review. In a separate PR we could refactor some of those variable names and functions so that they don't talk about pipelines but more generally about packages, micropackages etc. But maybe we can do that directly on develop and we live with it on main. This is not a huge deal, but it does keep our code base in a healthy state if we do it.

tests/framework/cli/micropkg/conftest.py Outdated Show resolved Hide resolved
tests/framework/cli/micropkg/test_micropkg_pull.py Outdated Show resolved Hide resolved
@merelcht
Copy link
Member

I agree with Lorena that refactoring can be done later, but anything that's user facing (like the help messages on the CLI commands) should be updated. I personally got a bit confused reading the code, because it still refers to pipeline in a lot of places.

kedro/framework/cli/micropkg.py Outdated Show resolved Hide resolved
Copy link
Member

@idanov idanov left a comment

Choose a reason for hiding this comment

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

Awesome work 🎉 In full agreement with @lorenabalan and @MerelTheisenQB 's suggestions. As for the refactoring, I think it should happen straight after this lands in develop after the sync and it should be done in the same PR where you remove pipeline package and pipeline pull and their tests as part of this issue.

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.

Great work @SajidAlamQB 👏 ⭐

Signed-off-by: SajidAlamQB <90610031+SajidAlamQB@users.noreply.github.com>
Signed-off-by: SajidAlamQB <90610031+SajidAlamQB@users.noreply.github.com>
Signed-off-by: SajidAlamQB <90610031+SajidAlamQB@users.noreply.github.com>
Copy link
Contributor

@lorenabalan lorenabalan left a comment

Choose a reason for hiding this comment

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

Almost there - please delete test_micropkg.py first. I would also hold off merging until #1235 is merged and the one raised after it is merged as well (because there's quite a few commits that haven't made it to develop yet, and the conflicts will be a nightmare to sort out). If we can keep this commit in its separate AUTO-MERGE pr that'd be great so it's easier to follow the changes.

RELEASE.md Outdated Show resolved Hide resolved
RELEASE.md Outdated Show resolved Hide resolved
tests/framework/cli/micropkg/test_micropkg_requirements.py Outdated Show resolved Hide resolved
tests/framework/cli/micropkg/test_micropkg_requirements.py Outdated Show resolved Hide resolved
kedro/framework/cli/micropkg.py Show resolved Hide resolved
Signed-off-by: SajidAlamQB <90610031+SajidAlamQB@users.noreply.github.com>
Copy link
Contributor

@lorenabalan lorenabalan left a comment

Choose a reason for hiding this comment

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

Feel free to merge this, all commits on main should've reached develop as well. :D

SajidAlamQB and others added 8 commits February 16, 2022 16:28
Signed-off-by: SajidAlamQB <90610031+SajidAlamQB@users.noreply.github.com>
This reverts commit bf0755c.
Signed-off-by: Antony Milne <antony.milne@quantumblack.com>
Signed-off-by: Antony Milne <antony.milne@quantumblack.com>
Signed-off-by: Antony Milne <antony.milne@quantumblack.com>
@antonymilne antonymilne merged commit 6c579d5 into main Feb 17, 2022
@antonymilne antonymilne deleted the Add-micropackaging-command-group#ked-2970 branch February 17, 2022 23:25
Galileo-Galilei pushed a commit to Galileo-Galilei/kedro that referenced this pull request Feb 19, 2022
lvijnck pushed a commit to lvijnck/kedro that referenced this pull request Apr 7, 2022
Signed-off-by: Laurens Vijnck <laurens_vijnck@mckinsey.com>
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.

5 participants