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

Make typing-extensions a test only dependency #248

Closed
dnicolodi opened this issue Dec 21, 2022 · 8 comments
Closed

Make typing-extensions a test only dependency #248

dnicolodi opened this issue Dec 21, 2022 · 8 comments
Labels
enhancement New feature or request

Comments

@dnicolodi
Copy link
Member

In #231 I need to make use to of typing definitions added only in Python 3.10, thus making typing-extensions a requirement for most of the supported Python version. It is quite annoying to have an extra dependency only for type checking. I think the dependency should be made a test only dependency.

@rgommers
Copy link
Contributor

I agree with that sentiment. Adding a runtime dependency that is never for anything at runtime, and used for one type annotation object, feels wrong. As dependencies go, typing-extensions is pretty light weight, but still.

@henryiii
Copy link
Contributor

henryiii commented Dec 22, 2022

If anyone uses the types downstream and typing-extensions is missing, this adds a hidden undeclared dependency. There's a pretty good chance something else depends on typing-extensions, but you technically are broken if a user don't have it. (Not sure this even has py.typed, but this is a bug I've seen for libraries).

IMO, any backport dependency should be treated as a 0-cost dependency. If someone complains about it, they can upgrade their Python and it goes away. As long as 3.8, 3.10, or 3.11+ doesn't require the dep, I'd say it's totally fine to depend on it on older versions. Same is true for tomllib, exceptiongroup, or any other backport. The older versions eventually won't matter. This happened with dataclasses, and now code prevously using it got to drop the dep and now has code that looks like it was designed for 3.7+. Maybe it's even less than 0-cost.

FYI, I usually have a module like https://github.com/scikit-build/scikit-build-core/blob/main/src/scikit_build_core/_compat/typing.py for types that can come from multiple places, and it would be pretty trivial for a small number of types to guard them with TYPE_CHECKING, so it could be done this way to ensure easy cleanup in the future if you really want to.

@rgommers
Copy link
Contributor

If anyone uses the types downstream and typing-extensions is missing,

We don't have a Python API, so how is this possible?

@henryiii
Copy link
Contributor

We don't have a Python API, so how is this possible?

That's what I meant by:

Not sure this even has py.typed, but this is a bug I've seen for libraries

And that's why I also said it could be done if you really wanted too.

@rgommers rgommers added the enhancement New feature or request label Dec 23, 2022
@rgommers
Copy link
Contributor

rgommers commented Dec 23, 2022

I had a closer look: this was already a dependency, but now it's a much stronger one. EDIT: This dependency was/is missing in conda-forge, which now matters. I've had to update the metadata for this in Spack, it was <3.8 The metadata also needs a tweak in all other downstream packaging systems. The change isn't listed in our Changelog, so that will probably go wrong in most places.

tl;dr this is certainly not a zero cost dependency

There is zero usage of static typing downstream, plus extra dependencies have a small overhead at runtime (downloads, build time). So I think @dnicolodi's proposal is the right one - best to ensure we don't have this dependency at all except for meson-python development purposes.

@eli-schwartz
Copy link
Member

Adding a runtime dependency that is never for anything at runtime, and used for one type annotation object, feels wrong.

This argument is more compelling in projects like Meson where type annotations that are never used for anything at runtime are defined/imported in T.TYPE_CHECKING as a matter of standard policy (other than, obviously, the typing module itself). Admittedly we also frequently import project-specific types across 215 files, not 6, so there's a significantly higher rate of types that are needed as annotations and not explicitly instantiated, so this has circular import relevance as well...

IMO it also has beneficial effects to annotate functions with T.Obj:

  • on code clarity to indicate where they came from
  • on code review instead of frequently updating the list of types being imported

but 🤷

@rgommers
Copy link
Contributor

Meson where type annotations that are never used for anything at runtime are defined/imported in T.TYPE_CHECKING as a matter of standard policy

I have to say I do like this policy. I'd be +1 on adopting it in meson-python.

@dnicolodi
Copy link
Member Author

Implemented in #255.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants