-
Notifications
You must be signed in to change notification settings - Fork 906
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
Remove the environment default for ConfigLoader #2971
Comments
We should also make sure it works even if user provide a relative path. To do that we may need to do a
|
The error is confusing if user define a base_env="." and default_run_env="local". This happen because "." will catch both Noted that duplicate value from other environment is OKAY, this is the purpose of the environment to override stuff like |
On the other hand, can we remove the assumption that users will have two environment all the time? (For standalone use case, framework will not be changed). The above error message can point to an more explicit error, the solution is still a bit "hacky" as we simply set Could we make it work with a single environment? (framework will default to use base and local, no change) @idanov Related to this issue: |
I'm in favour of removing the One concern I have though is around the user experience when moving from just using the
I have a preference for keeping the two configuration environment setup by default inside the framework, because it's aligned with the best practices that Kedro promises to introduce to ML projects. Option 3: Actually, while writing this I realised, perhaps @noklam 's first option in the ticket description: Change defaults to "." and change settings.py default to "base" and "local" , might be the better user experience when it comes to changing from just library components to using the |
Thanks for the analysis @merelcht
Is this option (2) among the three you listed? |
I've updated my previous comment to make my though process clearer, but essentially the way I see it is:
It's a slight nuance, but the difference for a user would be that with option 2 there's no assumption at all about configuration environments: not about the default location (base and local) but also not that there must be 2. With option 3 we keep the the default location as well as the assumption that there are 2 environments, to "teach" users about the best practices of using layered configuration. |
There are 4 possibles pathways:
So I think the key here is how we can help people to transit from library to framework without breaking stuff. It turns into a discussion of whether user "opt-in" to 2 environment or they "opt-out". I imagine the flow from library -> framework will still involve Do we also want to rename the arguments maybe, does (2) is orthogonal to (1), which add the ability to have an environment-less (or just 1 env) |
I'm not so sure. The moment you start populating your directory, |
@astrojuanlu |
To summarize previous discussions, @merelcht 's summary on #3029 (comment) gives the way forward: let's keep the assumption of 2 environments, let's make |
@astrojuanlu Can I ask for an update of this ticket? Which options are we going to implement? I am slightly confused with this statement since this ticket propose removing the default envs.
My answer to this is all YES but it will be good to double check. If we only do 3 but not 2, it will be a breaking change (maybe that's fine). Doing 2 without 3 is fine, it's doesn't change anything but it gives users more information what's the default and easier to change them if needed. |
Not sure what "remove" means in this case, but the end goal is that the user can do
(in a REPL or a script) and it works without
In principle yes. That's what @merelcht said in #3029:
Does it make sense?
As far as I understand, (3) was already decided (see above).
I'm not fully aware of the implications of doing or not doing this.
That's okay but if we can easily avoid it we should try. It will make upgrading to 0.19 from 0.18 easier, right? |
Completed in #3311 |
Context
See #2819 and more
Currently ConfigLoader assume
base
andlocal
exist, but for single config use cases ( a notebook) it's not relevant.Requirements
Current workaround
Potential Options:
settings.py
default to "base" and "local" (and starters)KedroSession.create
I prefer 2. and we can do it now instead of waiting 0.19 cc @merelcht
Originally posted by @noklam in #2819 (comment)
Option 3:
Use
_ProjectSettings
to bake framework assumption.kedro/kedro/framework/project/__init__.py
Lines 102 to 107 in d3171f1
Additionally
We should clean up the test for config when
local
isn't a necessary requirement. This can be consider as a separate ticket but probably best to do it together.The text was updated successfully, but these errors were encountered: