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

Add env_str resolver built-in resolver #383

Closed
omry opened this issue Sep 21, 2020 · 10 comments · Fixed by #606
Closed

Add env_str resolver built-in resolver #383

omry opened this issue Sep 21, 2020 · 10 comments · Fixed by #606
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@omry
Copy link
Owner

omry commented Sep 21, 2020

In some cases the automatic conversion of environment variable does not work well or is not what the user wants.
A simple fix is to add another resolver named "env_str" that would return the env string as is.

@omry
Copy link
Owner Author

omry commented Feb 3, 2021

Blocked on #445. Depending on the final decision there this may not be needed.

@omry omry added the enhancement New feature or request label Feb 3, 2021
@odelalleau
Copy link
Collaborator

Maybe we can discuss here wether or not we want env to parse and convert env variables?

My suggestion: keep them as strings, and add a new default resolver ("eval"? "parse"? "resolve"? "process"? "autoconvert"? ...) that does this step. This way, one could always use ${eval:${env:VAR}} if they want to convert the string.

@omry
Copy link
Owner Author

omry commented Feb 6, 2021

I like it.

With the possible support of consulting the annotations (#506), I think we will probably not need this in most cases anyway.
The annotation is a more precise way to express the type you actually want as opposed to the provided type. (and automatically convert it in some cases, like from an int 4 to a string "4".

@odelalleau
Copy link
Collaborator

With the possible support of consulting the annotations (#506), I think we will probably not need this in most cases anyway.
The annotation is a more precise way to express the type you actually want as opposed to the provided type. (and automatically convert it in some cases, like from an int 4 to a string "4".

Agreed. I'm going to adapt #445 accordingly, and add the new resolver in a different PR. Do you have a preferred name for that one?

@omry
Copy link
Owner Author

omry commented Feb 6, 2021

I was thinking to change the behavior to not parse in the existing env resolver.
It's a breaking change but a small one.
did you have anything else in mind?

Edit: you are talking about the parsing resolver.
maybe parse or decode. (might have a stronger opinion might I see what it looks like).

Collisions are still unlikely but maybe we should use a common prefix for built in resolvers:
oc.env, oc.decode etc (and deprecate the existing env one, telling people to use oc.env).

@odelalleau
Copy link
Collaborator

Collisions are still unlikely but maybe we should use a common prefix for built in resolvers:
oc.env, oc.decode etc (and deprecate the existing env one, telling people to use oc.env).

Ok sounds good, will do that in another PR afterwards. . is not a valid character for resolver names right now though, do you prefer to add it or to use oc_env / oc_decode instead?

@omry
Copy link
Owner Author

omry commented Feb 7, 2021

I think it would be nice support some more formal separator for namespacing.
I actually ran into this need recently too in Hydra, can't remember the details though.

@omry
Copy link
Owner Author

omry commented Feb 10, 2021

Since we are planning to break compatibility here, I think the best plan is to deprecate the current env resolver while keeping it as is in terms of behavior and switch people to use oc.env which can also have a new behavior.

@odelalleau
Copy link
Collaborator

Since we are planning to break compatibility here, I think the best plan is to deprecate the current env resolver while keeping it as is in terms of behavior and switch people to use oc.env which can also have a new behavior.

Makes sense. Should I do that in #445? (it would be more convenient if I want to close #230 with it)

@omry
Copy link
Owner Author

omry commented Feb 11, 2021

let's not add anything else to 445.
#230 has nothing to do with it, it's more of a bugfix.

We can do it in a new PR closing this one (after editing this one to reflect what we are actually doing).

@omry omry removed the blocked label Feb 22, 2021
odelalleau added a commit to odelalleau/omegaconf that referenced this issue Mar 15, 2021
* Restore and deprecate the old `env` resolver for backward
  compatibility with OmegaConf 2.0

* The new `oc.env` resolver keeps the string representation of
  environment variables, and does not use the cache

* The new `oc.decode` resolver can be used to parse and evaluate strings
  according to the OmegaConf grammar

Fixes omry#383
Fixes omry#573
Fixes omry#574
odelalleau added a commit to odelalleau/omegaconf that referenced this issue Mar 15, 2021
* Restore and deprecate the old `env` resolver for backward
  compatibility with OmegaConf 2.0

* The new `oc.env` resolver keeps the string representation of
  environment variables, and does not use the cache

* The new `oc.decode` resolver can be used to parse and evaluate strings
  according to the OmegaConf grammar

Fixes omry#383
Fixes omry#573
Fixes omry#574
odelalleau added a commit to odelalleau/omegaconf that referenced this issue Mar 18, 2021
* Restore and deprecate the old `env` resolver for backward
  compatibility with OmegaConf 2.0

* The new `oc.env` resolver keeps the string representation of
  environment variables, and does not use the cache

* The new `oc.decode` resolver can be used to parse and evaluate strings
  according to the OmegaConf grammar

Fixes omry#383
Fixes omry#573
Fixes omry#574
odelalleau added a commit that referenced this issue Mar 18, 2021
* Introduce new `oc.env` and `oc.decode` resolvers

* Restore and deprecate the old `env` resolver for backward
  compatibility with OmegaConf 2.0

* The new `oc.env` resolver keeps the string representation of
  environment variables, does not use the cache, and accepts None as
  default value

* The new `oc.decode` resolver can be used to parse and evaluate strings
  according to the OmegaConf grammar

Fixes #383
Fixes #573
Fixes #574

* Allow `oc.decode` to evaluate interpolations

* Improve documentation of `oc.decode`

In particular it explains that dictionaries and lists are automatically
converted to transient config nodes.

* Update docs/source/usage.rst

Co-authored-by: Omry Yadan <[email protected]>

* Update news/573.api_change

Co-authored-by: Omry Yadan <[email protected]>

* Update omegaconf/_utils.py

Co-authored-by: Omry Yadan <[email protected]>

* Update omegaconf/_utils.py

Co-authored-by: Omry Yadan <[email protected]>

* Remove the USERID example

* Show example of quoted string as default value for `oc.env`

* Remove duplicated comments (# #)

* Validate default value of `oc.env` even when not used

* Simplify tests with recwarn

* Use convenience `show()` function in doc to show type and value

* Update doc on string interpolations

* More readable test formatting

* Improve comment formatting

* Restore interpolation examples

* Update docs/notebook/Tutorial.ipynb

Co-authored-by: Omry Yadan <[email protected]>

* Update docs/source/usage.rst

Co-authored-by: Omry Yadan <[email protected]>

* Update docs/source/usage.rst

Co-authored-by: Omry Yadan <[email protected]>

* Rephrasing in doc

* Use `show()` function in doc

* Raise a KeyError instead of ValidationError for missing env variables

* Remove handling of "null" as default in legacy env resolver

* Update news

* Explicit typing for the default value of the `oc.env` resolver

* Use a more appropriate exception type

* Update tests/test_interpolation.py

Co-authored-by: Omry Yadan <[email protected]>

* Safer markers for default values

* Fix coverage

* Use more appropriate TypeError

* Refactor: consistent use of _DEFAULT_MARKER_

Co-authored-by: Omry Yadan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants