-
-
Notifications
You must be signed in to change notification settings - Fork 65
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
is there way to make both --opt-with-dash
and environment variable work at the same time?
#469
Comments
@kschwab Could you take a look? |
@braindevices, @hramezani, this is an issue specific to $ export HELLO-WORLD=today # fails
bash: export: 'HELLO-WORLD=today': not a valid identifier
$ export HELLO_WORLD=today # passes So, in theory the correct solution is argv: ['Settings2', '--sub_model.v1=cli']
CliSettingsSource {'sub-model': {'v1': 'cli'}}
InitSettingsSource {}
EnvSettingsSource {'v0': 'env no prefix', 'sub-model': {'v1': 'env__no__prefix', 'v2': 'env__no__prefix'}}
DotEnvSettingsSource {}
SecretsSettingsSource {}
DefaultSettingsSource {'sub_model': {'v1': 'top default', 'v2': b'hello', 'v3': 33}}
in test cli: Settings2()= v0='env no prefix' sub_model=SubModel(v1='top default', v2=b'hello', v3=33)
argv: ['Settings2', '--sub-model.v1=cli']
CliSettingsSource {'sub-model': {'v1': 'cli'}}
InitSettingsSource {}
EnvSettingsSource {'v0': 'env no prefix', 'sub-model': {'v1': 'env__no__prefix', 'v2': 'env__no__prefix'}}
DotEnvSettingsSource {}
SecretsSettingsSource {}
DefaultSettingsSource {'sub_model': {'v1': 'top default', 'v2': b'hello', 'v3': 33}}
in test cli: Settings2()= v0='env no prefix' sub_model=SubModel(v1='top default', v2=b'hello', v3=33) The difference between this issue and #406 is the |
Also, @hramezani there is still an issue with alias resolution consistency we'll need to address. Actually, I think this has potential to be general issue for any source. In the above example, the e.g., printing what argv: ['Settings2', '--sub_model.v1=cli']
{'sub_model.v1': 'cli'} # what CLI settings resolved
CliSettingsSource {'sub-model': {'v1': 'cli'}} # what internal env settings resolved
:
EnvSettingsSource {'sub-model': {'v1': 'env__no__prefix', 'v2': 'env__no__prefix'}, ...}
:
in test cli: Settings2()= v0='env no prefix' sub_model=SubModel(v1='top default', v2=b'hello', v3=33)
argv: ['Settings2', '--sub-model.v1=cli']
{'sub_model.v1': 'cli'} # what CLI settings resolved
CliSettingsSource {'sub-model': {'v1': 'cli'}} # what internal env settings resolved
:
EnvSettingsSource {'sub-model': {'v1': 'env__no__prefix', 'v2': 'env__no__prefix'}, ...}
:
in test cli: Settings2()= v0='env no prefix' sub_model=SubModel(v1='top default', v2=b'hello', v3=33) Because of "sub-model" vs "sub_model" difference it will not propagate correctly. In case of |
@kschwab the order does not even related to AliasChoices, if I make the alias_generator=AliasGenerator(
validation_alias=lambda field_name: AliasChoices(
field_name.replace("_", "-"),
field_name
)
) The final result won't change, because now the DefaultSettingsSource switch to 'sub-model': argv: ['setting', '--sub_model.v1=cli']
CliSettingsSource: {'sub_model': {'v1': 'cli'}}
InitSettingsSource: {}
EnvSettingsSource: {'v0': 'env no prefix', 'sub_model': {'v1': 'env__no__prefix', 'v2': 'env__no__prefix'}}
DotEnvSettingsSource: {}
SecretsSettingsSource: {}
DefaultSettingsSource: {'sub-model': {'v1': 'top default', 'v2': b'hello', 'v3': 33}}
v0='env no prefix' sub_model=SubModel(v1='top default', v2=b'hello', v3=33)
in test cli: Settings2()= v0='env no prefix' sub_model=SubModel(v1='top default', v2=b'hello', v3=33)
argv: ['setting', '--sub-model.v1=cli']
CliSettingsSource: {'sub_model': {'v1': 'cli'}}
InitSettingsSource: {}
EnvSettingsSource: {'v0': 'env no prefix', 'sub_model': {'v1': 'env__no__prefix', 'v2': 'env__no__prefix'}}
DotEnvSettingsSource: {}
SecretsSettingsSource: {}
DefaultSettingsSource: {'sub-model': {'v1': 'top default', 'v2': b'hello', 'v3': 33}}
v0='env no prefix' sub_model=SubModel(v1='top default', v2=b'hello', v3=33)
in test cli: Settings2()= v0='env no prefix' sub_model=SubModel(v1='top default', v2=b'hello', v3=33) So the DefaultSettingsSource has the correct priority. But the CliSettingsSource and EnvSettingsSource use the last field_key presented which is kind of reversed priority |
Correct. It's an internal issue related to alias resolution consistency. Externally it will be the same result.
It's related because internally it impacts how aliases are resolved during merging. It's a general problem, but for built-in sources they should all use the same strategy.
Technically it's just In any case, they all just need to use the same strategy. It is a separate issue from the I'll leave the |
Thanks @kschwab and @braindevices for working on this issue.
@kschwab, Could you explain the |
Sure @hramezani. Below is MRE with explanation of issue: class SubModel(BaseModel):
v1: str = 'model default'
class SettingsPass(BaseSettings):
model_config = SettingsConfigDict(
env_nested_delimiter='__',
nested_model_default_partial_update=True,
# Alias generator that yields "sub_model" and "sub-model".
alias_generator=AliasGenerator(
validation_alias=lambda s: AliasChoices(s, s.replace('_', '-'))
)
)
sub_model: SubModel = SubModel(v1='top default')
class SettingsFail(SettingsPass):
# Same as SettingsPass, but uses environment prefix "MYTEST_"
model_config = SettingsConfigDict(env_prefix='MYTEST_')
# Passes when not using a prefixed environment variable.
os.environ['SUB_MODEL__V1'] = 'env without prefix'
assert SettingsPass().model_dump() == {
'sub_model': {'v1': 'env without prefix'}
}
# Fails when using a prefixed environment variable. It is related to using the
# AliasGenerator in combination with environment prefix.
os.environ['MYTEST_SUB_MODEL__V1'] = 'env with prefix'
assert SettingsFail().model_dump() == {
'sub_model': {'v1': 'env with prefix'}
}
Nevermind, it was already supported. So some issue with the above alias generator in combination with environment prefix. |
Also, @braindevices @hramezani I opened #481 to resolve the separate alias resolution issue. |
@hramezani thanks, I forgot about this. @braindevices this is why the |
@kschwab @hramezani thanks for the heads up. Hmmm, this makes the whole solution a bit messier. Does this means I need to do something like Wonder if we need to rethink about how should we handle the environment var names and the A. maybe we should put a flag in CliSettingsSource to convert between |
I think yes. if you want to have both env_prefix and alias together.
@kschwab do you think this is a good idea? how complex is it?
Don't know what was the reason. I think at some point we should make aliases with env_prefix work but I am not sure it will happen soon. |
I don't think this would be too difficult, it's a great suggestion. I think it makes sense to have a flag like this, e.g. I'll work on putting up a PR soon. |
@braindevices thanks again for the suggestion on option A. That actually came out really clean and is a nice addition. Wish we would have had that in there sooner 👍🏾 Opened #489 for resolution. |
@kschwab thanks a lot for the implementation. But here is some question about the PR.
I think the test case should include these cases to make sure that the behaviour is defined. |
It will be
Correct. Subcommand and positional args would not have kebab applied. All other args would, e.g.
My concern here is that |
PR has been updated with feedback. |
@kschwab thanks, it looks perfect now. |
test using #468
output:
So the Settings, use
alias_generator=AliasGenerator(lambda s: s.replace('_', '-'))
which cause the env_prefix has no effect, the sub_model become sub-model so it won't find any sub_model in env.Settings1, doe not have the alias generator, so the env options works normally.
Settings2, attempt to use
alias_generator=AliasGenerator(validation_alias=lambda field_name: AliasChoices(field_name, field_name.replace("_", "-")))
to allow both--sub-model
and env_* options to work. But it raise error withoutextra="ignore"
:with
extra="ignore"
, both'--sub_model.v1=cli'
and'--sub-model.v1=cli'
has no effect.The text was updated successfully, but these errors were encountered: