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 an (optional?) astropy dependency #10689

Closed
nstarman opened this issue Aug 28, 2020 · 31 comments
Closed

Make typing_extensions an (optional?) astropy dependency #10689

nstarman opened this issue Aug 28, 2020 · 31 comments

Comments

@nstarman
Copy link
Member

nstarman commented Aug 28, 2020

Question

How should Astropy implement typing?
What is the timeline?
What Infrastructure needs to be built to enable this? e.g. mypy checker in test suite or submissions to python's type-shed.

OLD Description

See #10662

The python typing library is under active development and new and useful features are not available for py3.6-3.8. The typing_extensions library is now the official backport for all these types and is even developed within the main body of python (though offered as a separate install) — see https://github.com/python/typing/tree/master/typing_extensions.

As astropy moves to add static typing, the extended types options, in addition to the guaranteed stability and long-term support, would make this a useful and safe dependency.

I'm making this an issue, not PR, because I know how contentious adding new dependencies can be.

Edit: this would probably not be a permanent additional dependency since the features in typing_extension all become core python. When new features stop being added regularly and our python minveraion catches up, this can be dropped.

Edit: Now suggesting as an OPTIONAL dependency

@nstarman nstarman changed the title Make typing_extensions an astropy dependency Make [typing_extensions](https://pypi.org/project/typing-extensions) an astropy dependency Aug 28, 2020
@nstarman nstarman changed the title Make [typing_extensions](https://pypi.org/project/typing-extensions) an astropy dependency Make typing_extensions an astropy dependency Aug 28, 2020
@dhomeier
Copy link
Contributor

The python typing library is under active development and new and useful features are not available for py3.6-3.8.

Could you clarify which (especially non-experimental) features it would actually add for newer Python versions? The project page primarily discusses 3.4-3.6, and those alone might weaken the case a bit, since the schedule for dropping 3.6 support is already under discussion.

@nstarman
Copy link
Member Author

nstarman commented Aug 29, 2020

What I meant was that there are a number of types which are py3.7+ or py3.8+ or even py3.9+. Until our minimum version is 3.9, then not all the static type will be available.

To me personally, the most useful are:

  • The Annotated type (py3.9+ in typing) which allows for metadata to be associated with static types. This would have immediate use in add unit-aware quantity annotations. #10662 and also be applicable to the annotations in the Modeling module.
  • Literal (py3.8+) which indicates a value equivalent to the literal, for instance
def func(x: Literal["a", "b"]):
    ....

means x=a or b.

  • get_args (py3.8+) for breaking apart Union and Annotated types

Also useful are:

  • @runtime_checkable (py3.8+) for a custom class to be used with isinstance and as a static type check.
  • The Protocol type (py3.8+) enables structural subtyping.
  • OrderedDict for annotating all the ordered descriptors in astropy
  • TypedDict (py3.8+) for dictionaries that must contain a certain set of keys, like how BaseCoordinateFrames are implemented.
  • Final (py3.8+) to prevent subclasses from changing a value. This will be useful all over the place, to annotate protected attributes without having to make them private with single or double underscores.

There are also PEPs for adding other types, like 603 for frozen maps, and 604 for the pipe operator to create union types. These will be implemented in the typing_extensions backport long before astropy's minimum version catches up.

It would be amazing, though perhaps a pipe dream, to be able to fully static type astropy and compile into fast C.

Edit: also numpy's ArrayLike and DTypeLike rely on typing_extensions until py3.8+

@pllim
Copy link
Member

pllim commented Aug 29, 2020

This adds maintenance burden for the core maintainers, especially given that our minversion for Python has not caught up to your needs yet. Some cost-benefit discussions are needed.

@dhomeier
Copy link
Contributor

  • OrderedDict for annotating all the ordered descriptors in astropy

Isn't that part of collections since 3.5?

@nstarman
Copy link
Member Author

Beyond ensuring that a package has consistent and equivalent functionality across all supported python versions, I'm not that familiar with the cost aspect of adding dependencies. Could you highlight a couple so I can look into them and how they relate to the typing_extensions package?

@nstarman
Copy link
Member Author

  • OrderedDict for annotating all the ordered descriptors in astropy

Isn't that part of collections since 3.5?

Not the dedicated static type. This one is actually more flexible since its a Generic, so any ordered mapping falls under its purview.

@dhomeier
Copy link
Contributor

I think the only package beyond numpy that has ever (just) been accepted as a required dependency is pyerfa #10329. Arguably typing_extensions seems a much lower hurdle to install...

@nstarman
Copy link
Member Author

nstarman commented Aug 29, 2020

I've been reading the discussion around pyerfa and it seems one of the main points under consideration was pinning versions of dependencies. typing_extensions promises LTS for py3.5+, so there's no problem for minimum versions, I think all that would need to be done is set the minimum required version to the version of the latest-used feature. I think the only problem that could happen is if some other package set a conflicting version requirement; but honestly, given the backport nature of the package, I don't see why that would ever happen.

@nstarman
Copy link
Member Author

nstarman commented Sep 1, 2020

@mhvk I know we've loosely discussed the Annotated type in #10655. What do you think about a run-time dependency for that and the other static types?

@saimn
Copy link
Contributor

saimn commented Sep 4, 2020

Numpy's next major release will have types information, and they are using typing_extensions but as an optional dependency: https://github.com/numpy/numpy/search?q=typing_extensions&unscoped_q=typing_extensions
https://github.com/numpy/numpy/blob/5b5a74043985b5cf577cd14b9a326b8eeb13c11a/numpy/typing/__init__.py#L8-L23

@nstarman
Copy link
Member Author

nstarman commented Sep 4, 2020

Thanks for pointing this out! I'm looking at the code, if typing_extensions is not present than ArrayLike and DTypeLike become the generic type Any.

@taldcroft
Copy link
Member

I'd vote for optional dependency. Astropy has a pretty strong policy of keeping absolutely required dependencies at the bare minimum.

@taldcroft
Copy link
Member

It would be amazing, though perhaps a pipe dream, to be able to fully static type astropy and compile into fast C.

Many of us consider dynamic typing to be a feature, not a bug. 😄

@nstarman
Copy link
Member Author

nstarman commented Sep 26, 2020

I'd vote for optional dependency. Astropy has a pretty strong policy of keeping absolutely required dependencies at the bare minimum.

@taldcroft Thanks for the suggestion. I've been looking at implementation with regards to #10662 , and I think this would be a workable solution. Anyone who wants type annotations can use the optional dependency. The same feature set for Quantity type hint annotation is provided by a simple compatibility class (already implemented).

I'm not sure how these types of issues normally progress. Is there a voting mechanism, or what?

@nstarman nstarman changed the title Make typing_extensions an astropy dependency Make typing_extensions an optional astropy dependency Sep 26, 2020
@taldcroft
Copy link
Member

I'm not sure how these types of issues normally progress. Is there a voting mechanism, or what?

It does not seem there is very strong objection to adding this package as an optional dependency, but @pllim mentioned a point early in the discussion about maintenance burden. Does that still apply for an optional dependency? We definitely want the infrastructure maintainers on board with the change and they need to approve.

@pllim
Copy link
Member

pllim commented Sep 28, 2020

maintenance burden. Does that still apply for an optional dependency?

Yes, it does. For example, remember that time when pandas release broke Table doctest?

@mhvk
Copy link
Contributor

mhvk commented Sep 28, 2020

The argument here would be that python and numpy are very much moving towards typing, and since typing_extensions is maintained by the python team, I worry a bit less about breakage. It also solves quantity_input quite nicely.
@nstarman - one question I guess is whether you'd be willing to be trouble-shooter in case something does break.

@pllim
Copy link
Member

pllim commented Sep 28, 2020

I guess it will be a while before our minversion will be Python 3.8. 🤔

I am not against it, but I just want us to consider all angles before proceeding.

@nstarman
Copy link
Member Author

@nstarman - one question I guess is whether you'd be willing to be trouble-shooter in case something does break.

@mhvk, I would be willing to trouble-shoot issues as they arise. I've taken an interest in learning static typing with regards to my own code, and would be happy to contribute that to Astropy.

nstarman added a commit to nstarman/astropy that referenced this issue Oct 6, 2020
works better with astropy#10689, but even without.

Signed-off-by: nstarman <nstarkman@protonmail.com>
@nstarman nstarman changed the title Make typing_extensions an optional astropy dependency Make typing_extensions an (optional?) astropy dependency Oct 10, 2020
@nstarman
Copy link
Member Author

nstarman commented Oct 10, 2020

@embray discussion points about the requests package seem relevant to this discussion as well. To paraphrase and insert typing_extensions for requests

I think the community stands to benefit long-term from using first-class, well-supported third-party packages, where possible, for "technical" code that is not particular to Astropy. In particular it can alleviate us of the responsibility of
maintaining such code... [typing_extensions] is so widely used now that it would be hard to find a Python distribution that doesn't already have it installed. [It is developed in the] standard library.

While having typing_extensions as an optional rather than required dependency is fine, it introduces a bit of non-astro, "technical" code (see python version checks and codecov issues in #10662).

@nstarman
Copy link
Member Author

I'm revisiting #10662 and re-encountering the problems related to not having an Annotated annotation from typing. While first applicable to the units package, the Quantity[unit] would also be of benefit to the modeling package, which can use unit annotation in custom model definitions.

typing_extensions is the backward-compatible library for accessing py3.9+ features like Annotated. Just a resummary of typing_extensions, it is:

  1. developed in the main repository of python.
  2. maintained by GVR and the core python developers.
  3. (supposedly) compatible with any computer that has pip and python 2.7+.
  4. part of python and Numpy's solutions to static code analysis for older python distros.

There a larger conversation to be had on how Astropy wants to implement typing, but ``typing_extensions``` will certainly be one of the dependencies required to do this in a way compatible with python 3.7 and even 3.8. While NumPy is making this an optional dependency, my hope is that it doesn't seem far fetched to make it a dependency here... 🤞

@embray
Copy link
Member

embray commented May 17, 2021

Just to add my $0.02, based on my experience with my own recent forays into static typing with Python, having typing_extensions is absolutely crucial.

I just wasn't aware that there was any concerted effort yet to make Astropy fully type-aware (though it seems like it would be a good thing to do incrementally.

@nstarman
Copy link
Member Author

it seems like it would be a good thing to do incrementally

Agreed. Probably preaching to the choir here, but Mypy has incremental typing both in package scope and strictness. The easiest way to do typing would probably be to choose a subpackage (maybe a module in utils) that doesn't work with Units or Coordinates or anything that needs custom Astropy type hints and start building from there: get the Mypy.ini; see if mypyc can transpile the code, etc.

@taldcroft
Copy link
Member

I'd be interested in hearing any applicable case stories to show how spending (possibly considerable) time and effort to make astropy fully type aware would benefit. I'm not necessarily against working toward this goal, but I do think we need to have some justification for the effort and consider the costs.

We do have to remember that many contributors to the core are not likely to be up to speed on writing typed code, so making the entry barrier for contributions even higher is a real concern.

@mhvk
Copy link
Contributor

mhvk commented May 18, 2021

I share @taldcroft's fear, although I guess in a way it is just provides an easier-to-parse (computationally at least) method to express what we now do in the Parameters section. But I do wonder how it would work in practice for initializers for classes like SkyCoord or Time....

That said, I still like very much the implementation for quantity_input in #10662, where one can decorate arguments with things like Quantity[u.m].

p.s. numpy has gone the route of having a separate set of files (in numpy.typing) - a benefit of that is that those who like to use typing can, but it is not enforced on those who'd rather not.

@taldcroft
Copy link
Member

I will say that I've become a pretty big fan of VS code + Pylance, and so I am beginning to see the benefit in my own workflow of type inference to allow for improve autocompletion and docs. I've actually been pretty amazed sometimes that it figures out some variable is a Table or whatever and gives me the right method auto-complete.

From that perspective I'd be happy if there was a type annotation that said table.hstack returns a Table. Except that if you stack QTables it returns a QTable. So I have no idea how that works, though saying it is Table seems better than nothing at all.

@nstarman
Copy link
Member Author

nstarman commented May 18, 2021

@taldcroft

I'd be interested in hearing any applicable case stories to show how spending (possibly considerable) time and effort to make astropy fully type aware would benefit. I'm not necessarily against working toward this goal, but I do think we need to have some justification for the effort and consider the costs.

IMO there are a number of pros. Hopefully they outweigh the cons:

  1. with mypy we can do static type annotations gradually. Astropy may never by fully type aware. Difficult sections or areas under active development can be skipped for later efforts. The time and effort will be amortized over a long period.
  2. Static code analysis finds bugs, unreachable code, and various other mistakes.
  3. autocomplete in fancy things like VSCode
  4. Speed: https://mypyc.readthedocs.io/en/latest/introduction.html#why-mypyc . This is pretty amazing, is compatible with mypy + python, delivers up to 4x speed boosts, and supports gradual typing so we can do this one file at a time.

We do have to remember that many contributors to the core are not likely to be up to speed on writing typed code, so making the entry barrier for contributions even higher is a real concern.

Agreed. This could be problematic. OTOH, in my experimentation with typed code, I've generally found that mypy and other static code analyzers have helped me a) improve my docstrings to be more accurate about function parameters and output and b) actually write clearer, more atomic code, since fancy tricks and Frankenstein functions make mypy unhappy. Some well-typed code may be easier to contribute to, while other functions will be harder. I think one of many arguments for mypy is that gradual typing will enable us to choose, at a pretty granular level, what to type hint. Code in active development we can leave untyped, if we so choose. Very mature code that really only gets occasional bug fixes can be fully typed.

@mhvk

p.s. numpy has gone the route of having a separate set of files (in numpy.typing) - a benefit of that is that those who like to use typing can, but it is not enforced on those who'd rather not.

That is certainly one option. I personally am not a huge fan of all the file duplication. But it's presumably is easier for other packages to find / build a typeshd.

@mhvk
Copy link
Contributor

mhvk commented May 18, 2021

Slightly more on-topic, I actually do not have much of a problem with making typing_extensions a dependency, since it is part of the standard library in more recent pythons.

@embray
Copy link
Member

embray commented May 20, 2021

p.s. numpy has gone the route of having a separate set of files

I would also be +1 to having a separate hierarchy of type stubs. It's a bit of a pain to have to maintain two separate source hierarchies but it also would keep it out of the main codebase. One thing I don't like about typing and that kept me away from it a long time is having type hints everywhere really clutters the code up, especially if you're not used to reading python with type hints.

@taldcroft

From that perspective I'd be happy if there was a type annotation that said table.hstack returns a Table. Except that if you stack QTables it returns a QTable. So I have no idea how that works, though saying it is Table seems better than nothing at all.

There's quite a number of facilities behind exactly this sort of use case. For the case of hstack for example, very schematically you might have something like:

from astropy.table import Table, QTable
from astropy import units as u
from typing import TypeVar, List


T = TypeVar('T', bound=Table)


def hstack(tables: List[T]) -> T:
    ...


t1 = Table({'a': [1, 2, 3]})
t2 = QTable({'a': [1, 2, 3] * u.m})  # type: ignore


reveal_type(hstack([t1, t1]))
reveal_type(hstack([t2, t2]))
reveal_type(hstack([t1, t2]))
reveal_type(hstack([t1, 'asdf']))

Running mypy on this gives:

$ mypy table.py
table.py:17: note: Revealed type is 'astropy.table.table.Table*'
table.py:18: note: Revealed type is 'astropy.table.table.QTable*'
table.py:19: note: Revealed type is 'astropy.table.table.Table*'
table.py:20: error: Value of type variable "T" of "hstack" cannot be "object"
table.py:20: note: Revealed type is 'builtins.object*'

It finds an error on the last line because 'asdf' is not a Table. The variable T can represent any Table subclass, and this expresses that if you hstack two Table it will return a Table (or subclass thereof) based on the types of the inputs. I admit I'm not sure what happens with the real hstack if you combine a Table with a QTable. If it returns a QTable in this case I'm not exactly sure how to capture that in type hints, but it does know it at least returns a Table.

@nstarman
Copy link
Member Author

I think this specific issue will be resolved by #10662, which adds typing_extensions as an optional dependency.
So perhaps the discussion of that could move there.
I feel pretty comfortable adding it as an optional dependency for the simple reason that it's maintained as part of the main python code by the core python developers.

I think the related discussion on how to implement typing in astropy is well worth having and here might be the place.
I'll change the issue title and put a PSA in the top comment.

p.s. numpy has gone the route of having a separate set of files

I would also be +1 to having a separate hierarchy of type stubs. It's a bit of a pain to have to maintain two separate source hierarchies but it also would keep it out of the main codebase. One thing I don't like about typing and that kept me away from it a long time is having type hints everywhere really clutters the code up, especially if you're not used to reading python with type hints.

@embray. I'm very far from an expert in typing, so I'm trying to determine if type stubs can be mixed with integrated type hints. The questions I'm trying to figure out are: can each subpackage make a separate decision and everything play nice, or can mypy etc only understand one format? Can the same be applied within a subpackage?
I tentatively think the answer to both questions is yes... but any insight would be appreciated.

@nstarman
Copy link
Member Author

nstarman commented Nov 9, 2021

typing_extensions was added as an optional dependency in #10662.

@nstarman nstarman closed this as completed Nov 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants