Add fallback to starters pull on kedro new #3900

lrcouto · 2024-05-29T04:30:31Z

Description

During our current project creation and test setup, Kedro looks up to the kedro-starters repo and always uses the latest released version to get project templates. This can cause a problem during releases that depends on changes on the kedro-starters repo, as they won't be acknowledged by the current flow until after they are released.

This PR implements a fallback for when this situation happens. When the version of Kedro installed on your environment does not match the latest kedro-startersrelease, it will pull the main branch instead.

Development notes

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

Signed-off-by: lrcouto <laurarccouto@gmail.com>

lrcouto · 2024-06-05T04:58:11Z

A couple notes:

I've used the requests_mock library to run tests for this PR because I was having a lot of trouble mocking requests to the Github API with fixtures we already had.
In agreement with this response on Slack, I've preferred to not touch the kedro checkout function, since it is user-facing and this fallback feature is just for us.

merelcht

The approach looks good to me 👍 I left some small suggestions.

kedro/framework/cli/starters.py

Signed-off-by: lrcouto <laurarccouto@gmail.com>

kedro/framework/cli/starters.py

Signed-off-by: lrcouto <laurarccouto@gmail.com>

ElenaKhaustova

Thank you, @lrcouto, like the solution!

Left one question and suggestion.

ElenaKhaustova · 2024-06-07T12:21:24Z

kedro/framework/cli/starters.py

@@ -773,7 +812,7 @@ def _make_cookiecutter_args_and_fetch_template(

    tools = config["tools"]
    example_pipeline = config["example_pipeline"]
-    starter_path = "git+https://github.com/kedro-org/kedro-starters.git"
+    starter_path = _STARTERS_REPO


Cannot comment on the proper line as it was not changed but the suggestion is what if we do not introduce the starter_path variable as it complicates the logic? So instead we can do:

else: # Use the default template path for non PySpark, Viz or example options: return cookiecutter_args, template_path return cookiecutter_args, _STARTERS_REPO

That's a good point. To be honest, I'm not sure why this variable was there in the first place.

EDIT: Reading the code now, the starter_path variable might not aways be the same as the default _STARTERS_REPO, as the value may be passed through the template_path parameter.

Yes, I've seen it changed but I still prefer to return the original one instead. See the edited suggestion (I've missed a "return" and looked confusing)

ElenaKhaustova · 2024-06-07T12:30:51Z

kedro/framework/cli/starters.py

