Add official support for Python 3.12 #3587

astrojuanlu · 2024-02-02T08:05:55Z

Description

Development notes

First attempt, I haven't ran the test suite locally yet. But pip install -e .[test] worked first time on Apple Silicon.

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

Read the contributing guidelines
Signed off each commit with a Developer Certificate of Origin (DCO)
Opened this PR as a 'Draft Pull Request' if it is work-in-progress
Updated the documentation to reflect the code changes
Added a description of this change in the RELEASE.md file
Added tests to cover my changes
Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

astrojuanlu · 2024-02-02T08:24:29Z

Tests failing on 3.11 instead 🤔

Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.11.7/x64/lib/python3.11/multiprocessing/queues.py", line 244, in _feed
    obj = _ForkingPickler.dumps(obj)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.11.7/x64/lib/python3.11/multiprocessing/reduction.py", line 51, in dumps
    cls(buf, protocol).dump(obj)
AttributeError: Can't pickle local object 'PluginManager.add_hookcall_monitoring.<locals>.traced_hookexec'

rings a bell but I don't have time to look at this right now, will do so later.

merelcht · 2024-02-02T11:39:54Z

Tests failing on 3.11 instead 🤔

Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.11.7/x64/lib/python3.11/multiprocessing/queues.py", line 244, in _feed
    obj = _ForkingPickler.dumps(obj)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.11.7/x64/lib/python3.11/multiprocessing/reduction.py", line 51, in dumps
    cls(buf, protocol).dump(obj)
AttributeError: Can't pickle local object 'PluginManager.add_hookcall_monitoring.<locals>.traced_hookexec'

rings a bell but I don't have time to look at this right now, will do so later.

This is the flaky test that occasionally fails. I think @lrcouto was seeing this as well on her PR for adding the _EPHEMERAL attribute.

astrojuanlu · 2024-02-02T16:39:54Z

tests/config/test_omegaconf_config.py

@@ -437,7 +438,8 @@ def test_overlapping_patterns(self, tmp_path, mocker):

        mocked_load = mocker.patch("omegaconf.OmegaConf.load")
        expected_path = (tmp_path / "dev" / "user1" / "catalog2.yml").resolve()
-        assert mocked_load.called_once_with(expected_path)
+
+        mocked_load.assert_called_once_with(expected_path)


See python/cpython#100690

This assert was trivially passing, the test was not doing anything (and it's broken at least in 3.11 and 3.12)

astrojuanlu · 2024-02-02T17:37:11Z

One test failure left, only happening on Python 3.12:

    @use_config_dir
    def test_load_core_config_get_syntax(self, tmp_path):
        """Make sure core config can be fetched with .get()"""
        conf = OmegaConfigLoader(
            str(tmp_path), base_env=_BASE_ENV, default_run_env=_DEFAULT_RUN_ENV
        )
        params = conf.get("parameters")
        catalog = conf.get("catalog")
    
>       assert params["param1"] == 1
E       TypeError: 'NoneType' object is not subscriptable

lrcouto · 2024-02-02T17:49:25Z

Tests failing on 3.11 instead 🤔
Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.11.7/x64/lib/python3.11/multiprocessing/queues.py", line 244, in _feed
    obj = _ForkingPickler.dumps(obj)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.11.7/x64/lib/python3.11/multiprocessing/reduction.py", line 51, in dumps
    cls(buf, protocol).dump(obj)
AttributeError: Can't pickle local object 'PluginManager.add_hookcall_monitoring.<locals>.traced_hookexec'
rings a bell but I don't have time to look at this right now, will do so later.
This is the flaky test that occasionally fails. I think @lrcouto was seeing this as well on her PR for adding the _EPHEMERAL attribute.

This test is different than the one I've seen. There was one that was caused by a requirement version, which Nok fixed, and another that happened in the tests for the starters. This one is new to me.

ankatiyar · 2024-02-07T17:26:35Z

One test failure left, only happening on Python 3.12:

    @use_config_dir
    def test_load_core_config_get_syntax(self, tmp_path):
        """Make sure core config can be fetched with .get()"""
        conf = OmegaConfigLoader(
            str(tmp_path), base_env=_BASE_ENV, default_run_env=_DEFAULT_RUN_ENV
        )
        params = conf.get("parameters")
        catalog = conf.get("catalog")
    
>       assert params["param1"] == 1
E       TypeError: 'NoneType' object is not subscriptable

Did a little digging - python/cpython#105524

Close #3287. Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>

Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>

astrojuanlu · 2024-02-15T18:48:38Z

Looks like this will require a bit of work. I'm not pursuing this PR for the time being.

merelcht · 2024-02-20T11:46:29Z

Looks like this will require a bit of work. I'm not pursuing this PR for the time being.

@astrojuanlu Maybe you can add some context on the challenges you're seeing in #3287 and then we can properly plan it for a sprint?

astrojuanlu · 2024-02-21T08:52:25Z

Good point, done #3287 (comment)

Signed-off-by: Ahdra Merali <[email protected]>

AhdraMeraliQB · 2024-03-08T14:21:50Z

tests/config/test_omegaconf_config.py

@@ -401,6 +401,7 @@ def test_empty_catalog_file(self, tmp_path):
        )["catalog"]
        assert catalog == {}

+    @pytest.mark.xfail(reason="Logic failing")


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.

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.

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.

