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.

Sign up for GitHub

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

Jump to bottom

Add official support for Python 3.12 #3587

Merged
merged 10 commits into from
Mar 12, 2024
Merged

Add official support for Python 3.12 #3587

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
cc28696
Add official support for Python 3.12
astrojuanlu Feb 2, 2024
74705b8
Mark broken test as xfail
astrojuanlu Feb 2, 2024
4dc83f2
Overwrite inherited get to use __getitem__
Mar 8, 2024
d01541f
Appease mypy
Mar 8, 2024
43ab544
Fix test
Mar 8, 2024
afa5b1b
Merge branch 'main' into feat/python-3-12
AhdraMeraliQB Mar 8, 2024
62481cc
Fix test coverage
Mar 8, 2024
d544384
Merge branch 'feat/python-3-12' of github.com:kedro-org/kedro into fe…
Mar 8, 2024
f459d2f
Use spy instead of patch
Mar 8, 2024
9afc366
Make comment more clear
Mar 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/all-checks.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ jobs:
strategy:
matrix:
os: [ windows-latest, ubuntu-latest ]
python-version: [ "3.8", "3.9", "3.10", "3.11" ]
python-version: [ "3.8", "3.9", "3.10", "3.11", "3.12" ]
uses: ./.github/workflows/unit-tests.yml
with:
os: ${{ matrix.os }}
Expand All @@ -36,7 +36,7 @@ jobs:
strategy:
matrix:
os: [ windows-latest, ubuntu-latest ]
python-version: [ "3.8", "3.9", "3.10", "3.11" ]
python-version: [ "3.8", "3.9", "3.10", "3.11", "3.12" ]
uses: ./.github/workflows/e2e-tests.yml
with:
os: ${{ matrix.os }}
Expand All @@ -59,7 +59,7 @@ jobs:
strategy:
matrix:
os: [ windows-latest, ubuntu-latest ]
python-version: [ "3.8", "3.9", "3.10", "3.11" ]
python-version: [ "3.8", "3.9", "3.10", "3.11", "3.12" ]
uses: ./.github/workflows/pip-compile.yml
with:
os: ${{ matrix.os }}
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/docs-only-checks.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ jobs:
strategy:
matrix:
os: [ ubuntu-latest ]
python-version: [ "3.8", "3.9", "3.10", "3.11" ]
python-version: [ "3.8", "3.9", "3.10", "3.11", "3.12" ]
uses: ./.github/workflows/lint.yml
with:
os: ${{ matrix.os }}
Expand Down
1 change: 1 addition & 0 deletions RELEASE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

## Major features and improvements
* Create the debugging line magic `%load_node` for Jupyter Notebook and Jupyter Lab.
* Add official support for Python 3.12.
* Add better IPython, VSCode Notebook support for `%load_node` and minimal support for Databricks.
* Add full Kedro Node input syntax for `%load_node`.

Expand Down
2 changes: 1 addition & 1 deletion kedro/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class KedroPythonVersionWarning(UserWarning):
warnings.simplefilter("default", KedroDeprecationWarning)
warnings.simplefilter("error", KedroPythonVersionWarning)

if sys.version_info >= (3, 12):
if sys.version_info >= (3, 13):
warnings.warn(
"""Kedro is not yet fully compatible with this Python version.
To proceed at your own risk and ignore this warning,
Expand Down
9 changes: 9 additions & 0 deletions kedro/config/abstract_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,15 @@ def __init__(
self.env = env
self.runtime_params = runtime_params or {}

# From Python 3.12 __getitem__ isn't called in UserDict.get()
# Use the version from 3.11 and prior
def get(self, key: str, default: Any = None) -> Any:
"D.get(k[,d]) -> D[k] if k in D, else d. d defaults to None."
try:
return self[key]
except KeyError:
return default

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

See this issue, the UserDict's get implementation in 3.12 doesn't work for the config loader anymore, so the method is overwritten to use the get function from the previous python versions

Copy link
Contributor

@AhdraMeraliQB AhdraMeraliQB Mar 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I will make the comment in the file more clear Done!

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be on OmegaConfigLoader or the abstract method? I am slightly confused why get method is needed for OmegaConf.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be on OmegaConfigLoader or the abstract method?

I went for the AbstractConfigLoader to catch any cases where users have defined their own Config Loaders (for whatever reason) that inherit from the AbstractConfigLoader.

I am slightly confused why get method is needed for OmegaConf.

Whilst we don't promote using get over the dict syntax, we have made it available - and it features in our API docs - allowing users to do something like:

conf_loader = OmegaConfigLoader(<...>)
if not conf_loader.get("parameters"):
    parameters = MY_LOCAL_PARAMS

Ultimately, removing get() is a breaking change, which would block adding 3.12 support until the next major release.


class BadConfigException(Exception):
"""Raised when a configuration file cannot be loaded, for instance
Expand Down
39 changes: 23 additions & 16 deletions tests/config/test_omegaconf_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,9 +175,11 @@ def test_load_core_config_get_syntax(self, tmp_path):
)
params = conf.get("parameters")
catalog = conf.get("catalog")
missing_conf = conf.get("missing_conf")

