-
Notifications
You must be signed in to change notification settings - Fork 903
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
Conversation
There was a problem hiding this 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.
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 |
There was a problem hiding this 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.
This reverts commit a582d05.
There was a problem hiding this 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>
There was a problem hiding this 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.
Signed-off-by: SajidAlamQB <90610031+SajidAlamQB@users.noreply.github.com>
There was a problem hiding this 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
Signed-off-by: SajidAlamQB <90610031+SajidAlamQB@users.noreply.github.com>
This reverts commit bf0755c.
Signed-off-by: Antony Milne <antony.milne@quantumblack.com>
This reverts commit e6b7c69.
Signed-off-by: Antony Milne <antony.milne@quantumblack.com>
Signed-off-by: Antony Milne <antony.milne@quantumblack.com>
Signed-off-by: Laurens Vijnck <laurens_vijnck@mckinsey.com>
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 alongsidekedro 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
tomicropkg.py
including_install_files
,_generate_wheel_file
etc..Checklist
RELEASE.md
file