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

Allow any version of Python (including 3.11) in metadata #2742

Merged
merged 7 commits into from
Jul 11, 2023

Conversation

astrojuanlu
Copy link
Member

@astrojuanlu astrojuanlu commented Jun 28, 2023

See https://iscinumpy.dev/post/bound-version-constraints/#pinning-the-python-version-is-special, and discussion in https://discuss.python.org/t/requires-python-upper-limits/12663?u=astrojuanlu, as well as practical problems with leaving the version cap on in #2270 (comment).

Deliberately not adding anything to the changelog, and not touching our CI systems either.

Checklist

  • Read the contributing guidelines
  • 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

@astrojuanlu astrojuanlu requested a review from idanov as a code owner June 28, 2023 15:27
@astrojuanlu astrojuanlu mentioned this pull request Jun 28, 2023
8 tasks
@astrojuanlu
Copy link
Member Author

We've decided to wait a bit to hear from @merelcht and @idanov

@astrojuanlu astrojuanlu force-pushed the allow-python-3-11 branch 2 times, most recently from f076020 to 090f2e0 Compare July 4, 2023 10:14
@astrojuanlu
Copy link
Member Author

astrojuanlu commented Jul 4, 2023

Prompted by @idanov, I added a warning that by default is treated like an error:

❯ kedro info
Traceback (most recent call last):
  File "/Users/juan_cano/.micromamba/envs/kedro311-dev/bin/kedro", line 5, in <module>
    from kedro.framework.cli import main
  File "/Users/juan_cano/Projects/QuantumBlack Labs/kedro/kedro/__init__.py", line 20, in <module>
    warnings.warn(
kedro.KedroPythonVersionWarning: Kedro is not yet fully compatible with this Python version.
To proceed at your own risk and ignore this warning,
run Kedro with `python -W "default:Kedro is not yet fully compatible" -m kedro ...`
or set the PYTHONWARNINGS environment variable accordingly.

And can be treated as a normal warning or ignored in two different ways (see https://docs.python.org/3/library/warnings.html#describing-warning-filters):

❯ python -W "default:Kedro is not yet fully compatible" -m kedro info
/Users/juan_cano/Projects/QuantumBlack Labs/kedro/kedro/__init__.py:20: KedroPythonVersionWarning: Kedro is not yet fully compatible with this Python version.
To proceed at your own risk and ignore this warning,
run Kedro with `python -W "default:Kedro is not yet fully compatible" -m kedro ...`
or set the PYTHONWARNINGS environment variable accordingly.
  warnings.warn(

 _            _
| | _____  __| |_ __ ___
| |/ / _ \/ _` | '__/ _ \
|   <  __/ (_| | | | (_) |
|_|\_\___|\__,_|_|  \___/
v0.18.10

Kedro is a Python framework for
creating reproducible, maintainable
and modular data science code.

No plugins installed
❯ PYTHONWARNINGS="ignore:Kedro is not yet fully compatible" kedro info

 _            _
| | _____  __| |_ __ ___
| |/ / _ \/ _` | '__/ _ \
|   <  __/ (_| | | | (_) |
|_|\_\___|\__,_|_|  \___/
v0.18.10

Kedro is a Python framework for
creating reproducible, maintainable
and modular data science code.

No plugins installed

(custom categories can't be specified, see python/cpython#66733)

@astrojuanlu astrojuanlu force-pushed the allow-python-3-11 branch 3 times, most recently from 75b5770 to 3f33046 Compare July 4, 2023 10:57
Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@noklam
Copy link
Contributor

noklam commented Jul 5, 2023

Do we need to merge this in #2417? Not sure how we can test this without adding python 3.11 first, as the test will never be triggered in the CI and basically do nothing for 3.7 - 3.10.

@astrojuanlu
Copy link
Member Author

Merging this into gh-2417 doesn't really solve the bootstrapping problem (that there has to be a 3.11-enabled version to unblock the CI).

On the other hand, the plan is to test these changes - it's just that we'll have to mock the sys.version_info check, rather than using a real Python 3.11 environment.

@noklam
Copy link
Contributor

noklam commented Jul 5, 2023

good point👍

@noklam
Copy link
Contributor

noklam commented Jul 6, 2023

@astrojuanlu It turns out to be tricky with sys modules, I haven't found a way to handle version switch nicely.

A few challenges here

  1. To use importlib.reload, it takes a module, so you need to import the module beforehand (which we don't if we expect the test fail). So I use a setup_environment fixture to bypass it and reset the PYTHONWARNING, which I think is fine.
  2. mocking sys directly will cause error because this interfere with pytest, the correct way to do it is always mock the local module which is kedro.sys. This is fine, but when we do importlib.reload(kedro) we effectively remove kedro.sys so the mock that we insert is removed.

Any idea how it could work?

@astrojuanlu
Copy link
Member Author

Ugh, I see what you mean. I'll give it some thought.

@astrojuanlu
Copy link
Member Author

The key is to not use reload:

In [1]: import importlib

In [2]: import kedro

In [3]: kedro.__loader__.exec_module(kedro)

In [4]: from unittest.mock import patch

In [5]: with patch("kedro.sys.version_info", (3, 11)):
   ...:     kedro.__loader__.exec_module(kedro)
   ...: 
---------------------------------------------------------------------------
KedroPythonVersionWarning                 Traceback (most recent call last)
Cell In[5], line 2
      1 with patch("kedro.sys.version_info", (3, 11)):
----> 2     kedro.__loader__.exec_module(kedro)

File <frozen importlib._bootstrap_external>:883, in exec_module(self, module)

File <frozen importlib._bootstrap>:241, in _call_with_frames_removed(f, *args, **kwds)

File ~/Projects/QuantumBlack Labs/kedro/kedro/__init__.py:22
     19     warnings.simplefilter("error", KedroPythonVersionWarning)
     21 if sys.version_info >= (3, 11):
---> 22     warnings.warn(
     23         """Kedro is not yet fully compatible with this Python version.
     24 To proceed at your own risk and ignore this warning,
     25 run Kedro with `python -W "default:Kedro is not yet fully compatible" -m kedro ...`
     26 or set the PYTHONWARNINGS environment variable accordingly.""",
     27         KedroPythonVersionWarning,
     28     )

KedroPythonVersionWarning: Kedro is not yet fully compatible with this Python version.
To proceed at your own risk and ignore this warning,
run Kedro with `python -W "default:Kedro is not yet fully compatible" -m kedro ...`
or set the PYTHONWARNINGS environment variable accordingly.

@noklam noklam mentioned this pull request Jul 7, 2023
tests/test_import.py Outdated Show resolved Hide resolved
@astrojuanlu astrojuanlu force-pushed the allow-python-3-11 branch 2 times, most recently from a0bc4b3 to c0c5270 Compare July 10, 2023 15:10
astrojuanlu and others added 7 commits July 10, 2023 21:10
See https://iscinumpy.dev/post/bound-version-constraints/#pinning-the-python-version-is-special,
and discussion in https://discuss.python.org/t/requires-python-upper-limits/12663?u=astrojuanlu,
as well as practical problems with leaving the version cap on in
#2270 (comment).

Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
Copy link
Contributor

@SajidAlamQB SajidAlamQB left a comment

Choose a reason for hiding this comment

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

This PR looks great to me! 🚀 I think changing it to a warning when using unsupported Python versions is a good approach.

Thank you @astrojuanlu!

@astrojuanlu
Copy link
Member Author

Thanks a bunch @SajidAlamQB ! This is going in

@astrojuanlu astrojuanlu merged commit 7e8d9f4 into main Jul 11, 2023
3 checks passed
@astrojuanlu astrojuanlu deleted the allow-python-3-11 branch July 11, 2023 12:23
@noklam
Copy link
Contributor

noklam commented Jul 11, 2023

🚀🚀🚀🚀

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.

4 participants