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.

Sign up for GitHub

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

Use click features for project creation prompts #4387

Open
wants to merge 31 commits into
base: main
Choose a base branch
from
Open

Use click features for project creation prompts #4387

Changes from 24 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
9bb3361
Test prompt change
lrcouto Dec 17, 2024
10dbea5
Test adding default value
lrcouto Dec 17, 2024
9b4a9a7
Change other prompts
lrcouto Dec 17, 2024
73180ca
Move prompting logic to the inside of new function
lrcouto Dec 18, 2024
48934d5
fix name default
lrcouto Dec 18, 2024
3f184a8
Merge branch 'main' into click-validation-options
lrcouto Jan 13, 2025
75456b7
Merge branch 'main' into click-validation-options
lrcouto Jan 13, 2025
601b02f
Add auxiliary function for converting numbers to tools
lrcouto Jan 13, 2025
8305cae
Merge branch 'main' into click-validation-options
lrcouto Jan 14, 2025
05b7479
Merge branch 'main' into click-validation-options
lrcouto Jan 14, 2025
7b71418
Normalize whitespaces in test outputs
lrcouto Jan 14, 2025
d0af3d3
Merge branch 'main' into click-validation-options
lrcouto Jan 15, 2025
7aade5b
Merge branch 'main' into click-validation-options
lrcouto Jan 15, 2025
0ce0219
Prevent prompts from happening when a config file is used
lrcouto Jan 15, 2025
6d37aed
move click prompting to replace cookiecutter's
lrcouto Jan 16, 2025
893415d
Remove reduntant variables
lrcouto Jan 16, 2025
f8cc6c3
Fix more tests
lrcouto Jan 16, 2025
c1e96df
All unit tests passing
lrcouto Jan 16, 2025
eae9d5a
Merge branch 'main' into click-validation-options
lrcouto Jan 16, 2025
1d7af53
Revert unnecessary changes on tests
lrcouto Jan 16, 2025
0ac3d2b
remove reduntant leftover code
lrcouto Jan 16, 2025
d8a6b6f
remove reduntant leftover code
lrcouto Jan 16, 2025
f9534ee
Set up prompt appearance
lrcouto Jan 17, 2025
f777d09
Revert default to unformatted
lrcouto Jan 18, 2025
da38e1a
Merge branch 'main' into click-validation-options
lrcouto Jan 21, 2025
6cc3e15
Switch yes/no CLI prompts to click validation
lrcouto Jan 21, 2025
e71e015
Lint
lrcouto Jan 21, 2025
5828415
Use click.Choice for telemetry, rest as our own function
lrcouto Jan 21, 2025
0923ca4
Merge branch 'main' into click-validation-options
lrcouto Jan 22, 2025
8eec43b
Change example validation back to what it was initially
lrcouto Jan 22, 2025
20e5fe7
Merge branch 'main' into click-validation-options
lrcouto Jan 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 35 additions & 14 deletions kedro/framework/cli/starters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ def _kedro_version_equal_or_lower_to_starters(version: str) -> bool:
"pyspark": "6",
"viz": "7",
}

