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

Explicitly set the environment defaults in settings.py #3326

Merged
merged 4 commits into from
Nov 22, 2023

Conversation

merelcht
Copy link
Member

@merelcht merelcht commented Nov 21, 2023

Description

I think Remove the environment default for ConfigLoader might have an unexpected side effect. If you set CONFIG_LOADER_ARGS it will override the default set in _ProjectSettings and so it will override base_env and default_run_env. I noticed this because in the starters using spark we have:

CONFIG_LOADER_ARGS = {
#       "base_env": "base",
#       "default_run_env": "local",
    "config_patterns": {
        "spark": ["spark*", "spark*/**"],
    }
}

If I don’t uncomment base/default env, I will get an error saying it can’t find spark config inside myproject/conf: https://app.circleci.com/pipelines/github/kedro-org/kedro-starters/889/workflows/f1376cb4-be57-4429-8a1b-a8b9a3a31150/jobs/10413

Development notes

Option 1:
Make sure the env defaults don't get overwritten when not explictly a…
This is a fix to check inside the session if base_env or default_run_env has been specified and if not it shouldn’t overwrite the default.

Option 2:
7ad31c8
Alternatively, we can just add the default base_env and default_run_env to settings.py. If a user removes that, it's less unexpected that this will change the behaviour IMO. That way we don't need to change the session.

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

@merelcht merelcht self-assigned this Nov 21, 2023
@merelcht merelcht requested a review from noklam November 21, 2023 16:53
@merelcht merelcht requested a review from astrojuanlu November 21, 2023 16:59
@merelcht merelcht requested a review from idanov November 21, 2023 17:11
Signed-off-by: Merel Theisen <[email protected]>
@merelcht merelcht changed the title Don't overwrite the environment defaults when not added in settings.py Explicitly set the environment defaults in settings.py Nov 22, 2023
@merelcht merelcht requested a review from noklam November 22, 2023 12:40
@merelcht
Copy link
Member Author

My preference goes to option 2 in the PR description, because IMO it’s more transparent to the user what happens here. No more magic treating of base and local environment.

@merelcht merelcht requested a review from DimedS November 22, 2023 12:41
Copy link
Member

@DimedS DimedS left a comment

Choose a reason for hiding this comment

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

Option 2 also seems to me more clear, thanks for PR @merelcht

@astrojuanlu
Copy link
Member

Alternatively, we can just add the default base_env and default_run_env to settings.py. If a user removes that, it's less unexpected that this will change the behaviour IMO. That way we don't need to change the session.

+1

Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

💯 "Explicit is better than implicit"

@merelcht merelcht merged commit f3fbce7 into develop Nov 22, 2023
28 checks passed
@merelcht merelcht deleted the fix-overwriting-of-env-defaults branch November 22, 2023 16:05
stichbury pushed a commit that referenced this pull request Nov 28, 2023
Signed-off-by: Merel Theisen <[email protected]>
Signed-off-by: Jo Stichbury <[email protected]>
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.

4 participants