-
Notifications
You must be signed in to change notification settings - Fork 915
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
MVP for "add-ons" flow within kedro new CLI command #2987
Conversation
Signed-off-by: SajidAlamQB <90610031+SajidAlamQB@users.noreply.github.com>
Signed-off-by: SajidAlamQB <90610031+SajidAlamQB@users.noreply.github.com>
I've looked into boolean_variables on cookiecutter and I'm not so sure they are the best approach here. Our options are multi-choices if we opt with using the boolean_variables it would mean each add-on then becomes a separate prompt for a yes/no decision. I feel this would be a detriment to user experience as it would take users longer to answer a series of yes/no questions rather than making a single multi-choice selection. The drawback is if we go with single multi-choice option is the implementation on |
Signed-off-by: SajidAlamQB <90610031+SajidAlamQB@users.noreply.github.com>
Signed-off-by: SajidAlamQB <90610031+SajidAlamQB@users.noreply.github.com>
@merelcht @amandakys @lrcouto @deepyaman, this is still very much a WIP but its in a usable state. I was hoping to get some early feedback on it before going further? This PR is essentially doing both #2850 and #2837 as it made more sense to do these in tandem. |
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.
I have had a quick look and left some initial comments. In general, I think the approach looks good 👍
Signed-off-by: SajidAlamQB <90610031+SajidAlamQB@users.noreply.github.com>
Signed-off-by: SajidAlamQB <90610031+SajidAlamQB@users.noreply.github.com>
As we're also implementing the Cookiecutter hooks in this PR I had some thoughts on some approaches we could take. Specifically to handle the dependencies in Approach A (Reactive): Start with a requirements.txt and pyproject.toml that includes dependencies for all add-ons by default as we do now. Adv:
Cons:
Approach B (Proactive): Start with a barebones Adv:
Cons:
Which approach do you think is more better for our needs? I am inclined to Approach B but also open to hear everyone's thoughts or opinions. PS, maybe an even more radical idea we have separate dependency files for each add-on: e.g., |
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: lrcouto <laurarccouto@gmail.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@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.
Looks good overall left some comments. I think its almost ready to go in.
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@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.
I tried it out once more and it all looks great! Much better error messages 👍
I would add this change to the release notes already, so we don't forget for 0.19.0
!
Co-authored-by: Sajid Alam <90610031+SajidAlamQB@users.noreply.github.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@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.
Manually tested and it works great! 💯
The only thing I noticed is that it doesn't accept different capitalisations of "all" or "none". Maybe it's a design decision? But we could easily allow any capitalisation of "all" or "none" and simple convert to lowercase before checking!
Noticed that as well! I think it's fine for the MVP, but definitely something to consider with design. CC: @amandakys |
Notes for reviewers - @AhdraMeraliQB
Thank you for taking the time to review almost 600 new lines of code - we hope you enjoy your stay.
Just a few notes before diving right in:
1. This is the first iteration implementation of the add-ons flow (see: #2837 and #2850). It is important to note that the design of what the final workflow will look like is still in progress, and as a result, this PR may deviate somewhat from the descriptions in the aforementioned issues. Any changes/additions to the design will be addressed in #3047.
2. Not all changes are created equal. Here's a quick walkthrough of the changed files and what implementation lives where (ordered by degree of change):
Implementation
kedro/templates/project/hooks/post_gen_project.py
: The code run bycookiecutter
after the project template is generated.kedro/templates/project/hooks/utils.py
: Functions called bypost_gen_project.py
to setup the add-ons. Includes parsing user input, tear-down of unnecessary files, injection of necessary requirements, and alphabetic sorting of requirements.kedro/templates/project/prompts.yml
: The new add-on related prompt output when runningkedro new
with no starter and no config path.kedro/framework/cli/starters.py
: Additions to the output generated after project creation and regex validation for add-ons passed through from configkedro/templates/project/{{ cookiecutter.repo_name }}/pyproject.toml
,kedro/templates/project/{{ cookiecutter.repo_name }}/src/pyproject.toml
, andkedro/templates/project/{{ cookiecutter.repo_name }}/src/requirements.txt
: Removal of testing, linting, and documentation requirements from the base template.kedro/templates/project/cookiecutter.json
: Updated to include add-onskedro/templates/project/{{ cookiecutter.repo_name }}/conf/logging.yml
: The return of the logging configpyproject.toml
: A fix to a somewhat recurring issue; see this stackoverflow postTests
tests/framework/cli/test_starters.py
: Modified old tests to work with new flow, added requirements check helper function, and added tests to check success of including add-ons via CLI prompt and config file.features/*
: Any tests that make use ofkedro new
or any processes that are now part of add-ons have been updated in accordance to the new flow3. If you've got the time, try and break it! After checking out this PR and installing the local version of Kedro, testing the changes is as simple as running
kedro new
. This is what it should look like if you do:At which point you can select whichever combination of add-ons you wish to include (in this examples I chose none, the default). You'll then be prompted to give this project a name:
In this example I chose
test add-ons
. You should then see the following text:Exploring the file structure of the template generated, you should be able to determine if the add-on selection was successful (hopefully it is 🤞 ).
4. Opinion time! What do you think of this as a first implementation?
Any small pieces you think would look better changed around? (E.g Should the flow prompt for the project name before the add-ons?) Do you think the implementation is perfect as it is and you'd like to see this in ASAP? Let us know!
The rest of the notes below are the original PR description.
Description
Following #2758, this PR will seek to implement #2850
Update the kedro new command to allow users to select "add-ons" from:
testing
linting
logging
data structure
documentation
Development notes
This PR will also be partly completing #2837 as the design of the CLI is tightly coupled with implementation.
prompts.yml
andcookiecutter.json
updated.post_gen_update.py
hook for cookiecutter added left placeholder code for rest of implementation. (Should the implementation live here? Maybe it should go intostarters.py
?Note: This PR does not implement the
--add-on
flag proposed by Add addon flags tokedro new
command #2873TODO:
Checklist
RELEASE.md
file