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

Simplify kedro new flow #1643

Merged
merged 76 commits into from
Jul 6, 2022
Merged

Simplify kedro new flow #1643

merged 76 commits into from
Jul 6, 2022

Conversation

merelcht
Copy link
Member

@merelcht merelcht commented Jun 22, 2022

Description

#1361

Development notes

  • Removed the prompts for repo_name and python_package. These variables will be generated automatically from the project_name provided by the user.
  • Added validation for the project_name input and changed it to only accept alphanumeric symbols, hyphens and underscores and check it's at least 2 characters long. This is to prevent the user from adding in special characters that would then also be used for the repo_name and python_package. See comments on this below.
  • Users can still provide all variables in a configuration file if they want custom repository and python package names.
  • Added tests to check that users can provide custom prompts as well and fixed some bugs that at first failed those tests.

The new flow looks like:

Screen Recording 2022-06-23 at 14 30 10

Left to do:
Need to update the starters as well and remove the repo_name and python_package prompts there.

Checklist

  • Read the contributing guidelines
  • 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

SajidAlamQB and others added 30 commits May 16, 2022 09:59
* remove deprecated kedro docs command

Signed-off-by: SajidAlamQB <90610031+SajidAlamQB@users.noreply.github.com>

* changes based on review

Signed-off-by: SajidAlamQB <90610031+SajidAlamQB@users.noreply.github.com>
[AUTO-MERGE] Merge main into develop via merge-main-to-develop
[AUTO-MERGE] Merge main into develop via merge-main-to-develop
* remove colorhandler

Signed-off-by: SajidAlamQB <90610031+SajidAlamQB@users.noreply.github.com>

* remove colorhandler test

Signed-off-by: SajidAlamQB <90610031+SajidAlamQB@users.noreply.github.com>

* fix ci issues

Signed-off-by: SajidAlamQB <90610031+SajidAlamQB@users.noreply.github.com>
[AUTO-MERGE] Merge main into develop via merge-main-to-develop
[AUTO-MERGE] Merge main into develop via merge-main-to-develop
* add config_loader property

Signed-off-by: noklam <nok.lam.chan@quantumblack.com>

* Update reference with `._config_loader` to the public attirbute.

Signed-off-by: noklam <nok.lam.chan@quantumblack.com>

* Wrap kedrocontext as dataclass

Signed-off-by: noklam <nok.lam.chan@quantumblack.com>

* Bug fix - swap the signature order for hook_manger and extra_params

Signed-off-by: noklam <nok.lam.chan@quantumblack.com>

* Fix test that should use the public attribute to access project path

Signed-off-by: noklam <nok.lam.chan@quantumblack.com>

* fix Liniting

Signed-off-by: noklam <nok.lam.chan@quantumblack.com>

* Use attrs to construct KedroContext

Signed-off-by: noklam <nok.lam.chan@quantumblack.com>

* update type info

Signed-off-by: noklam <nok.lam.chan@quantumblack.com>

* Use frozen attribute to replace property boilerplate

Signed-off-by: noklam <nok.lam.chan@quantumblack.com>

* Updated `project_path` to be frozen attribute.

Signed-off-by: noklam <nok.lam.chan@quantumblack.com>

* Update RELEASE.md

Signed-off-by: noklam <nok.lam.chan@quantumblack.com>

* update linting

Signed-off-by: noklam <nok.lam.chan@quantumblack.com>

* Update requirements.txt

Co-authored-by: Antony Milne <49395058+AntonyMilneQB@users.noreply.github.com>

* Update tests/framework/context/test_context.py

Co-authored-by: Antony Milne <49395058+AntonyMilneQB@users.noreply.github.com>

* Update kedro/framework/context/context.py

Co-authored-by: Antony Milne <49395058+AntonyMilneQB@users.noreply.github.com>

* Update RELEASE.md

Co-authored-by: Antony Milne <49395058+AntonyMilneQB@users.noreply.github.com>

* Convert context to frozen class and further clean up

Signed-off-by: noklam <nok.lam.chan@quantumblack.com>

* Update RELEASE.md

Co-authored-by: Antony Milne <49395058+AntonyMilneQB@users.noreply.github.com>

* Update kedro/framework/context/context.py

Co-authored-by: Antony Milne <49395058+AntonyMilneQB@users.noreply.github.com>

* Update kedro/framework/context/context.py

Co-authored-by: Antony Milne <49395058+AntonyMilneQB@users.noreply.github.com>

* attempt to fix slow window install issue

Signed-off-by: noklam <nok.lam.chan@quantumblack.com>

* fix requirement.txt

* fix requirement

* remove the protected variable `self._config_loader`

Signed-off-by: noklam <nok.lam.chan@quantumblack.com>

Co-authored-by: Antony Milne <49395058+AntonyMilneQB@users.noreply.github.com>
Signed-off-by: noklam <nok.lam.chan@quantumblack.com>
[AUTO-MERGE] Merge main into develop via merge-main-to-develop
merelcht and others added 6 commits June 29, 2022 17:35
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
# Conflicts:
#	kedro/framework/cli/starters.py
#	tests/framework/cli/test_starters.py
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
@datajoely
Copy link
Contributor

