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

Add addon flags to kedro new #3081

Merged
merged 99 commits into from
Oct 24, 2023
Merged

Add addon flags to kedro new #3081

merged 99 commits into from
Oct 24, 2023

Conversation

lrcouto
Copy link
Contributor

@lrcouto lrcouto commented Sep 25, 2023

NOTE: Kedro datasets are moving from kedro.extras.datasets to a separate kedro-datasets package in
kedro-plugins repository. Any changes to the dataset implementations
should be done by opening a pull request in that repository.

Description

Part of the add-ons flow work. Adds an --addons flag to kedro new that allows the used to select which addons they would like to have on their new project through the CLI.

Examples:

kedro new --addons=lint,test,log,docs,data
kedro new --addons=all
kedro new --addons=none

Development notes

Checklist

  • Read the contributing guidelines
  • Signed off each commit with a Developer Certificate of Origin (DCO)
  • 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
  • Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

SajidAlamQB and others added 30 commits August 30, 2023 13:17
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>
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>
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>
Signed-off-by: lrcouto <laurarccouto@gmail.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>
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>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
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.

The addons flag now works well if you provide valid add-ons, however if you try something like kedro new --addons=random it will still prompt for the project name and then you get an error like:

(dev) M-C02XN0V5JHD5:Testing merel_theisen$ kedro new --addons=random

Project Name
============
Please enter a human readable name for your new project.
Spaces, hyphens, and underscores are allowed.
 (New Kedro Project): bla
Traceback (most recent call last):
  File "/var/folders/_3/kzkd3jjj31s24wjnsg7qgnk80000gn/T/tmp02ezo4dy.py", line 27, in <module>
    main()
  File "/var/folders/_3/kzkd3jjj31s24wjnsg7qgnk80000gn/T/tmp02ezo4dy.py", line 18, in main
    selected_add_ons_list = parse_add_ons_input(selected_add_ons)
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/merel_theisen/Projects/kedro/kedro/templates/project/hooks/utils.py", line 90, in parse_add_ons_input
    _validate_selection(selected)
  File "/Users/merel_theisen/Projects/kedro/kedro/templates/project/hooks/utils.py", line 58, in _validate_selection
    if int(add_on) < 1 or int(add_on) > 5:
       ^^^^^^^^^^^
ValueError: invalid literal for int() with base 10: 'random'
[10/19/23 13:26:09] ERROR    Stopping generation because post_gen_project hook script didn't exit         hooks.py:159
                             successfully
kedro.framework.cli.utils.KedroCliError: Failed to generate project when running cookiecutter.
Run with --verbose to see the full exception
Error: Failed to generate project when running cookiecutter.

Ideally, the add on input gets validated before it promts for the project name and the message shouldn't be ValueError: invalid literal for int() with base 10: 'random' but more something along the lines of "random isn't a valid option for addons, the possible options are ..."

lrcouto and others added 6 commits October 19, 2023 16:12
Signed-off-by: lrcouto <laurarccouto@gmail.com>
Signed-off-by: lrcouto <laurarccouto@gmail.com>
Signed-off-by: lrcouto <laurarccouto@gmail.com>
Signed-off-by: lrcouto <laurarccouto@gmail.com>
Copy link
Contributor

@SajidAlamQB SajidAlamQB left a comment

Choose a reason for hiding this comment

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

Thank you @lrcouto, I've tested it manually and it looks great and on the right track!

There were a few things I noticed though. When we separate the add-ons with spaces after each comma the command fails. i.e lint,test,log works but not lint, test, log maybe we need some logic that sanitises the whitespace from inputs.

Also I noticed if we do lint,all we get unhelpful/confusing error:

ValueError: invalid literal for int() with base 10: 'all'
[10/23/23 16:14:58] ERROR    Stopping generation because post_gen_project hook script didn't exit successfully                  generate.py:256
kedro.framework.cli.utils.KedroCliError: Failed to generate project when running cookiecutter.
Run with --verbose to see the full exception
Error: Failed to generate project when running cookiecutter.

