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 PyprojectTomlConfigSettingsSource #255

Merged
merged 8 commits into from
Mar 28, 2024
Merged

add PyprojectTomlConfigSettingsSource #255

merged 8 commits into from
Mar 28, 2024

Conversation

ITProKyle
Copy link
Contributor

@ITProKyle ITProKyle commented Mar 13, 2024

PR template copied from https://github.com/pydantic/pydantic/blob/18d39febf00cb07954e19ebbdbd8168ef20e3df1/.github/PULL_REQUEST_TEMPLATE.md as this repo does not contain one.

Change Summary

  • added a PyprojectTomlConfigSettingsSource source class
    • finds and uses a pyproject.toml file to fill variables
    • optionally use the values from a specific table
  • added SettingsConfigDict.pyproject_toml_depth (only used by PyprojectTomlConfigSettingsSource)
    • specify a number of levels up the directory tree to look for a pyproject.toml if not found in the current working directory
    • if not provided, 0 is used
    • if no file is found, path in current working directory is used
  • added SettingsConfigDict.pyproject_toml_table_header (only used by PyprojectTomlConfigSettingsSource)
    • used to optionally select a table to fill values from
    • if not provided the root table is used
    • header was used over path (mention in the feature request) after reviewing the terminology used in TOML v1.0.0

Related issue number

resolves #253

Checklist

  • The pull request title is a good summary of the changes - it will be used in the changelog
  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable
  • My PR is ready to review

@ITProKyle ITProKyle marked this pull request as ready for review March 14, 2024 19:55
@hramezani
Copy link
Member

Thanks @ITProKyle for this PR. I still haven't looked at it in detail.

added SettingsConfigDict.pyproject_toml_depth (only used by PyprojectTomlConfigSettingsSource)
specify a number of levels up the directory tree to look for a pyproject.toml if not found in the current working directory
if not provided, 0 is used
if no file is found, path in current working directory is used

Isn't it better to have a pyproject_toml_file config to specify the path of pyproject.toml file. the default value could be the pyproject.toml.
I think the pyproject_toml_depth config is not needed as we can use the pyproject_toml_file

added SettingsConfigDict.toml_table_header (only used by PyprojectTomlConfigSettingsSource)
used to optionally select a table to fill values from
if not provided the root table is used
header was used over path (mention in the feature request) after reviewing the terminology used in TOML v1.0.0

Let's have a default header. something like tools.pydantic-settings

@ITProKyle
Copy link
Contributor Author

I changed toml_table_header to pyproject_toml_table_header for consistency of scoped configs. This would also make it possible to add support for toml_table_header to the TOML source if desired for loading global settings (e.g. from ~/.conf/pydantic-settings/config.toml) that does not need to follow the same nested table structure.

pyproject_toml_table_header defaults to using the [tool.pydantic-settings] table.


Isn't it better to have a pyproject_toml_file config to specify the path of pyproject.toml file. the default value could be the pyproject.toml.
I think the pyproject_toml_depth config is not needed as we can use the pyproject_toml_file

The rationale behind supporting discovery is to mirror the behavior of most tools (I have used) that use pyproject.toml for configuration - accept an explicit path and try to find the file if that path is not provided.
I am fine with adding a pyproject_toml_file config field if you feel that it would be beneficial but removal of discovery slightly hinders the advantages of using a standardized file.

However, I feel that pyproject_toml_file would have limited use in pydantic-settings current state because, as mentioned in #250, it can't be set at runtime which would be the primary case for setting that config field.
I exposed this as in arg when instantiating the source class so that functionality can be implemented in the future or by a user of pydantic-settings if they want to sort out providing an explicit path.

Here are a few references for the discovery behavior:

  • poetry has no depth control, it walks the entire tree
  • ruff (docs instead of source code because rust) has a very complex approach to its discover, walking the tree in both directions (/ <- cwd -> source file being linted)
    • bidirectional walk is well out of scope for generic settings file handling like this is trying to achieve, just point this out as a tool used by this project
  • black does a bit more work to find the project root or use a global (for the user) config file
    • this speaks to the change from toml_table_header to pyproject_toml_table_header but allowing for different table headers to be used

