-
Notifications
You must be signed in to change notification settings - Fork 961
Use tomllib, if available, else fall back to tomli #5043
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Deepyaman Datta <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR modernizes TOML handling by migrating from the toml
library to use the standard library tomllib
(Python 3.11+) with a fallback to the more standards-compliant tomli
library for older Python versions.
- Updated dependency from
toml
totomli
with Python version constraint - Implemented compatibility layer using conditional imports based on Python version
- Updated TOML file reading across multiple modules to use the new import structure
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
File | Description |
---|---|
pyproject.toml | Updated dependency from toml to tomli with version constraint and fixed mkdocstrings version |
kedro/templates/project/hooks/utils.py | Added conditional import for tomllib/tomli and updated TOML operations |
kedro/framework/startup.py | Added conditional import and updated exception handling for TOML parsing |
kedro/framework/startup.py
Outdated
@@ -7,7 +7,10 @@ | |||
from pathlib import Path | |||
from typing import NamedTuple | |||
|
|||
import toml | |||
if sys.version_info >= (3, 11): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing import for sys
module. The code uses sys.version_info
but sys
is not imported.
Copilot uses AI. Check for mistakes.
kedro/framework/startup.py
Outdated
@@ -66,8 +69,8 @@ def _get_project_metadata(project_path: Path) -> ProjectMetadata: | |||
) | |||
|
|||
try: | |||
metadata_dict = toml.load(pyproject_toml) | |||
except Exception as exc: | |||
metadata_dict = tomllib.load(pyproject_toml) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tomllib.load()
function expects a binary file opened in 'rb' mode, but pyproject_toml
appears to be opened in text mode based on typical usage patterns.
metadata_dict = tomllib.load(pyproject_toml) | |
with pyproject_toml.open("rb") as f: | |
metadata_dict = tomllib.load(f) |
Copilot uses AI. Check for mistakes.
|
||
# Remove the specified sections | ||
for section in sections_to_remove: | ||
_remove_nested_section(data, section) | ||
|
||
with open(file_path, "w") as file: | ||
toml.dump(data, file) | ||
tomllib.dump(data, file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tomllib
module (standard library) does not have a dump()
function - it's read-only. This will cause an AttributeError. You need to use a different library like tomli_w
for writing TOML files.
Copilot uses AI. Check for mistakes.
Signed-off-by: Deepyaman Datta <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Deepyaman Datta <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Deepyaman Datta <[email protected]>
Signed-off-by: Deepyaman Datta <[email protected]>
Signed-off-by: Deepyaman Datta <[email protected]>
Signed-off-by: Deepyaman Datta <[email protected]>
Signed-off-by: Deepyaman Datta <[email protected]>
Signed-off-by: Deepyaman Datta <[email protected]>
Signed-off-by: Deepyaman Datta <[email protected]>
Signed-off-by: Deepyaman Datta <[email protected]>
Signed-off-by: Deepyaman Datta <[email protected]>
Signed-off-by: Deepyaman Datta <[email protected]>
Signed-off-by: Deepyaman Datta <[email protected]>
Signed-off-by: Deepyaman Datta <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes make sense, just had a question on why two dependencies are needed.
"tomli>=1.1.0;python_version<'3.11'", # tomllib is part of the standard library in Python 3.11+ | ||
"tomli-w", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need the separate tomli-w
dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tomli
(and the tomllib
included in the Python standard library from 3.11) are read-only, and tomli-w
is the recommended writing library (same author).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it! Shame they didn't put it in the same library 🥲
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @deepyaman !
"install", | ||
"-e", | ||
".", | ||
"toml>=0.10.0", # TODO(deepyaman): Migrate `kedro-telemetry` off `toml` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still needed after kedro-org/kedro-plugins#1155 is merged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't be necessary once kedro-telemetry
is released?
I do think kedro-telemetry
should be released first regardless. I'm not sure if there's a specific consideration around using a newer version of Kedro (that uses tomli
or tomllib
instead of toml
) with an older version of kedro-telemetry
(that depended on the implicit toml
dependency coming from Kedro), but I think this could just be documented/it would be pretty obvious to install toml
(or upgrade kedro-telemetry
).
Description
tomli
is more standards-compliant thantoml
, and it's including in the standard library astomllib
since Python 3.11.toml
has not had a release since October 31, 2020.Development notes
Built the compatibility layer, as documented.
Unfortunately,
kedro-telemetry
relies onkedro
to provide thetoml
dependency, so that needs to be updated as well.Developer Certificate of Origin
We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a
Signed-off-by
line in the commit message. See our wiki for guidance.If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.
Checklist
RELEASE.md
file