Make InitSettingsSource resolution deterministic

[enrico-stauss]

This PR fixes the issue described in #679, by making the alias resolution in InitSettingsSource deterministically follow the configured precedence.

[hramezani]

Thanks @enrico-stauss for the PR.

@kschwab Could you review the PR?

[kschwab]

This is a great catch, thanks @enrico-stauss. Looks good to me. Could simplify to a single line if desired:

init_kwarg_name = sorted(init_kwarg_names & set(alias_names))

[enrico-stauss]

This is a great catch, thanks @enrico-stauss. Looks good to me. Could simplify to a single line if desired:

init_kwarg_name = sorted(init_kwarg_names & set(alias_names))

Not sure that I understand correctly @kschwab , you want to sort the set and then always take the last element by init_kwarg_name.pop()? This would make it deterministic but ignore the user-set prioritization, right?

Thanks for the approval, I'm glad I could contribute :)

May I ask for the reason why nesting BaseSettings is disencouraged? Could it be that this PR voids that?

[kschwab]

Not sure that I understand correctly @kschwab , you want to sort the set and then always take the last element by init_kwarg_name.pop()? This would make it deterministic but ignore the user-set prioritization, right?

You're right. I think currently precedence is implicit, but I agree. Stick with what you have, it's more correct.

May I ask for the reason why nesting BaseSettings is disencouraged? Could it be that this PR voids that?

Your fix is independent of nested BaseSettings. It's a good fix. Nesting BaseSettings can have unintended consequences since all the source modules will be re-ran at different levels. i.e., YMMV. @hramezani can likely provide more insight here.

Otherwise, everything else looks good. Thanks again for the fix!

[hramezani]

Thanks @kschwab for the review.

Nesting BaseSettings can have unintended consequences since all the source modules will be re-ran at different levels.

This is exactly the reason and we mentioned it in the docs as well.

[hramezani]

Thanks both ❤️

[enrico-stauss]

Thanks @kschwab for the review.

Nesting BaseSettings can have unintended consequences since all the source modules will be re-ran at different levels.

This is exactly the reason and we mentioned it in the docs as well.

My thoughts were going in the direction of "it maybe not having unintended consequences anymore" 😄

I had obviously read the dochmentation, but wanted to understand a bit better of what nature these unintended consequences are. Is there an assessment or something available?

[hramezani]

if the nested model inherits from BaseSettings it will be initialized during the parent model init.

Example from #426

from pydantic_settings import BaseSettings, SettingsConfigDict

class InnerSettings(BaseSettings):
    port: int = 5432

class Settings(BaseSettings):
    model_config = SettingsConfigDict(env_prefix="SETTINGS__", env_nested_delimiter="__")
    
    inner_settings: InnerSettings = InnerSettings()
    port: int = 5433

settings = Settings()
print(settings.model_dump())

if we run the above code with PORT=666 python test.py, you will get {'inner_settings': {'port': 666}, 'port': 5433} which is wrong.
but if the InnerSettings inherits from BaseModel, you will get {'inner_settings': {'port': 5432}, 'port': 5433} that is correct.

[enrico-stauss]

Thanks for the concrete example! I personally would probably consider this a feature, not a bug, since the resolution order makes sense to me. In the example I would encourage users to do:

from pydantic_settings import BaseSettings, SettingsConfigDict

class InnerSettings(BaseSettings):
    port: int = 5432

class Settings(BaseSettings):
    model_config = SettingsConfigDict(env_prefix="SETTINGS__", env_nested_delimiter="__")
    
    inner_settings: InnerSettings = InnerSettings(port=5432)  # Explicitly pass the default to the initializer
    port: int = 5433

if the default to the inner BaseSettings object should take precedence over environment variables.

But I agree that this could be a bit unintuitive, given that there is no way to completely deactivate parsing of non-prefixed environment variables for inner BaseSettings objects. Maybe for Pydantic V3 you could consider having a mechanism that (by default) surpresses parsing of non-prefixed environment variables for inner BaseSettings and allows users to opt-in via the SettingsConfigDict of the outer class?

This was my last thoughts on the topic, I promise I'll leave you in peace now :D
Thanks and kind regards
Enrico