black and ruff's discovery behavior can both be checked within this project by cd pydantic_settings before running them.

@hramezani
Copy link
Member

I changed toml_table_header to pyproject_toml_table_header for consistency of scoped configs. This would also make it possible to add support for toml_table_header to the TOML source if desired for loading global settings (e.g. from ~/.conf/pydantic-settings/config.toml) that does not need to follow the same nested table structure.

pyproject_toml_table_header defaults to using the [tool.pydantic-settings] table.

👍

The rationale behind supporting discovery is to mirror the behavior of most tools (I have used) that use pyproject.toml for configuration - accept an explicit path and try to find the file if that path is not provided.

Can we have the same behavior here as well?(I still haven't checked the code. So, if you implemented like this it would be fine)

@ITProKyle
Copy link
Contributor Author

ITProKyle commented Mar 20, 2024

Can we have the same behavior here as well?(I still haven't checked the code. So, if you implemented like this it would be fine)

ya, that's there. explicitly provided when instantiating the source class (PyprojectTomlConfigSettingsSource(settings_cls, toml_file=Path(...)) which disables any attempt at discovery.
if not provided (or None), discovery is used from the current working directory up int number of directories in the tree (SettingsConfigDict(pyproject_toml_depth=<int>)) until found. if not found within int directories, the current working directory is chosen to populate the object's attribute even though the file does not exist just to have something to fall back to.

By default, discovery is set to a depth of 0 (e.g. no discovery, only use the current working directory) making it opt-in in case someone doesn't want this behavior.

return (PyprojectTomlConfigSettingsSource(settings_cls),)


class RootSettings(Settings):
Copy link
Member

Choose a reason for hiding this comment

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

Why did you define the RootSettings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like I forgot to update the example code after adding a default. This is to show how to load from different locations. Updating this in an incoming commit plus adding a third example to show another use case.

  • Settings for using the default table
  • SomeTableSettings replaces what Settings was doing before, providing a pyproject_toml_table_header config value with a different table header
  • RootSettings updated to work as I had it before setting a default header, loading from the root of pyproject.toml

docs/index.md Outdated
model_config = SettingsConfigDict(extra='ignore')
```

This will be able to read the following "pyproject.toml" file, located in your working directory, resulting in `Settings(field='some-table')` & `RootSettings(field='root')`:
Copy link
Member

Choose a reason for hiding this comment

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

RootSettings also will be RootSettings(field='some-table')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as previous comment, I overlooked updating this. A fix is prepped in an incoming commit.

docs/index.md Outdated Show resolved Hide resolved
@@ -0,0 +1,99 @@
"""Test pydantic_settings.sources."""
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest moving the important tests here to the test_settings file as a complete test case.
for example:

  • Test for a custom pyproject.toml file
  • Test for different depth

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added some tests to test_settings.py for additional coverage. However, I added these test to test_sources.py to make assertions at the source level that arn't possible (from what I can tell) at the BaseSettings subclass level (e.g. asserting that the correct file path is chosen). This can be inferred from the settings level by doing things like only creating a single file with the expected value but it's not quite as precise.

I'll leave this file in place for now with the additional tests added to test_settings.py. If you feel they are still not necessary, feel free to delete the file or LMK and I can do it alongside any additional changes requested.

@hramezani
Copy link
Member

Thanks @ITProKyle for updating the PR. Overall LGTM. I left a couple of comments.

Please update

@hramezani
Copy link
Member

Thanks @ITProKyle

@hramezani hramezani merged commit c2fd92f into pydantic:main Mar 28, 2024
18 checks passed
@ITProKyle ITProKyle deleted the feat/pyproject-toml branch March 28, 2024 11:40
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.

PyprojectTomlConfigSettingsSource or extend TomlConfigSettingsSource
2 participants