+_STARTERS_REPO = (
+    "git+https://github.com/kedro-org/kedro-starters.git"
+    if _kedro_and_starters_version_identical()
+    else "https://github.com/kedro-org/kedro-starters.git@main"


What happens if we point to the main branch (_STARTERS_REPO="https://github.com/kedro-org/kedro-starters.git@main") but then below in _make_cookiecutter_args_and_fetch_template we set the checkout version as well? For example in this case:

if "PySpark" in tools and "Kedro Viz" in tools: # Use the spaceflights-pyspark-viz starter if both PySpark and Kedro Viz are chosen. cookiecutter_args["directory"] = "spaceflights-pyspark-viz" # Ensures we use the same tag version of kedro for kedro-starters cookiecutter_args["checkout"] = version

Will it then work as we expect I mean creating a project and passing the branch and the checkout version to the cookiecutter?

kedro/kedro/framework/cli/starters.py

Line 932 in 15ba4da

def _create_project(

This will lead us to cookiecutter shenanigans, which is where these parameters are ultimately used. You have to dig down four or five functions to see it, but cookiecutter is using the template parameter to determine the repo address and the checkout parameter to determine the branch, tag or commit ID. What it does is the following:

if clone: try: subprocess.check_output( # nosec [repo_type, 'clone', repo_url], cwd=clone_to_dir, stderr=subprocess.STDOUT, ) if checkout is not None: checkout_params = [checkout] # Avoid Mercurial "--config" and "--debugger" injection vulnerability if repo_type == "hg": checkout_params.insert(0, "--") subprocess.check_output( # nosec [repo_type, 'checkout', *checkout_params], cwd=repo_dir, stderr=subprocess.STDOUT, )

So if you passed the repo_url as the address to the main branch and, for example, passed the checkout value as a different branch, cookiecutter would clone the main branch and then checkout to the other branch.

Thank you for explaining, that's what I thought as well. Doesn't it mean that in this case instead of main branch (which we aim for) we again will have the released branch (as the checkout argument is the installed kedro version in case it's not passed) which is not desired?

kedro/kedro/framework/cli/starters.py

Line 344 in 15ba4da

checkout = checkout or version

That's a good point, let me test that.

Yeah I think you're right. The stuff passed to cookiecutter has to be formatted in a different way.

I was thinking of refactoring the logic in _make_cookiecutter_args_and_fetch_template because now we have two places where we define the template which is a bit confusing. But that can be done in a separate PR.

Now, we can just identify the case when repos versions are not matched (as you do for setting the _STARTERS_REPO). In this case, we only need to use the template path provided but skip setting up the checkout parameter in _make_cookiecutter_args_and_fetch_template.

Or we can just do something like

checkout = checkout or version if _kedro_and_starters_version_identical() else None

kedro/kedro/framework/cli/starters.py

Line 344 in 15ba4da

checkout = checkout or version

Edit: updated link

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

Signed-off-by: lrcouto <laurarccouto@gmail.com>

ElenaKhaustova

Thank you for the updates!

One of the concerns which is still in my head is how we use checkout argument provided with version and the way we change the related logic between _get_cookiecutter_dir and _make_cookiecutter_args_and_fetch_template methods. I would suggest to stick to common approach if possible, otherwise it's a bit confusing to follow the logic and some corner cases are appearing.

Once we're done I would also suggest to update the release check list to clarify how it works now.

Happy to help with the above.

ElenaKhaustova · 2024-06-24T16:42:01Z

kedro/framework/cli/starters.py

    if directory:
        cookiecutter_args["directory"] = directory

    tools = config["tools"]
    example_pipeline = config["example_pipeline"]
    starter_path = "git+https://github.com/kedro-org/kedro-starters.git"
+    checkout_version = version if kedro_version_match_starters else "main"


Are we sure that the checkout key should be different for the cases below? In my understanding, we should:

Use the checkout argument passed

In case the checkout is None go with version if kedro version matches the starters

Use main otherwise

Now we're mixing checkout and version which is a bit confusing. Please correct me if I'm missing any other logic behind it or if there's any particular reason why we don't want to use the provided checkout argument in case some tools are selected.

ElenaKhaustova · 2024-06-24T16:47:36Z

kedro/framework/cli/starters.py

    else:
        # Use the default template path for non PySpark, Viz or example options:
        starter_path = template_path
-
+        cookiecutter_args["checkout"] = (


Related to the previous comment.

If the checkout argument is provided and kedro version doesn't match starters, do we indeed want to sick to main?

In this case, there's also a discrepancy with _get_cookiecutter_dir method, where we use the provided checkout argument.

DimedS

Thank you for the PR, @lrcouto!

I have a few questions regarding it:

I'm trying to understand the algorithm in this PR. Could you please help with that?

In my opinion, we need to check the following:

If current_kedro_version is not in the kedro_starters_tags_list, we should not use checkout in the starters repo (since that tag won't exist). Instead, we should use the default starters repo.
Otherwise, if current_kedro_version is in the list, we should set kedro_starters_tag to current_kedro_version.

From what I see in the current PR, we are checking current_kedro_version against latest_kedro_starters_tag. If they don't match, we use the default repo. It seems that if a user is using an older version of Kedro, it won't match with latest_kedro_starters_tag, and the user will receive the latest default repo. However, they should receive the version of starters that matches current_kedro_version (I believe this is a current logic that we don't want to change).

Why are we using os.environ["KEDRO_STARTERS_VERSION"] to store the latest starters version? Why isn't a global variable sufficient?

lrcouto · 2024-06-25T20:15:28Z

Thank you for your reviews, @ElenaKhaustova and @DimedS ! From what I'm getting, the logic of the version/branch/tag selection for starters itself is a little confusing, so I'm gonna look into refactoring it a little bit.

As for the question regarding the environment variable, it came from this discussion. We were having problems with repeated requests to Github being rejected, which happened every time tests were ran on out CI. My idea is that setting it on the environment once and then checking it would avoid this large number of requests, and it would not need to be re-declared every time the file is loaded.

Signed-off-by: lrcouto <laurarccouto@gmail.com>

lrcouto · 2024-07-02T15:52:08Z

If current_kedro_version is not in the kedro_starters_tags_list, we should not use checkout in the starters repo (since that tag won't exist). Instead, we should use the default starters repo. Otherwise, if current_kedro_version is in the list, we should set kedro_starters_tag to current_kedro_version.

Regarding this point, would we have to get all of the existing Kedro versions from Git? Or does that exist already somewhere? I'm asking because it'd be another request that we'd have to make.

DimedS · 2024-07-03T15:42:48Z

If current_kedro_version is not in the kedro_starters_tags_list, we should not use checkout in the starters repo (since that tag won't exist). Instead, we should use the default starters repo. Otherwise, if current_kedro_version is in the list, we should set kedro_starters_tag to current_kedro_version.

Regarding this point, would we have to get all of the existing Kedro versions from Git? Or does that exist already somewhere? I'm asking because it'd be another request that we'd have to make.

I believe we can use the latest_kedro_starters_tag that you already received and compare it with the current_kedro_version. If the current_kedro_version is greater than the latest_kedro_starters_tag (indicating that Kedro has been released with a new version, but the starters have not yet been updated), then we should use the default starters repository (i.e., the latest starters from the main branch). Otherwise, we should maintain the current logic and use the same version of starters that matches the Kedro version.

Signed-off-by: lrcouto <laurarccouto@gmail.com>

…kedro into fallback-to-starters-git-pull

merelcht

I left one more question, but otherwise the approach with checking if the version is smaller or equal to the starters version looks good!

merelcht · 2024-07-09T10:49:28Z

kedro/framework/cli/starters.py

    else:
        # Use the default template path for non PySpark, Viz or example options:
        starter_path = template_path
-
+        cookiecutter_args["checkout"] = (


Shouldn't this just be cookiecutter_args["checkout"] = checkout_version as well?

DimedS

Thank you for changing the logic, @lrcouto. It looks mostly good now! I've left a few additional questions.

DimedS · 2024-07-09T15:52:10Z

kedro/framework/cli/starters.py

+_STARTERS_REPO = (
+    "git+https://github.com/kedro-org/kedro-starters.git"
+    if _kedro_version_equal_or_lower_to_starters(version)
+    else "https://github.com/kedro-org/kedro-starters.git@main"


Could you please comment why do we need that two lines here now:

if _kedro_version_equal_or_lower_to_starters(version) else "https://github.com/kedro-org/kedro-starters.git@main"

DimedS · 2024-07-09T15:55:07Z

kedro/framework/cli/starters.py

    elif "Kedro Viz" in tools:
        # Use the spaceflights-pandas-viz starter if only Kedro Viz is chosen.
        cookiecutter_args["directory"] = "spaceflights-pandas-viz"
+        cookiecutter_args["checkout"] = checkout_version


I agree with Merel, it's better to put cookiecutter_args["checkout"] = checkout_version on top and write it only once, if we can handle that section properly:

else: # Use the default template path for non PySpark, Viz or example options: starter_path = template_path

could you please explain checkout logic with that part, what should we do here in terms of checkout? As I understood here we are taking standard kedro template from kedro repo?

DimedS · 2024-07-09T16:01:51Z

kedro/framework/cli/starters.py

+            logging.error(f"Error fetching kedro-starters latest release version: {e}")
+            return ""
+
+        os.environ["KEDRO_STARTERS_VERSION"] = latest_release["tag_name"]


We started discussing the use of environment variables here to optimise time for different runs, such as in CI/CD pipelines. However, I read the following about os.environ:

The environment variable set using os.environ within a Python script is only set for the duration of the current process (i.e., the script execution) and does not persist beyond that. It will not be available to other processes or sessions, and once the script finishes executing, the environment variable will be lost.

If this is correct, it doesn't make sense to set this type of environment variable. Have you tested that it works?

ElenaKhaustova · 2024-07-09T17:31:35Z

kedro/framework/cli/starters.py

    if directory:
        cookiecutter_args["directory"] = directory

    tools = config["tools"]
    example_pipeline = config["example_pipeline"]
    starter_path = "git+https://github.com/kedro-org/kedro-starters.git"

+    if checkout:


Thank you for making setting the checkout_version logic cleaner. Just to double check: shouldn't we differ checkout and version here? if yes, above we do the following:

checkout = checkout or version

kedro/kedro/framework/cli/starters.py

Line 345 in 9ca8844

checkout = checkout or version

Do we expect here that checkout is True because it is set to version?

Can this logic be global within new command as well as selection on starter_path?

Signed-off-by: lrcouto <laurarccouto@gmail.com>

lrcouto · 2024-07-10T23:06:30Z

Pushed some changes based on the conversation earlier today with @ElenaKhaustova and @DimedS.

The logic that selects which checkout argument will be passed to the Cookiecutter function was moved to is own function.
_STARTERS_REPO now points only to the main starters repo.
Removed redundant logic from _make_cookiecutter_args_and_fetch_template.

merelcht

LGTM 👍

DimedS

Thank you for the PR and addressing comment, @lrcouto ! Looks nice without that cookiecutter_args["checkout"] = version repetition in every if else line.

lrcouto and others added 12 commits May 29, 2024 01:29

Add fallback to starters pull on kedro new

48064c5

Signed-off-by: lrcouto <laurarccouto@gmail.com>

Merge branch 'main' into fallback-to-starters-git-pull

791e821

Lint

2ffa441

Signed-off-by: lrcouto <laurarccouto@gmail.com>

Add types-requests for mypy

4bd7ec9

Signed-off-by: lrcouto <laurarccouto@gmail.com>

allow checkout flag to be used for different starters version

41c7a60

Signed-off-by: lrcouto <laurarccouto@gmail.com>

Attempt to fix e2e tests

aa42e54

Signed-off-by: lrcouto <laurarccouto@gmail.com>

Store starters version on env variable

11e599b

Signed-off-by: lrcouto <laurarccouto@gmail.com>

Add unit tests

a53b459

Signed-off-by: lrcouto <laurarccouto@gmail.com>

Fix unit tests

b1b03ea

Signed-off-by: lrcouto <laurarccouto@gmail.com>

Add requests_mock

6aca1f4

Signed-off-by: lrcouto <laurarccouto@gmail.com>

Catch exception if request fails

2572d3d

Signed-off-by: lrcouto <laurarccouto@gmail.com>

Lint

6867f92

Signed-off-by: lrcouto <laurarccouto@gmail.com>

lrcouto marked this pull request as ready for review June 5, 2024 04:44

lrcouto requested a review from merelcht as a code owner June 5, 2024 04:44

merelcht reviewed Jun 5, 2024

View reviewed changes

kedro/framework/cli/starters.py Outdated Show resolved Hide resolved

kedro/framework/cli/starters.py Outdated Show resolved Hide resolved

Change error condition on _get_latest_starters_version

5088d84

Signed-off-by: lrcouto <laurarccouto@gmail.com>

lrcouto requested review from astrojuanlu, DimedS, ElenaKhaustova and ankatiyar June 5, 2024 16:00

lrcouto linked an issue Jun 5, 2024 that may be closed by this pull request

Implement version fallback for starters & framework #3879

Closed

astrojuanlu reviewed Jun 6, 2024

View reviewed changes

kedro/framework/cli/starters.py Outdated Show resolved Hide resolved

lrcouto and others added 2 commits June 6, 2024 12:13

Merge branch 'main' into fallback-to-starters-git-pull

18790ad

Change env variable name to be more specific

15ba4da

Signed-off-by: lrcouto <laurarccouto@gmail.com>

ElenaKhaustova reviewed Jun 7, 2024

View reviewed changes

astrojuanlu mentioned this pull request Jun 10, 2024

Investigate how we can remove Kedro's dependency on git (for starters) #2051

Closed

lrcouto and others added 3 commits June 12, 2024 12:26

Merge branch 'main' into fallback-to-starters-git-pull

d2358af

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

Merge branch 'main' into fallback-to-starters-git-pull

c3bc522

Set all starter options to pull from main if versions don't match

2a3572f

Signed-off-by: lrcouto <laurarccouto@gmail.com>

ElenaKhaustova reviewed Jun 24, 2024

View reviewed changes

DimedS reviewed Jun 25, 2024

View reviewed changes

Merge branch 'main' into fallback-to-starters-git-pull

486671d

lrcouto and others added 6 commits June 27, 2024 12:47

Merge branch 'main' into fallback-to-starters-git-pull

3e35466

Merge branch 'main' into fallback-to-starters-git-pull

92b0058

Changes on logic for fetching templates

4077bad

Signed-off-by: lrcouto <laurarccouto@gmail.com>

Lint

c7b261d

Signed-off-by: lrcouto <laurarccouto@gmail.com>

Update test to match changes on template fetching

4ee2587

Signed-off-by: lrcouto <laurarccouto@gmail.com>

Merge branch 'main' into fallback-to-starters-git-pull

5cd8dc7

lrcouto and others added 5 commits July 4, 2024 10:00

Merge branch 'main' into fallback-to-starters-git-pull

715afe6

Merge branch 'main' into fallback-to-starters-git-pull

c30e6eb

Merge branch 'main' into fallback-to-starters-git-pull

eaacab8

Change logic of version comparison, add tests

c279c6a

Signed-off-by: lrcouto <laurarccouto@gmail.com>

Merge branch 'fallback-to-starters-git-pull' of github.com:kedro-org/…

d6246b3

…kedro into fallback-to-starters-git-pull

merelcht reviewed Jul 9, 2024

View reviewed changes

Merge branch 'main' into fallback-to-starters-git-pull

9ca8844

DimedS reviewed Jul 9, 2024

View reviewed changes

ElenaKhaustova reviewed Jul 9, 2024

View reviewed changes

lrcouto and others added 3 commits July 10, 2024 10:05

Merge branch 'main' into fallback-to-starters-git-pull

34e60ba

Merge branch 'main' into fallback-to-starters-git-pull

5a9142c

Extract checkout logic to its own function

8e4c8f0

Signed-off-by: lrcouto <laurarccouto@gmail.com>

merelcht approved these changes Jul 15, 2024

View reviewed changes

DimedS approved these changes Jul 15, 2024

View reviewed changes

lrcouto merged commit 1aea4a3 into main Jul 15, 2024
34 checks passed

lrcouto deleted the fallback-to-starters-git-pull branch July 15, 2024 15:47

ElenaKhaustova mentioned this pull request Aug 1, 2024

Fix cli starters tests #4052

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fallback to starters pull on kedro new #3900

Add fallback to starters pull on kedro new #3900

lrcouto commented May 29, 2024 •

edited

Loading

lrcouto commented Jun 5, 2024

merelcht left a comment

ElenaKhaustova left a comment

ElenaKhaustova Jun 7, 2024 •

edited

Loading

lrcouto Jun 7, 2024 •

edited

Loading

ElenaKhaustova Jun 7, 2024

ElenaKhaustova Jun 7, 2024

lrcouto Jun 7, 2024

ElenaKhaustova Jun 7, 2024 •

edited

Loading

lrcouto Jun 7, 2024

lrcouto Jun 7, 2024

ElenaKhaustova Jun 7, 2024

ElenaKhaustova Jun 7, 2024 •

edited

Loading

ElenaKhaustova left a comment

ElenaKhaustova Jun 24, 2024

ElenaKhaustova Jun 24, 2024

DimedS left a comment

lrcouto commented Jun 25, 2024

lrcouto commented Jul 2, 2024

DimedS commented Jul 3, 2024

merelcht left a comment

merelcht Jul 9, 2024

DimedS left a comment

DimedS Jul 9, 2024 •

edited

Loading

DimedS Jul 9, 2024

DimedS Jul 9, 2024

ElenaKhaustova Jul 9, 2024 •

edited

Loading

ElenaKhaustova Jul 9, 2024 •

edited

Loading

lrcouto commented Jul 10, 2024

merelcht left a comment

DimedS left a comment

Add fallback to starters pull on kedro new #3900

Add fallback to starters pull on kedro new #3900

Conversation

lrcouto commented May 29, 2024 • edited Loading

Description

Development notes

Developer Certificate of Origin

Checklist

lrcouto commented Jun 5, 2024

merelcht left a comment

Choose a reason for hiding this comment

ElenaKhaustova left a comment

Choose a reason for hiding this comment

ElenaKhaustova Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

lrcouto Jun 7, 2024 • edited Loading

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

ElenaKhaustova Jun 7, 2024 • edited Loading

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

ElenaKhaustova Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

ElenaKhaustova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DimedS left a comment

Choose a reason for hiding this comment

lrcouto commented Jun 25, 2024

lrcouto commented Jul 2, 2024

DimedS commented Jul 3, 2024

merelcht left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DimedS left a comment

Choose a reason for hiding this comment

DimedS Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElenaKhaustova Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

ElenaKhaustova Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

lrcouto commented Jul 10, 2024

merelcht left a comment

Choose a reason for hiding this comment

DimedS left a comment

Choose a reason for hiding this comment

lrcouto commented May 29, 2024 •

edited

Loading

ElenaKhaustova Jun 7, 2024 •

edited

Loading

lrcouto Jun 7, 2024 •

edited

Loading

ElenaKhaustova Jun 7, 2024 •

edited

Loading

ElenaKhaustova Jun 7, 2024 •

edited

Loading

DimedS Jul 9, 2024 •

edited

Loading

ElenaKhaustova Jul 9, 2024 •

edited

Loading

ElenaKhaustova Jul 9, 2024 •

edited

Loading