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

Fix PathType typing in case of sequence #402

Merged
merged 5 commits into from
Sep 16, 2024

Conversation

just-maiyak
Copy link
Contributor

Fixes #401

@just-maiyak just-maiyak changed the title Fix PathType typing in case of sequence Draft: Fix PathType typing in case of sequence Sep 13, 2024
@hramezani
Copy link
Member

I just fixed the syntax error to let the CI run

@just-maiyak
Copy link
Contributor Author

Perfect thank you so much, I amended without staging the changes and after that I didn't have my laptop on hand 😅

@just-maiyak just-maiyak force-pushed the fix/pathtype-sequence branch 4 times, most recently from bbf5846 to 252729d Compare September 13, 2024 22:12
@just-maiyak just-maiyak changed the title Draft: Fix PathType typing in case of sequence Fix PathType typing in case of sequence Sep 13, 2024
@just-maiyak
Copy link
Contributor Author

I fixed the CI, there were still a couple of unused imports. It should be good to merge :)

@hramezani
Copy link
Member

Thanks @just-maiyak for updating this PR.

Could you provide a simple example that has mypy error? I am going to test the error and the fix?

Thanks!

@just-maiyak
Copy link
Contributor Author

just-maiyak commented Sep 16, 2024

Sure, if you run mypy on this file :

from pydantic_settings import BaseSettings, YamlConfigSettingsSource


class Settings(BaseSettings):
    config_files = ["config.yaml"]
    model_config = YamlConfigSettingsSource(BaseSettings, yaml_file=config_files)

It will fail with the error I reported in #401.

@just-maiyak just-maiyak reopened this Sep 16, 2024
@hramezani
Copy link
Member

Thanks @just-maiyak for providing the example.

The model that you provided looks weird to me. Could you explain why you are using YamlConfigSettingsSource in the model_config?

@just-maiyak
Copy link
Contributor Author

I'm trying to read settings from a list of yaml files (just like we would from a .env file). I'm following the docs right here : Other Settings Source.

@hramezani
Copy link
Member

But this should be like the first example in the page:

from typing import Tuple, Type

from pydantic import BaseModel

from pydantic_settings import (
    BaseSettings,
    PydanticBaseSettingsSource,
    SettingsConfigDict,
    TomlConfigSettingsSource,
)


class Nested(BaseModel):
    nested_field: str


class Settings(BaseSettings):
    foobar: str
    nested: Nested
    model_config = SettingsConfigDict(toml_file='config.toml')

    @classmethod
    def settings_customise_sources(
        cls,
        settings_cls: Type[BaseSettings],
        init_settings: PydanticBaseSettingsSource,
        env_settings: PydanticBaseSettingsSource,
        dotenv_settings: PydanticBaseSettingsSource,
        file_secret_settings: PydanticBaseSettingsSource,
    ) -> Tuple[PydanticBaseSettingsSource, ...]:
        return (TomlConfigSettingsSource(settings_cls),)

You only need to change the TomlConfigSettingsSource with YamlConfigSettingsSource

@just-maiyak
Copy link
Contributor Author

just-maiyak commented Sep 16, 2024

Alright, it's not very clear from the docs that you have to keep settings_customize_sources. It's also not so clear how SettingsConfigDict works. Is the latter built from calling settings_customize_sources ?
Edit: I'll close this PR because I using settings_customize_sources solved my problem, but I do think the docs need to be more explicit. Should I open another issue ?

@hramezani
Copy link
Member

Alright, it's not very clear from the docs that you have to keep settings_customize_sources

This is because yaml settings source is not part of the default settings sources and you need to somehow override the default behavior to enable this source. we decided to use this approach to prevent breaking change in V2.
Here is the doc.

BTW, any improvement on docs is welcome. feel free to create a PR if you want. You don't need to create a new issue.

Thanks!

@just-maiyak
Copy link
Contributor Author

I'll work on a docs contribution :) Thanks for helping out !

@just-maiyak
Copy link
Contributor Author

just-maiyak commented Sep 16, 2024

Sorry for being so persistent ... I'm reopening this again because I still thing the issue is happening in the case I start from the sample you provide and parametrize the yaml_file like so :

from typing import Tuple, Type

from pydantic import BaseModel

from pydantic_settings import (
    BaseSettings,
    PydanticBaseSettingsSource,
    SettingsConfigDict,
    TomlConfigSettingsSource,
)


class Nested(BaseModel):
    nested_field: str


class Settings(BaseSettings):
    foobar: str
    nested: Nested

    config_files: list[str] = ["config.yaml"] # This might be the return of a funtion or initialized in __init__
    model_config = SettingsConfigDict(toml_file=config_files)

    @classmethod
    def settings_customise_sources(
        cls,
        settings_cls: Type[BaseSettings],
        init_settings: PydanticBaseSettingsSource,
        env_settings: PydanticBaseSettingsSource,
        dotenv_settings: PydanticBaseSettingsSource,
        file_secret_settings: PydanticBaseSettingsSource,
    ) -> Tuple[PydanticBaseSettingsSource, ...]:
        return (TomlConfigSettingsSource(settings_cls),)

This produces yet again the same error as before. Should this be done in another way ?

@just-maiyak just-maiyak reopened this Sep 16, 2024
@hramezani
Copy link
Member

I think the config_files: list[str] = ["config.yaml"] is not the correct way. You probably did it like this because the config file name is not specified and you want to pass it when you initialize the model.

There is an issue for this problem #259
I am going to prepare a PR for this issue but before that probably you can use one of the workaround suggested in the issue.

@just-maiyak
Copy link
Contributor Author

just-maiyak commented Sep 16, 2024

That's my case indeed, want to provide additional files at runtime. Unfortunately, I still have the same typing problem even with the workaround as soon as I wrap the settings filenames in a list.

I still think the Sequence type is more appropriate if write operations to that variable are not possible (which I think is the case because it seems we only construct settings once as soon as we read that yaml_config parameter).

What also works is to explicitly type whatever we're passing as list[Path | str] but that introduces unnecessary noise imo.

@hramezani hramezani merged commit b680531 into pydantic:main Sep 16, 2024
54 checks passed
@just-maiyak just-maiyak deleted the fix/pathtype-sequence branch September 16, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mypy reports error when passing a list to YamlConfigSettingsSource through yaml_file
2 participants