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 option to load traitlets values from environement. #856

Merged
merged 1 commit into from
Nov 27, 2023

Conversation

Carreau
Copy link
Member

@Carreau Carreau commented Aug 22, 2023

For example this makes the following works:

With the env variables:

 MYAPP__Foo__j=42
 MYAPP__Bar__mylist='[4,5,3]'
 MYAPP__Foo__SubConfigurable__subvalue=-1

$ python examples/myapp.py
app.config:
{'Foo': {'j': DeferredConfigString('42'), 'SubConfigurable': {'subvalue': DeferredConfigString('-1')}}, 'Bar': {'mylist': DeferredConfigString('[4,5,3]')}}
I am MyApp with myapp False and 2 sub configurables Foo and bar:
I am Foo with:
    i    = 0
    j    = 42
    name = Brian
    mode = on
I am SubConfigurable with:
    subvalue = -1
I am Bar with:
    enabled =  True
    mylist  =  [4, 5, 3]
I am SubConfigurable with:
    subvalue = 0
try running with --help-all to see all available flags
[MyApp] WARNING | Warning Message
[MyApp] CRITICAL | Critical Message

We use app.name uppercased as a prefix, as usually env variables are uppercase.

We use __ instead of . as a separator as:

  • . is not valid in env variables,
  • _ can be present in class names, so we can't use it.

I'm not going the full way of a config loader, as I don't think it is necessary.

I'm wondering if we should put env loading by default.

This also extend one of the examples to load from env, show the parent=self and describe the status of the various traits at run time instead of just the configuration.

@Carreau
Copy link
Member Author

Carreau commented Aug 22, 2023

@ndmlny-qs FYI.

Comment on lines +975 to +992
for k, v in os.environ.items():
if k.startswith(PREFIX):
self.log.debug('Seeing environ "%s"="%s"', k, v)
# use __ instead of . as separator in env variable.
# Warning, case sensitive !
_, *path, key = k.split("__")
section = new_config
for p in path:
section = section[p]
setattr(section, key, DeferredConfigString(v))
Copy link
Member Author

@Carreau Carreau Aug 22, 2023

Choose a reason for hiding this comment

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

We do not handle errors here, I'm not sure if we should keep going on invalid values.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should error out if an env variable is set to an invalid value.

Copy link
Member Author

@Carreau Carreau Sep 11, 2023

Choose a reason for hiding this comment

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

I tested and we do actually already error in the same way than when we pass an invalid CLI value (MYAPP__Foo__i=a42 or --Foo.i=a42)

TB when invalid value
Traceback (most recent call last):
  File "/Users/bussonniermatthias/dev/traitlets/examples/myapp.py", line 155, in <module>
    main()
  File "/Users/bussonniermatthias/dev/traitlets/examples/myapp.py", line 150, in main
    app.initialize()
  File "/Users/bussonniermatthias/dev/traitlets/examples/myapp.py", line 129, in initialize
    self.init_foo()
  File "/Users/bussonniermatthias/dev/traitlets/examples/myapp.py", line 118, in init_foo
    self.foo = Foo(parent=self)
  File "/Users/bussonniermatthias/dev/traitlets/examples/myapp.py", line 56, in __init__
    super().__init__(**kwargs)
  File "/Users/bussonniermatthias/dev/traitlets/traitlets/config/configurable.py", line 108, in __init__
    self.config = config
  File "/Users/bussonniermatthias/dev/traitlets/traitlets/traitlets.py", line 688, in __set__
    self.set(obj, value)
  File "/Users/bussonniermatthias/dev/traitlets/traitlets/traitlets.py", line 677, in set
    obj._notify_trait(self.name, old_value, new_value)
  File "/Users/bussonniermatthias/dev/traitlets/traitlets/traitlets.py", line 1459, in _notify_trait
    self.notify_change(
  File "/Users/bussonniermatthias/dev/traitlets/traitlets/traitlets.py", line 1471, in notify_change
    return self._notify_observers(change)
  File "/Users/bussonniermatthias/dev/traitlets/traitlets/traitlets.py", line 1518, in _notify_observers
    c(event)
  File "/Users/bussonniermatthias/dev/traitlets/traitlets/traitlets.py", line 1100, in compatible_observer
    return func(self, change)
  File "/Users/bussonniermatthias/dev/traitlets/traitlets/config/configurable.py", line 218, in _config_changed
    self._load_config(change.new, traits=traits, section_names=section_names)
  File "/Users/bussonniermatthias/dev/traitlets/traitlets/config/configurable.py", line 180, in _load_config
    setattr(self, name, deepcopy(config_value))
  File "/Users/bussonniermatthias/dev/traitlets/traitlets/traitlets.py", line 688, in __set__
    self.set(obj, value)
  File "/Users/bussonniermatthias/dev/traitlets/traitlets/traitlets.py", line 662, in set
    new_value = self._validate(obj, value)
  File "/Users/bussonniermatthias/dev/traitlets/traitlets/traitlets.py", line 694, in _validate
    value = self.validate(obj, value)
  File "/Users/bussonniermatthias/dev/traitlets/traitlets/traitlets.py", line 2341, in validate
    self.error(obj, value)
  File "/Users/bussonniermatthias/dev/traitlets/traitlets/traitlets.py", line 798, in error
    raise TraitError(e)
traitlets.traitlets.TraitError: The 'i' trait of a Foo instance expected an int, not the str 'a42'.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, you don't want to pass some security-related option and have it ignored.

For example this makes the following works:

With the env variables:

     MYAPP__Foo__j=42
     MYAPP__Bar__mylist='[4,5,3]'
     MYAPP__Foo__SubConfigurable__subvalue=-1

    $ python examples/myapp.py
    app.config:
    {'Foo': {'j': DeferredConfigString('42'), 'SubConfigurable': {'subvalue': DeferredConfigString('-1')}}, 'Bar': {'mylist': DeferredConfigString('[4,5,3]')}}
    I am MyApp with myapp False and 2 sub configurables Foo and bar:
    I am Foo with:
        i    = 0
        j    = 42
        name = Brian
        mode = on
    I am SubConfigurable with:
        subvalue = -1
    I am Bar with:
        enabled =  True
        mylist  =  [4, 5, 3]
    I am SubConfigurable with:
        subvalue = 0
    try running with --help-all to see all available flags
    [MyApp] WARNING | Warning Message
    [MyApp] CRITICAL | Critical Message

We use app.name uppercased as a prefix, as usually env variables are
uppercase.

We use `__` instead of `.` as a separator as:
  - `.` is not valid in env variables,
  - `_` can be present in class names, so we can't use it.

I'm not going the full way of a config loader, as I don't think it is
necessary.

I'm wondering if we should put env loading by default.
@Carreau Carreau merged commit 4d75046 into ipython:main Nov 27, 2023
29 checks passed
@Carreau Carreau deleted the load_from_env branch November 27, 2023 10:56
@blink1073
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants