Add an option to get an example pipeline #3295

DimedS · 2023-11-09T14:22:19Z

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

This PR introduces an --example flag to the kedro new command. Users can specify --example y/n directly in the command line to indicate their preference. If not specified, users will be prompted interactively, with 'No' as the default selection.

User flow:

When a user selects Viz, PySpark, or both, the corresponding starter will be provided.
If example=No is chosen in this scenario, data and config will be removed via hooks in the selected starter.
If neither Viz nor PySpark is selected:
If example=Yes is chosen, the 'spaceflights-starter' will be provided.
If example=No is chosen, a standard template will be provided.
Then final template will be stripped according to add_ons 1-5 selection, exception: Data folder, it will not be stripped if example=Yes chosen.

Development notes

Temporary added a checkout for "3076-add-example-pipeline-to-hooks" branch of starters git repo to properly test together with changes in starters in PR 181 (643-663 lines in starters.py)
Will add additional unit tests to cover my changes.

Developer 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

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

DimedS · 2023-11-09T14:27:06Z

Do we need to copy postGen cookiecutter hooks to 4 spaceflights starters to enable stripping based on add_ons chosen, or we already have a better solution for that?
@merelcht @noklam @ankatiyar @SajidAlamQB

merelcht · 2023-11-13T10:40:11Z

kedro/framework/cli/starters.py

    starter_path = "git+https://github.com/kedro-org/kedro-starters.git"
-    if add_ons:
+    if example_pipeline:


This isn't correct. Even if the example pipeline isn't wanted, we might need to fetch from a starter template other than the default. @SajidAlamQB can probably help out here about what the flow should be.

@merelcht , thank you. You right, I need to consult with @SajidAlamQB what should we do in case of Spark and Viz but no example option choosen.

I think this doesn't need to be changed as we already select the right template with examples in them for pyspark, viz and pyspark & viz. These templates already contain the example pipelines but are removed in hooks/utils.py _handle_starter_setup. So the current default case is example=no. I think we just need a check in utils.py to skip _handle_starter_setup if example=yes.

@SajidAlamQB, many thanks, got it

I think only for the default template [1-5], options without pyspark and viz, we don't already have example pipelines so we might need to change template to spaceflights-pandas in here if example=yes.

I think only for the default template [1-5], options without pyspark and viz, we don't already have example pipelines so we might need to change template to spaceflights-pandas in here if example=yes.

yes, looks like logic should be different for pyspark, viz and without them:

without pyspark, viz: If --example==yes, use sfaceflights-pandas starter, elif --example ==no, use default template

with pyspark or viz or both: use starter anyway, but we need modifications in _handle_starter_setup

Last step: strip all of them with the same algorithm based on add_ons 1-5.

Is it correct?

Yes, this sounds right.

noklam · 2023-11-14T06:49:57Z

#3054

@DimedS
I notice the current order doesn't match with #3054, the example prompt should happen before project_name

In addition, I wonder is --example y/n needed? Can it be reduced to --example since it is a boolean option or is it unclear? @amandakys

DimedS · 2023-11-14T09:09:58Z

#3054

@DimedS I notice the current order doesn't match with #3054, the example prompt should happen before project_name

In addition, I wonder is --example y/n needed? Can it be reduced to --example since it is a boolean option or is it unclear? @amandakys

Hm, I see that the last version of #3054 with --example on the end:
#3054 (comment)

noklam · 2023-11-14T09:39:39Z

I see - I didn't see the latest comment. We should push the project name as the first prompt, but it's out of scope for this PR.

Signed-off-by: Dmitry Sorokin <dmd40in@gmail.com>

noklam · 2023-11-16T04:39:46Z

MANIFEST.in