NUMBER_TO_TOOLS_NAME = {
"1": "Linting",
"2": "Testing",
Expand Down Expand Up @@ -311,8 +312,18 @@ def starter() -> None:
@click.option("--starter", "-s", "starter_alias", help=STARTER_ARG_HELP)
@click.option("--checkout", help=CHECKOUT_ARG_HELP)
@click.option("--directory", help=DIRECTORY_ARG_HELP)
@click.option("--tools", "-t", "selected_tools", help=TOOLS_ARG_HELP)
@click.option("--name", "-n", "project_name", help=NAME_ARG_HELP)
@click.option(
"--name",
"-n",
"project_name",
help=NAME_ARG_HELP,
)
@click.option(
"--tools",
"-t",
"selected_tools",
help=TOOLS_ARG_HELP,
)
@click.option("--example", "-e", "example_pipeline", help=EXAMPLE_ARG_HELP)
Copy link
Member

Choose a reason for hiding this comment

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

Can you use Click choice options here https://click.palletsprojects.com/en/stable/options/#choice-options to simplify the prompting and validation logic for example project?

@click.option("--telemetry", "-tc", "telemetry_consent", help=TELEMETRY_ARG_HELP)
def new( # noqa: PLR0913
Expand All @@ -337,6 +348,7 @@ def new( # noqa: PLR0913
"example": example_pipeline,
"telemetry_consent": telemetry_consent,
}

_validate_flag_inputs(flag_inputs)
starters_dict = _get_starters_dict()

Expand Down Expand Up @@ -381,6 +393,7 @@ def new( # noqa: PLR0913
shutil.rmtree(tmpdir, onerror=_remove_readonly) # type: ignore[arg-type]

# Obtain config, either from a file or from interactive user prompts.

extra_context = _get_extra_context(
prompts_required=prompts_required,
config_path=config_path,
Expand Down Expand Up @@ -727,7 +740,8 @@ def _fetch_validate_parse_config_from_file(


def _fetch_validate_parse_config_from_user_prompts(
prompts: dict[str, Any], cookiecutter_context: OrderedDict | None
prompts: dict[str, Any],
cookiecutter_context: OrderedDict | None,
) -> dict[str, str]:
"""Interactively obtains information from user prompts.

Expand All @@ -739,9 +753,6 @@ def _fetch_validate_parse_config_from_user_prompts(
Configuration for starting a new project. This is passed as ``extra_context``
to cookiecutter and will overwrite the cookiecutter.json defaults.
"""
from cookiecutter.environment import StrictEnvironment
from cookiecutter.prompt import read_user_variable, render_variable

if not cookiecutter_context:
raise Exception("No cookiecutter context available.")

Expand All @@ -751,14 +762,16 @@ def _fetch_validate_parse_config_from_user_prompts(
prompt = _Prompt(**prompt_dict)

# render the variable on the command line
cookiecutter_variable = render_variable(
env=StrictEnvironment(context=cookiecutter_context),
raw=cookiecutter_context.get(variable_name),
cookiecutter_dict=config,
)
default_value = cookiecutter_context.get(variable_name) or ""

# read the user's input for the variable
user_input = read_user_variable(str(prompt), cookiecutter_variable)
user_input = click.prompt(
str(prompt),
default=default_value,
show_default=True,
type=str,
).strip()

if user_input:
prompt.validate(user_input)
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need to use our own custom validation or is it possible to use click for that as well? I saw that was one of the suggestions on the original ticket #3878

Copy link
Member

Choose a reason for hiding this comment

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

Especially lower down in the code we have pretty complex parsing/validation methods e.g. _parse_tools_input.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm looking into this right now. From what I had seen I think the options that click has might be a bit unspecific for what we need, but there's an option to apply custom validation logic.

Copy link
Member

Choose a reason for hiding this comment

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

Yes for the more complicated options it might not be possible, but for --example and --telemetry we only need to check yes/no/y/n. I think you can do something like:

@click.option(
    "--example",
    "-e",
    "example_pipeline",
    help=EXAMPLE_ARG_HELP,
    type=click.Choice(["yes", "no", "y", "n"], case_sensitive=False),
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That does work for yes/no options. One minor issue though is that the error message is not customizable though. On top of that, the example pipeline option received through a config file would still need to have some validation since it doesn't go through the CLI options, so we'd still need the function.

This is what the error message from click.Choice looks like:

image

As opposed to the one we have right now, from _validate_input_with_regex_pattern:

image

For interactive prompts, that are happening inside a loop, there would need to be some sort of logic to pass the click.Choice value for the different prompts. Could be done through the prompts.yml file maybe.

Copy link
Contributor Author

@lrcouto lrcouto Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

For that specific validate we could use one of the click Exception options, though I feel like they were more designed to be used with CLI options and not with prompts. I also personally like having finer control over the appearance of the error messages.

(Also, could we maybe move this user_input parameter to be inside the _Prompt class and validate it as we set it? The function is called only once, I don't think it necessarily needs to be a function)

Copy link
Contributor Author

@lrcouto lrcouto Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Another possibility is using the callback parameter in click for custom validation. For example, this:

def _validate_yes_no(ctx, param, value) -> None:
    if not re.match(r"(?i)^\s*(y|yes|n|no)\s*$", value, flags=re.X):
        raise click.BadParameter(f"'{value}' is an invalid value for {param}. It must contain only y, n, YES, or NO (case insensitive).")
    return value
    
    @click.option("--telemetry", "-tc", "telemetry_consent", help=TELEMETRY_ARG_HELP, callback=_validate_yes_no)

Results in this:

image

Does require writing the callback functions though. And the output is the click Exception output.

Copy link
Member

Choose a reason for hiding this comment

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

I personally think the default click exception ("Invalid value for..") is fine. But maybe @astrojuanlu has a different view on that.

Copy link
Member

Choose a reason for hiding this comment

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

I agree the default click exception is fine, considering the tradeoffs

config[variable_name] = user_input
Expand Down Expand Up @@ -997,9 +1010,17 @@ def __init__(self, *args: Any, **kwargs: Any) -> None:
self.error_message = kwargs.get("error_message", "")

def __str__(self) -> str:
# Format the title with optional color
title = self.title.strip().title()
title = click.style(title + "\n" + "=" * len(title), bold=True)
prompt_lines = [title, self.text]
title = click.style(title, fg="cyan", bold=True)
title_line = "=" * len(self.title)
title_line = click.style(title_line, fg="cyan", bold=True)

# Format the main text
text = self.text.strip()

# Combine everything
prompt_lines = [title, title_line, text]
prompt_text = "\n".join(str(line).strip() for line in prompt_lines)
return f"\n{prompt_text}\n"

Expand Down
Loading