assert params["param1"] == 1
assert catalog["trains"]["type"] == "MemoryDataset"
assert missing_conf == None

@use_config_dir
def test_load_local_config_overrides_base(self, tmp_path):
Expand Down Expand Up @@ -401,6 +403,7 @@ def test_empty_catalog_file(self, tmp_path):
)["catalog"]
assert catalog == {}

@pytest.mark.xfail(reason="Logic failing")
Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed the test to check for the intended behaviour. However, it fails, and manual testing shows that this is due to the implementation of the OmegaConfigLoader. I would suggest correcting this should be split out into a separate issue to be addressed.

Copy link
Member

Choose a reason for hiding this comment

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

I don't agree with this being addressed in a separate issue, adding the 3.12 support is what's making this fail and thus it should be fixed in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not the move to include 3.12 that's causing this test to fail, but rather it was never truly passing, the previous method call used for the assert was mistyped, leading to a trivial pass. To resolve this requires digging further into the implementation of the OmegaConfigLoader, as it is not behaving as expected, and perhaps never was. See xref.

def test_overlapping_patterns(self, tmp_path, mocker):
"""Check that same configuration file is not loaded more than once."""
_write_yaml(
Expand All @@ -421,23 +424,27 @@ def test_overlapping_patterns(self, tmp_path, mocker):
]
}

catalog = OmegaConfigLoader(
conf_source=str(tmp_path),
base_env=_BASE_ENV,
env="dev",
config_patterns=catalog_patterns,
)["catalog"]
expected_catalog = {
"env": "dev",
"common": "common",
"dev_specific": "wiz",
"user1_c2": True,
}
assert catalog == expected_catalog
# Use a mocked function to keep track of function calls
with mocker.patch(
"omegaconf.OmegaConf.load", wraps=OmegaConf.load
) as mocked_load:
AhdraMeraliQB marked this conversation as resolved.
Show resolved Hide resolved
catalog = OmegaConfigLoader(
conf_source=str(tmp_path),
base_env=_BASE_ENV,
env="dev",
config_patterns=catalog_patterns,
)["catalog"]
expected_catalog = {
"env": "dev",
"common": "common",
"dev_specific": "wiz",
"user1_c2": True,
}
assert catalog == expected_catalog

mocked_load = mocker.patch("omegaconf.OmegaConf.load")
expected_path = (tmp_path / "dev" / "user1" / "catalog2.yml").resolve()
assert mocked_load.called_once_with(expected_path)
# Assert any specific config file was only loaded once
expected_path = (tmp_path / "dev" / "user1" / "catalog2.yml").resolve()
mocked_load.assert_called_once_with(expected_path)

def test_yaml_parser_error(self, tmp_path):
conf_path = tmp_path / _BASE_ENV
Expand Down
8 changes: 4 additions & 4 deletions tests/test_import.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@


def test_import_kedro_with_no_official_support_raise_error(mocker):
"""Test importing kedro with python>=3.12 should fail"""
mocker.patch("kedro.sys.version_info", (3, 12))
"""Test importing kedro with python>=3.13 should fail"""
mocker.patch("kedro.sys.version_info", (3, 13))

# We use the parent class to avoid issues with `exec_module`
with pytest.raises(UserWarning) as excinfo:
Expand All @@ -15,8 +15,8 @@ def test_import_kedro_with_no_official_support_raise_error(mocker):


def test_import_kedro_with_no_official_support_emits_warning(mocker):
"""Test importing kedro python>=3.12 and controlled warnings should work"""
mocker.patch("kedro.sys.version_info", (3, 12))
"""Test importing kedro python>=3.13 and controlled warnings should work"""
mocker.patch("kedro.sys.version_info", (3, 13))
mocker.patch("kedro.sys.warnoptions", ["default:Kedro is not yet fully compatible"])

# We use the parent class to avoid issues with `exec_module`
Expand Down
Loading