-
Notifications
You must be signed in to change notification settings - Fork 906
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
Move _find_run_command
from template to framework
#4012
Conversation
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
_find_run_command
from template to framework
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.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.
LGTM! ✨
Shall we create a ticket on starters side?
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.
It looks good in general. I wonder if you found the reason why this was necessary to be implemented in the starter
side instead of kedro
at first place?
It was implemented as part of making |
|
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
.../test_starter/{{ cookiecutter.repo_name }}/src/{{ cookiecutter.python_package }}/__main__.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.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.
Thank you! Approved with a small comment.
Co-authored-by: Nok Lam Chan <nok.lam.chan@quantumblack.com> Signed-off-by: Merel Theisen <49397448+merelcht@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.
LGTM, thank you!
Description
#3051
Moved the
_find_run_command
method from the project template to the framework itself.This is necessary refactoring to make running packaged Kedro better (#3237)
Development notes
In the first attempt I moved the find run logic to
kedro.framework.project.__init__
, because that's also whereconfigure_project()
is. However, with that solution there are direct imports fromkedro.framework.project
tokedro.framework.cli
which breaks the contract we've setup.Contract: https://github.com/kedro-org/kedro/blob/main/pyproject.toml#L159
In order not to break the import contract, I've moved the find run logic to
kedro/framework/cli/project.py
Note
The same change to
__main__.py
needs to be done on the starters sideDeveloper Certificate of Origin
We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a
Signed-off-by
line in the commit message. See our wiki for guidance.If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.
Checklist
RELEASE.md
file