@@ -3,4 +3,4 @@ include LICENSE.md
 include kedro/framework/project/default_logging.yml
 include kedro/ipython/*.png
 include kedro/ipython/*.svg
-recursive-include templates *
+recursive-include kedro/templates *


is this intended?

noklam

First of all, thank you for the PR and the detailed description. Having it checkout to a feature starter branch save me a lot of time to review and test.

I leave some comments but I think the key questions are

Can we simplify example_pipeline as a boolean flag from the CLI with just --example? If not, we should parse this early on as a boolean save ourselves from checking example_pipeline is a None or not at every step, since I think they don't make a difference.
Validation - the validation function assume click and it also does parsing, I think should keep it separated.

kedro/framework/cli/starters.py

noklam · 2023-11-16T04:50:56Z

kedro/framework/cli/starters.py

+    extra_context["example_pipeline"] = (
+        example_pipeline  # type: ignore
+        if example_pipeline is not None
+        else _validate_yn(None, None, extra_context.get("example_pipeline", None))


The _validate_yn is expected to used in click. It feels weird to have _validate_yn(None, None, ...). Can we simplify this a little bit, there is a lot of handling or None, if we can make sure this is a bool early on we can save ourselves a lot of checking.

kedro/framework/cli/starters.py

noklam · 2023-11-16T05:29:49Z

kedro/templates/project/hooks/utils.py

+    if "Data Structure" not in selected_add_ons_list and example_pipeline != "True":
        _remove_dir(current_dir / "data")

-    if "Pyspark" in selected_add_ons_list:
+    if "Pyspark" in selected_add_ons_list and example_pipeline != "True":
        _handle_starter_setup(selected_add_ons_list, python_package_name)

-    if "Kedro Viz" in selected_add_ons_list:
+    if "Kedro Viz" in selected_add_ons_list and example_pipeline != "True":


I think the logic here is correct but it is hard to follow. I notice that _handle_starter_setup is highly coupled with PySpark and Kedro Viz, in fact this function only get called if these plugins get selected.

The only difference is if viz is selected, one extra reporting pipeline is removed. I think this should reflected in the argument of _handle_starter_setup, we don't really need selected_add_ons_list, a boolean flag is enough.

Maybe it helps to have a nested if here, for me it easier to map it to the diagram.

I totally agree with @noklam here. I would rename _handle_starter_setup to make it clear this is removal of files, and I also think the code inside _handle_starter_setup can be more generic. We don't need to check pipeline names and test names, but just remove all files inside the pipelines folder etc.

I updated that part to clarify that it's only about Viz and PySpark, @merelcht do you think we should remove all files in tests/pipelines except of init.py?

DimedS · 2023-11-16T11:50:50Z

Can we simplify example_pipeline as a boolean flag from the CLI with just --example? If not, we should parse this early on as a boolean save ourselves from checking example_pipeline is a None or not at every step, since I think they don't make a difference.

Validation - the validation function assume click and it also does parsing, I think should keep it separated.

@noklam , many thanks for your notes.
I agree with all you written:

If we can use just --example flag not --example='y/n' in CLI, it will be simple - less validation and parsing, but I think we cannot do that - if client will not provide --example flag in CLI it doesn't mean that he don't want example (we cannot put False here) - we need to ask him in Prompt. So I think we should leave it the same to provide a possibility to write 'No' in CLI.
I'm also unhappy with current validation situation, I tried to put CLI validation directly to @click.option in click.Choice(["y", "n", "yes", "no"] and parsing in callback=_validate_yn func (seems to be a wrong function naming better name that func _parsing, but validation and parsing are separated currently). But it seems unbeautiful because of specific callback parsing function format that you mentioned.
And I agree that it's better to unify validation process (also wrote about it there) because it's 3 possible inputs (CLI, config, prompts) better to validate and parse them in the same code if it's possible.
And you right, currently I don't provide a validation for loading from config, only parsing in _validate_yn().
I will rewrite validation and parsing part.

kedro/templates/project/hooks/utils.py

merelcht

This part of the code

kedro/kedro/framework/cli/starters.py

Lines 862 to 868 in 8c6d5c1

    
           if template_path == str(TEMPLATE_PATH) or ( 
        
               add_ons and ("Pyspark" in add_ons or "Kedro Viz" in add_ons) 
        
           ): 
        
               if add_ons == "[]":  # TODO: This should be a list 
        
                   click.secho("\nYou have selected no add-ons") 
        
               else: 
        
                   click.secho(f"\nYou have selected the following add-ons: {add_ons}")

should also be updated. Now you don't see which add-ons you have selected when you use an example.

kedro/framework/cli/starters.py

kedro/templates/project/cookiecutter.json

merelcht · 2023-11-17T12:16:49Z

kedro/templates/project/hooks/utils.py

+    if "Data Structure" not in selected_add_ons_list and example_pipeline != "True":
        _remove_dir(current_dir / "data")

-    if "Pyspark" in selected_add_ons_list:
+    if "Pyspark" in selected_add_ons_list and example_pipeline != "True":
        _handle_starter_setup(selected_add_ons_list, python_package_name)

-    if "Kedro Viz" in selected_add_ons_list:
+    if "Kedro Viz" in selected_add_ons_list and example_pipeline != "True":


I totally agree with @noklam here. I would rename _handle_starter_setup to make it clear this is removal of files, and I also think the code inside _handle_starter_setup can be more generic. We don't need to check pipeline names and test names, but just remove all files inside the pipelines folder etc.

Signed-off-by: Dmitry Sorokin <dmd40in@gmail.com>

…line

Signed-off-by: Dmitry Sorokin <dmd40in@gmail.com>

DimedS · 2023-11-20T16:56:22Z

It appears that I've addressed all the review comments and have also included new tests to cover the example option. Could you please review it again? Currently, I'm noticing an issue with the docs, I will investigate it further, other tests are ok.

merelcht

This part of the code

kedro/kedro/framework/cli/starters.py

Lines 862 to 868 in 8c6d5c1

if template_path == str(TEMPLATE_PATH) or (

add_ons and ("Pyspark" in add_ons or "Kedro Viz" in add_ons)

):

if add_ons == "[]": # TODO: This should be a list

click.secho("\nYou have selected no add-ons")

else:

click.secho(f"\nYou have selected the following add-ons: {add_ons}")

should also be updated. Now you don't see which add-ons you have selected when you use an example.

@DimedS This still needs to be updated. If I choose an example I don't get a message showing me what add_ons I chose.

merelcht · 2023-11-21T10:04:18Z

kedro/framework/cli/starters.py

+        cookiecutter_args["directory"] = "spaceflights-pyspark-viz"
+        cookiecutter_args[
+            "checkout"
+        ] = "3076-add-example-pipeline-to-hooks"  # ToDel: temporary for test


Don't forget to change this back before merging, and all other occurrences below.

Looks like I can change it back only after this PR will be merged, otherwise some tests will fail

kedro/framework/cli/starters.py

kedro/templates/project/hooks/utils.py

tests/framework/cli/test_starters.py

…-to-get-an-example-pipeline

Signed-off-by: Dmitry Sorokin <dmd40in@gmail.com>

…line

DimedS · 2023-11-21T14:11:41Z

@DimedS This still needs to be updated. If I choose an example I don't get a message showing me what add_ons I chose.

Sorry, missed it, fixed with other last comments

Signed-off-by: Dmitry Sorokin <dmd40in@gmail.com>

merelcht

This looks good to be merged now! Nice work @DimedS 👏
Don't forget to add it to the release notes 😃

SajidAlamQB

Tested this manually and all looks good! Thanks you @DimedS awesome work! 🌟

SajidAlamQB · 2023-11-21T16:14:49Z

tests/framework/cli/test_starters.py

+        expected_files += sum(example_files_count)
+        expected_files += (
+            4 if "7" in add_ons_list else 0
+        )  # add 3 .py and 1 parameters files in reporting for Viz


Thanks for the comments, these tests can be confusing to understand. 😁

noklam

Thanks for the PR! This is one of the more complicated piece.

Don't remember to update the RELEASE.md and revert the hardcoded branch.

        "checkout"
        ] = "3076-add-example-pipeline-to-hooks"

…line

Signed-off-by: Dmitry Sorokin <dmd40in@gmail.com>

Add an option to get an example pipeline, fix and add new tests --------- Signed-off-by: Dmitry Sorokin <dmd40in@gmail.com> Signed-off-by: Jo Stichbury <jo_stichbury@mckinsey.com>

DimedS requested review from noklam, ankatiyar and SajidAlamQB November 9, 2023 14:23

merelcht reviewed Nov 13, 2023

View reviewed changes

DimedS linked an issue Nov 13, 2023 that may be closed by this pull request

Add an option to get an example pipeline in the project creation flow #3076

Closed

DimedS force-pushed the 3076-add-an-option-to-get-an-example-pipeline branch from c73d901 to 73d4acb Compare November 13, 2023 15:08

DimedS force-pushed the 3076-add-an-option-to-get-an-example-pipeline branch from 2ecafbf to 01d1c0d Compare November 14, 2023 11:31

DimedS mentioned this pull request Nov 14, 2023

Add example_pipeline to starters hooks kedro-org/kedro-starters#181

Merged

3 tasks

DimedS added 3 commits November 15, 2023 12:29

Add example option and fix tests

e6395f3

Signed-off-by: Dmitry Sorokin <dmd40in@gmail.com>

Linting

e53a170

Signed-off-by: Dmitry Sorokin <dmd40in@gmail.com>

Fix regex

8c6d5c1

Signed-off-by: Dmitry Sorokin <dmd40in@gmail.com>

DimedS force-pushed the 3076-add-an-option-to-get-an-example-pipeline branch from 32c4477 to 8c6d5c1 Compare November 15, 2023 16:21

DimedS marked this pull request as ready for review November 15, 2023 16:59

noklam reviewed Nov 16, 2023

View reviewed changes

SajidAlamQB reviewed Nov 16, 2023

View reviewed changes

kedro/templates/project/hooks/utils.py Show resolved Hide resolved

merelcht reviewed Nov 17, 2023

View reviewed changes

DimedS and others added 7 commits November 17, 2023 18:35

Add tests to cover example option

67791c4

Signed-off-by: Dmitry Sorokin <dmd40in@gmail.com>

Update valdiation

faaa319

Signed-off-by: Dmitry Sorokin <dmd40in@gmail.com>

Fix to allow spaces around answer

e695556

Signed-off-by: Dmitry Sorokin <dmd40in@gmail.com>

Fix cookiecutter and invalid tests

60d708f

Signed-off-by: Dmitry Sorokin <dmd40in@gmail.com>

Fix hooks

9b8abff

Signed-off-by: Dmitry Sorokin <dmd40in@gmail.com>

Merge branch 'develop' into 3076-add-an-option-to-get-an-example-pipe…

5146ad9

…line

Final clean up

36901a1

Signed-off-by: Dmitry Sorokin <dmd40in@gmail.com>

DimedS requested a review from noklam November 20, 2023 16:51

DimedS requested review from merelcht and SajidAlamQB November 20, 2023 16:51

DimedS requested review from yetudada, astrojuanlu and stichbury as code owners November 21, 2023 10:04

merelcht reviewed Nov 21, 2023

View reviewed changes

stichbury removed their request for review November 21, 2023 10:49

DimedS added 2 commits November 21, 2023 11:38

Merge remote-tracking branch 'origin/develop' into 3076-add-an-option…

518f1eb

…-to-get-an-example-pipeline

Clean up

6c03f9f

Signed-off-by: Dmitry Sorokin <dmd40in@gmail.com>

DimedS force-pushed the 3076-add-an-option-to-get-an-example-pipeline branch from b9e91b7 to 6c03f9f Compare November 21, 2023 12:11

Merge branch 'develop' into 3076-add-an-option-to-get-an-example-pipe…

ec485a4

…line

Fix Viz name

d4eb4c0

Signed-off-by: Dmitry Sorokin <dmd40in@gmail.com>

merelcht approved these changes Nov 21, 2023

View reviewed changes

SajidAlamQB approved these changes Nov 21, 2023

View reviewed changes

noklam approved these changes Nov 22, 2023

View reviewed changes

DimedS and others added 2 commits November 22, 2023 17:34

Merge branch 'develop' into 3076-add-an-option-to-get-an-example-pipe…

7d6de74

…line

Update readme, clean checkout

cca5eff

Signed-off-by: Dmitry Sorokin <dmd40in@gmail.com>

DimedS merged commit 9ea4ce8 into develop Nov 22, 2023
36 checks passed

DimedS deleted the 3076-add-an-option-to-get-an-example-pipeline branch November 22, 2023 20:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an option to get an example pipeline #3295

Add an option to get an example pipeline #3295

DimedS commented Nov 9, 2023 •

edited

DimedS commented Nov 9, 2023

merelcht Nov 13, 2023

DimedS Nov 13, 2023 •

edited

SajidAlamQB Nov 13, 2023

DimedS Nov 13, 2023

SajidAlamQB Nov 13, 2023 •

edited

DimedS Nov 13, 2023 •

edited

SajidAlamQB Nov 13, 2023

noklam commented Nov 14, 2023

DimedS commented Nov 14, 2023

noklam commented Nov 14, 2023 •

edited

noklam Nov 16, 2023

noklam left a comment

noklam Nov 16, 2023

noklam Nov 16, 2023

merelcht Nov 17, 2023

DimedS Nov 20, 2023

DimedS commented Nov 16, 2023 •

edited

merelcht left a comment

merelcht Nov 17, 2023

DimedS commented Nov 20, 2023

merelcht left a comment •

edited

merelcht Nov 21, 2023

DimedS Nov 21, 2023

DimedS commented Nov 21, 2023

merelcht left a comment

SajidAlamQB left a comment

SajidAlamQB Nov 21, 2023 •

edited

noklam left a comment

	if template_path == str(TEMPLATE_PATH) or (
	add_ons and ("Pyspark" in add_ons or "Kedro Viz" in add_ons)
	):
	if add_ons == "[]": # TODO: This should be a list
	click.secho("\nYou have selected no add-ons")
	else:
	click.secho(f"\nYou have selected the following add-ons: {add_ons}")

Add an option to get an example pipeline #3295

Add an option to get an example pipeline #3295

Conversation

DimedS commented Nov 9, 2023 • edited

Description

Development notes

Developer Certificate of Origin

Checklist

DimedS commented Nov 9, 2023

Choose a reason for hiding this comment

DimedS Nov 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SajidAlamQB Nov 13, 2023 • edited

Choose a reason for hiding this comment

DimedS Nov 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noklam commented Nov 14, 2023

DimedS commented Nov 14, 2023

noklam commented Nov 14, 2023 • edited

Choose a reason for hiding this comment

noklam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DimedS commented Nov 16, 2023 • edited

merelcht left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DimedS commented Nov 20, 2023

merelcht left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DimedS commented Nov 21, 2023

merelcht left a comment

Choose a reason for hiding this comment

SajidAlamQB left a comment

Choose a reason for hiding this comment

SajidAlamQB Nov 21, 2023 • edited

Choose a reason for hiding this comment

noklam left a comment

Choose a reason for hiding this comment

DimedS commented Nov 9, 2023 •

edited

DimedS Nov 13, 2023 •

edited

SajidAlamQB Nov 13, 2023 •

edited

DimedS Nov 13, 2023 •

edited

noklam commented Nov 14, 2023 •

edited

DimedS commented Nov 16, 2023 •

edited

merelcht left a comment •

edited

SajidAlamQB Nov 21, 2023 •

edited