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

style(datasets): use PEP 585 #191

Closed
wants to merge 3 commits into from
Closed

style(datasets): use PEP 585 #191

wants to merge 3 commits into from

Conversation

merelcht
Copy link
Member

@merelcht merelcht commented Apr 26, 2023

Description

Follow up on #190

Development notes

https://peps.python.org/pep-0585/

Checklist

  • 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 relevant RELEASE.md file
  • Added tests to cover my changes

Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
@merelcht merelcht self-assigned this Apr 26, 2023
@merelcht
Copy link
Member Author

What am I doing wrong @deepyaman @astrojuanlu

I've changed

Union[plt.figure, List[plt.figure], Dict[str, plt.figure]]

to

Union[plt.figure, list[plt.figure], dict[str, plt.figure]]

(and added the import from __future__ import annotations at the top of the file)

But now the Python 3.7 and 3.8 tests are failing with:

tests/matplotlib/test_matplotlib_writer.py:12: in <module>
    from kedro_datasets.matplotlib import MatplotlibWriter
kedro_datasets/matplotlib/__init__.py:8: in <module>
    from .matplotlib_writer import MatplotlibWriter
kedro_datasets/matplotlib/matplotlib_writer.py:24: in <module>
    Union[plt.figure, list[plt.figure], dict[str, plt.figure]], NoReturn
E   TypeError: 'type' object is not subscriptable

@astrojuanlu
Copy link
Member

Uh, sorry for sending you through this rabbit hole @merelcht. It seems like there's a common misconception that these annotations are usable in Python 3.7 and 3.8...

I think we have to park this until we drop support for Python 3.8.

@merelcht
Copy link
Member Author

Uh, sorry for sending you through this rabbit hole @merelcht. It seems like there's a common misconception that these annotations are usable in Python 3.7 and 3.8...

I managed to get it working okay on the core Kedro repo. Just the read the docs builds complaining there.

@astrojuanlu
Copy link
Member

😳 Well, I'm puzzled now.

@merelcht
Copy link
Member Author

Same, I thought this repo would be much more straightforward..

@leycec
Copy link

leycec commented Apr 26, 2023

...heh. @leycec has been summoned! Exhausting block of monolithic text now follows. Oh – and thanks so much for pinging me here, @astrojuanlu. Kedro looks amazing, everybody.

I have two concrete suggestions with respect to PEP 585 under Python < 3.9. Something is guaranteed to work if you repeatedly bash it hard enough:

  • (Recommended) Add typing_extensions as a mandatory runtime dependency. Then import type hint factories from typing_extensions (e.g., from typing_extensions import Dict, List) rather than attempting to subscript raw builtin types (e.g., list[str]):
# Import the requisite machinery. If lucky, then worky.
from typing_extensions import Dict, List, Union
#from beartype.typing import Dict, List, Union  # <-- alternate timeline approach

# @leycec guarantees success. Would he lie?
Union[plt.figure, List[plt.figure], Dict[str, plt.figure]]
  • Alternately, add @beartype as a mandatory runtime dependency. You, of course, want to do this anyway. Then import type hint factories from beartype.typing (e.g., from beartype.typing import Dict, List) rather than attempting to subscript raw builtin types (e.g., list[str]). See above for fun code that is fun.

typing_extensions and beartype.typing were both intentionally created to obviate forward and backward compatibility concerns across Python versions. Why everything gotta be so hard? 😮‍💨

Type hints: the place where nothing is straightforward and every road leads back to a critically explosive blocker in your codebase.

@astrojuanlu
Copy link
Member

Thanks a lot for chiming in! It's still not clear to me though why things are failing here while working normally over kedro-org/kedro#2540, and also how your advice helps us leverage PEP 585 proper, namely using native collections. We'll have to do a bit more digging I suppose.

@leycec
Copy link

leycec commented Apr 27, 2023

from __future__ import annotations is almost certainly the issue, whatever the issue is. from __future__ import annotations is almost always the issue, because enabling that enables PEP 563. PEP 563 is well-known to be harmful to things that introspect type hints at runtime. This includes things like Sphinx's builtin autodoc extension, which would explain this:

Just the read the docs builds complaining there.

Likewise...

...helps us leverage PEP 585 proper, namely using native collections...

That is a noble goal. Sadly, you can't really achieve that noble goal under Python < 3.9 without breaking runtime things. Thankfully, while noble, that isn't really a useful goal. Right? PEP 585 is basically just syntactic sugar; it doesn't actually gain you anything tangible. typing_extensions and beartype.typing are both dramatically preferable. They work everywhere.

In fact, mypy itself now strongly encourages use of typing_extensions over from __future__ import annotations:

Starting with Python 3.9 (PEP 585), the type objects of many collections in the standard library support subscription at runtime. This means that you no longer have to import the equivalents from typing; you can simply use the built-in collections or those from collections.abc:

from collections.abc import Sequence
x: list[str]
y: dict[int, str]
z: Sequence[str] = x

There is limited support for using this syntax in Python 3.7 and later as well: if you use from __future__ import annotations, mypy will understand this syntax in annotations. However, since this will not be supported by the Python interpreter at runtime, make sure you’re aware of the caveats mentioned in the notes at future annotations import.

Note the critical statement: "this will not be supported by the Python interpreter at runtime". In other words, what you are currently trying to do is likely to raise exceptions under Python < 3.9 from things that introspect type hints at runtime (e.g., sphinx.ext.autodoc).

@merelcht
Copy link
Member Author

I'm going to close this for now as it doesn't seem easy to enforce PEP 585 everywhere and it doesn't add a lot other than syntactic sugar.

@merelcht merelcht closed this May 18, 2023
@merelcht merelcht deleted the use-pep-585 branch May 19, 2023 15:48
@astrojuanlu
Copy link
Member

This can be brought back to life after #442 is merged cc @DimedS

@astrojuanlu
Copy link
Member

Well, in fact I think #442 already does this 😄

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.

3 participants