If at all useful, this is how twine resolves a similar problem https://github.com/pypa/twine/blob/8f5e5d6d42d582ef3ea6ef07da277e0cabd22fd2/twine/package.py#L51

@noklam
Copy link
Contributor

noklam commented Jun 30, 2022

One minor comment, when user input an invalid prompt. It will throw an error ValueError: new project!!!! at the bottom, but a more informative message is printed above the stack trace. Maybe they can all be combined in the ValueError? I don't know if we did this before for the coloring, but with the current Rich coloring I think we don't need both.

'new project!!!!' is an invalid value.
It must contain only alphanumeric symbols and/or underscores and/or hyphens.
    def validate(self, user_input: str) -> None:
        """Validate a given prompt value against the regex validator"""
        if self.regexp and not re.match(self.regexp, user_input):
            click.secho(f"'{user_input}' is an invalid value.", fg="red", err=True)
            click.secho(self.error_message, fg="red", err=True)
            raise ValueError(user_input)

If you scroll up, you see this.
image

At the bottom, it shows this
image

Copy link
Contributor

@noklam noklam left a comment

Choose a reason for hiding this comment

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

Nice work! This makes it easier for the beginners.

@merelcht
Copy link
Member Author

merelcht commented Jul 4, 2022

One minor comment, when user input an invalid prompt. It will throw an error ValueError: new project!!!! at the bottom, but a more informative message is printed above the stack trace. Maybe they can all be combined in the ValueError? I don't know if we did this before for the coloring, but with the current Rich coloring I think we don't need both.

'new project!!!!' is an invalid value.
It must contain only alphanumeric symbols and/or underscores and/or hyphens.
    def validate(self, user_input: str) -> None:
        """Validate a given prompt value against the regex validator"""
        if self.regexp and not re.match(self.regexp, user_input):
            click.secho(f"'{user_input}' is an invalid value.", fg="red", err=True)
            click.secho(self.error_message, fg="red", err=True)
            raise ValueError(user_input)

If you scroll up, you see this. image

At the bottom, it shows this image

Ah good catch! The stacktrace doesn't really add anything and makes it quite a bit harder to find our own message. I'll change it.

Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
@merelcht merelcht changed the base branch from develop to main July 4, 2022 16:12
merelcht and others added 4 commits July 4, 2022 17:12
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.

This looks great, so much more seamless! 🎉

Copy link
Contributor

@AhdraMeraliQB AhdraMeraliQB left a comment

Choose a reason for hiding this comment

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

Great work on this! 🌟

Copy link
Contributor

@antonymilne antonymilne left a comment

Choose a reason for hiding this comment

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

Looks good! Just one small question.

Also I think it might be nice just to tidy cookiecutter.json up a bit. It is a constant source of minor irritation to me that it's not quite consistent 😅 Here's what we currently have:

    "repo_name": "{{ cookiecutter.project_name.replace(' ', '-').lower().strip('-') }}",
    "python_package": "{{ cookiecutter.project_name.replace(' ', '_').replace('-', '_').lower() }}",

and what I would suggest:

    "repo_name": "{{ cookiecutter.project_name.strip().replace(' ', '-').replace('_', '-').lower() }}",
    "python_package": "{{ cookiecutter.project_name.strip().replace(' ', '_').replace('-', '_').lower() }}",

Comment on lines 358 to 362
extra_context = cookiecutter_args.get("extra_context", {})
project_name = extra_context.get("project_name", "")
python_package = extra_context.get(
"python_package", project_name.lower().replace(" ", "_").replace("-", "_")
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would extra_context not exist here, and what happens in that case? 🤔 project_name would just be an empty string which doesn't seem right.

Why can't we just do project_name = extra_context["project_name"] and python_package = extra_context["python_package"]?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair point! I guess I was just being overly cautious but it might be better if this throws an error if there's no extra_context because then something would be really wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't do python_package = extra_context["python_package"] because the extra_context will only contain the values that have come in through prompts + kedro_version because it's set explicitly or from config.

Copy link
Member Author

@merelcht merelcht Jul 6, 2022

Choose a reason for hiding this comment

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

Oohh and actually I forgot about this, but we added in backward compatibility for when there's no prompts.yml file. Then you can still run kedro new and it will create a new project with "New Kedro Project" as project name which is the default in cookiecutter.json. So project_name = extra_context["project_name"] would break that and we should instead do extra_context.get("project_name", "New Kedro Project")

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@antonymilne antonymilne Jul 6, 2022

Choose a reason for hiding this comment

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

Thanks for checking this! I think I understand. We should definitely see if we can get rid of the prompts.yml file at some point though, it is pretty confusing 😬

merelcht and others added 2 commits July 6, 2022 12:25
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
@merelcht merelcht merged commit a026e2e into main Jul 6, 2022
@merelcht merelcht deleted the feature/simplify-kedro-new branch July 6, 2022 12:42
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.

[KED-3025] Simplify kedro new workflow into one question
7 participants