Signed-off-by: Ahdra Merali <[email protected]>

…at/python-3-12

noklam · 2024-03-08T14:29:36Z

kedro/config/abstract_config.py

+    # 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
+


Why is this needed?

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

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

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

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.

tests/config/test_omegaconf_config.py

Signed-off-by: Ahdra Merali <[email protected]>

tests/config/test_omegaconf_config.py

merelcht

Thanks @astrojuanlu and @AhdraMeraliQB for making this work 👍

ankatiyar

Thanks @astrojuanlu & @AhdraMeraliQB :D

astrojuanlu requested a review from merelcht as a code owner February 2, 2024 08:05

This comment was marked as outdated.

Sign in to view

astrojuanlu force-pushed the feat/python-3-12 branch from 79e190c to 3d804c9 Compare February 2, 2024 16:39

astrojuanlu commented Feb 2, 2024

View reviewed changes

astrojuanlu added 2 commits February 12, 2024 08:41

Add official support for Python 3.12

cc28696

Close #3287. Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>

Mark broken test as xfail

74705b8

Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>

astrojuanlu force-pushed the feat/python-3-12 branch from 3d804c9 to 74705b8 Compare February 12, 2024 07:41

astrojuanlu mentioned this pull request Feb 21, 2024

Add official support for Python 3.12 #3287

Closed

Ahdra Merali and others added 4 commits March 8, 2024 13:43

Overwrite inherited get to use __getitem__

4dc83f2

Signed-off-by: Ahdra Merali <[email protected]>

Appease mypy

d01541f

Signed-off-by: Ahdra Merali <[email protected]>

Fix test

43ab544

Signed-off-by: Ahdra Merali <[email protected]>

Merge branch 'main' into feat/python-3-12

afa5b1b

Signed-off-by: Ahdra Merali <[email protected]>

AhdraMeraliQB marked this pull request as draft March 8, 2024 14:17

AhdraMeraliQB reviewed Mar 8, 2024

View reviewed changes

AhdraMeraliQB requested review from noklam and ankatiyar March 8, 2024 14:22

AhdraMeraliQB marked this pull request as ready for review March 8, 2024 14:23

Ahdra Merali added 2 commits March 8, 2024 14:26

Fix test coverage

62481cc

Signed-off-by: Ahdra Merali <[email protected]>

Merge branch 'feat/python-3-12' of github.com:kedro-org/kedro into fe…

d544384

…at/python-3-12

noklam reviewed Mar 8, 2024

View reviewed changes

tests/config/test_omegaconf_config.py Outdated Show resolved Hide resolved

Use spy instead of patch

f459d2f

Signed-off-by: Ahdra Merali <[email protected]>

Make comment more clear

9afc366

Signed-off-by: Ahdra Merali <[email protected]>

AhdraMeraliQB requested a review from noklam March 8, 2024 14:48

astrojuanlu commented Mar 8, 2024

View reviewed changes

tests/config/test_omegaconf_config.py Show resolved Hide resolved

merelcht approved these changes Mar 11, 2024

View reviewed changes

ankatiyar approved these changes Mar 12, 2024

View reviewed changes

AhdraMeraliQB merged commit 338736e into main Mar 12, 2024
41 checks passed

AhdraMeraliQB deleted the feat/python-3-12 branch March 12, 2024 10:08

AhdraMeraliQB mentioned this pull request Mar 12, 2024

Update broken OmegaConfigLoader test #3696

Merged

7 tasks

ankatiyar mentioned this pull request Mar 18, 2024

Fix credentials throwing error on kedro catalog resolve #3712

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add official support for Python 3.12 #3587

Add official support for Python 3.12 #3587

astrojuanlu commented Feb 2, 2024

astrojuanlu commented Feb 2, 2024

merelcht commented Feb 2, 2024

This comment was marked as outdated.

astrojuanlu Feb 2, 2024

astrojuanlu commented Feb 2, 2024

lrcouto commented Feb 2, 2024

ankatiyar commented Feb 7, 2024

astrojuanlu commented Feb 15, 2024

merelcht commented Feb 20, 2024

astrojuanlu commented Feb 21, 2024

AhdraMeraliQB Mar 8, 2024

merelcht Mar 8, 2024

AhdraMeraliQB Mar 8, 2024

noklam Mar 8, 2024

AhdraMeraliQB Mar 8, 2024

AhdraMeraliQB Mar 8, 2024 •

edited

Loading

noklam Mar 8, 2024

AhdraMeraliQB Mar 8, 2024

merelcht left a comment

ankatiyar left a comment

Add official support for Python 3.12 #3587

Add official support for Python 3.12 #3587

Conversation

astrojuanlu commented Feb 2, 2024

Description

Development notes

Developer Certificate of Origin

Checklist

astrojuanlu commented Feb 2, 2024

merelcht commented Feb 2, 2024

This comment was marked as outdated.

Choose a reason for hiding this comment

astrojuanlu commented Feb 2, 2024

lrcouto commented Feb 2, 2024

ankatiyar commented Feb 7, 2024

astrojuanlu commented Feb 15, 2024

merelcht commented Feb 20, 2024

astrojuanlu commented Feb 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AhdraMeraliQB Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merelcht left a comment

Choose a reason for hiding this comment

ankatiyar left a comment

Choose a reason for hiding this comment

AhdraMeraliQB Mar 8, 2024 •

edited

Loading