We might want a more clear error message telling users they have to select between a combination of add ons or none/all.

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.

I've tested this again after your changes and I think this looks really good 👏 . I can't think of any other edge cases at the moment. I would add this change to the release notes for visibility of the feature ✍️

@SajidAlamQB you made a very good point about the spaces. The way it's implemented now is consistent with e.g. kedro run --tags=.... spaces can only be "allowed" if they are inside a string so: kedro new --addons="test, docs, log", but not like kedro new --addons=test, docs.

Signed-off-by: L. R. Couto <57910428+lrcouto@users.noreply.github.com>
@lrcouto
Copy link
Contributor Author

lrcouto commented Oct 24, 2023

I've tested this again after your changes and I think this looks really good 👏 . I can't think of any other edge cases at the moment. I would add this change to the release notes for visibility of the feature ✍️

@SajidAlamQB you made a very good point about the spaces. The way it's implemented now is consistent with e.g. kedro run --tags=.... spaces can only be "allowed" if they are inside a string so: kedro new --addons="test, docs, log", but not like kedro new --addons=test, docs.

The way it is right now, trying to pass them outside of quotation marks will return an "unexpected argument" error. Using them inside quotation marks as in kedro new --addons="lint, test, log" should work.

Signed-off-by: L. R. Couto <57910428+lrcouto@users.noreply.github.com>
@astrojuanlu

This comment was marked as outdated.

Signed-off-by: L. R. Couto <57910428+lrcouto@users.noreply.github.com>
@lrcouto
Copy link
Contributor Author

lrcouto commented Oct 24, 2023

I confirm @SajidAlamQB 's case above still raises an unhelpful error:

❯ kedro new --addons=lint,all                                                                       (kedro38-dev) 

Project Name
============
Please enter a human readable name for your new project.
Spaces, hyphens, and underscores are allowed.
 [New Kedro Project]: test-spaceflights
Traceback (most recent call last):
  File "/var/folders/r7/ywj0_kvj0mxfkdkx0jrgh73r0000gn/T/tmphs49kn1j.py", line 28, in <module>
    main()
  File "/var/folders/r7/ywj0_kvj0mxfkdkx0jrgh73r0000gn/T/tmphs49kn1j.py", line 19, in main
    selected_add_ons_list = parse_add_ons_input(selected_add_ons)
  File "/Users/juan_cano/Projects/QuantumBlack Labs/kedro/kedro/templates/project/hooks/utils.py", line 91, in parse_add_ons_input
    _validate_selection(selected)
  File "/Users/juan_cano/Projects/QuantumBlack Labs/kedro/kedro/templates/project/hooks/utils.py", line 59, in _validate_selection
    if int(add_on) < 1 or int(add_on) > 5:
ValueError: invalid literal for int() with base 10: 'lint'

When I try to reproduce this, I get the folllwing result:

image

@astrojuanlu
Copy link
Member

Turns out I was using an old commit 🤦🏽 All good!

@astrojuanlu
Copy link
Member

Only missing bit then is adding this to the release notes

lrcouto and others added 2 commits October 24, 2023 16:23
Signed-off-by: lrcouto <laurarccouto@gmail.com>
Signed-off-by: L. R. Couto <57910428+lrcouto@users.noreply.github.com>
@astrojuanlu astrojuanlu enabled auto-merge (squash) October 24, 2023 20:09
Signed-off-by: L. R. Couto <57910428+lrcouto@users.noreply.github.com>
@lrcouto lrcouto enabled auto-merge (squash) October 24, 2023 21:09
@lrcouto lrcouto merged commit 5752f68 into develop Oct 24, 2023
53 of 58 checks passed
@lrcouto lrcouto deleted the add-addon-flags-to-kedro-new branch October 24, 2023 21:36
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.

6 participants