Add script to make type stub for units - #18547

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

• Do the proposed changes actually accomplish desired goals?
• Do the proposed changes follow the Astropy coding guidelines?
• Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
• Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
• Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
• Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
• Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
• Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
• At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

Note: ruff check seems to be a false positive. There's only one "real" import.

I like the idea to generate type stubs and to tie it into CI in some way.
Things to consider:

1. do these stubs elide the other imports? I think the contents of __all__ need to be added.
2. Maybe make this a script that can be called outside of CI?
   This relates to your pre-commit.ci suggestion, though I'm not sure we want it at the pre-commit stage rather than a post-commit.

@nstarman: Thanks for the quick reply.

If I understand your comment correctly: this stub iterates through astropy.units.all, but then makes a selection as for what types to import. This is in the spirit of "good enough". Sorting through the long tail will require some time and this way we get 98% of the types with little effort.
Will happily put it wherever you maintainers think it fits best.

I think we want to import everything that's in __all__, which defines the public API. In particular what first tripped my notice was Quantity

@nstarman: Thank you clarifying and pointing out Quantity. I reworked the code to handle classes as well. Now only the types functions, functools._lru_cache_wrapper, method, and DataInfoMeta are missing... and, again, I think from experience, good enough is usually a good strategy. It can always be improved upon as the need arises, and will save us development time.

Great. The generation LGTM.
I think we can figure out the CI of this — including committing the changes — in a followup PR.
But for this one can you add unit tests for the generated pyi file?
Like in Astropy/units/tests use a tempdir and call main and show a few imports.

The main objection to #17902 was the size of the generated files, and this pull request must have the same problem.

I think the difference being the focus on autogen.
In #17902 my objection wasn't wasn't the resulting file size but the initial file size — balking at committing 5k lines that had been generated by an external script, e.g. see #17902 (comment).
I do think we can solve this with autogen + CI (see above comment).
I think the CI bit can be for a different PR, but I'm thinking about ...

1. As part of CI, before unit/etc tests are run check if astropy/units has been changed. If so run this script. Check the git diff. If different, auto-commit, ignoring any git-ignore. Probably also check that the only committer to the pyi files are GH actions.
2. Add the pyi files to .gitignore so that only GH Actions can add them to our git history
3. Expose the script as a [project.scripts] so devs can trigger the stub gen (probably also how GHA would do it). .gitignore ensures this isn't committed.

I think @mikez's script is a good contribution.
If we all like the autogen route, then maybe @eerovaher we merge in your script from #17902 and build the CI stuff to autogen all the necessary stubs.
Ping @mhvk for additional thoughts.

@eerovaher Thanks for the pointer to your commit. I missed that one. I agree with @nstarman here; for maintenance reasons, these files should be auto-generated by a bot – then file sizes aren't an issue. I'm glad we both had the same intuition that generating stub files are a good idea. :)

@nstarman I like your route. CI helps with maintenance, .gitignore protects against humans accidentally committing, and [project.scripts] ensure we or bots can run/test it as needed.

I had two additional thoughts:

1. We could go the [project.scripts] route before we even add it to CI... that way anybody who wants stubs can generate them today already... and we'll get an opportunity to test-run it in practice.
2. CI: Checking if astropy/units has changed... would you do this by checking if the prefix of any changed file has astropy/units in it? Or were you envisioning something more involved?

@nstarman If we take the [project.scripts] route, what would be a good home for the script? If so, I'll move it there, update pyproject.toml and .gitignore and then we can discuss CI in another PR.

Thanks for the ping. I like the approach here, and like even more the idea to put this in [project.scripts] or another place where any user can execute it, so that those interested in typing can run with it already and check that things work as expected.

I also really like how this separates concerns: after this PR providing the script, we can add automated tests that ensure that whatever it produces is up to date (probably mostly the unit test that is to be included here), and then find out how best to ensure the .pyi files gets auto-included on pypi (or auto-generated as needed? a bit like .pyc files?).

On the home for the script, given the precedent by the other sub-packages in pyproject.toml, I guess astropy.units.scripts.<name> would seem best. I'd change the name to make_units_type_stubs or just units_type_stubs, because the scripts are all going to be in one place as far as the user can see, so that avoids name collisions.

@mhvk I like your approach. I moved the script to astropy/units/scripts and renamed it to typestubs_for_units.py. For the pyproject.toml, I called it typestubs-for-units. Just tested it on my machine and it worked great.

Super! To do still is a test. Note also my comment about possible arguments for the script - if we go the route of allowing an alternate path, testing would be even easier.

@mhvk What did you have in mind in terms of testing? I'd prefer not to over-complicate this for maintenance reasons.

Responding to #18547 (comment)

I think the difference being the focus on autogen. In #17902 my objection wasn't wasn't the resulting file size but the initial file size — balking at committing 5k lines that had been generated by an external script, e.g. see #17902 (comment). I do think we can solve this with autogen + CI (see above comment).

What difference does it make if the stubs are generated in CI or not? The size of the stub files depends on how many objects need to be annotated and that is the same either way.

2. Add the pyi files to `.gitignore` so that only GH Actions can add them to our git history

Git documentation makes it clear that "A gitignore file specifies intentionally untracked files that Git should ignore. Files already tracked by Git are not affected;" In other words if the stubs have been committed then them being listed in .gitignore makes no difference.

Responding to #18547 (comment)

I like the approach here, and like even more the idea to put this in [project.scripts] or another place where any user can execute it, so that those interested in typing can run with it already and check that things work as expected.

The standard way of distributing optional stubs annotations is by publishing them as a stub-only package. I sounds like you are attempting to invent a custom way of distributing optional type stubs, but you haven't justified why the standard way isn't good enough.

we can add automated tests that ensure that whatever it produces is up to date (probably mostly the unit test that is to be included here),

It would be simplest to use stubtest that is bundled with Mypy. But stubtest will not approve astropy unit stubs no matter how they are generated because astropy dynamically inserts entries into the unit definition module __all__ lists that are duplicates under NFKC normalization. Addressing that requires code changes in astropy.

and then find out how best to ensure the .pyi files gets auto-included on pypi (or auto-generated as needed? a bit like .pyc files?).

Why was the size of the stub files a problem for #17902 but not here?

ruff side-remark: I001 (unsorted-imports); does anyone know why ruff wants to place sys before pathlib and collections? That seems like a weird sorted order for me. To keep the sorting order, I personally use isort here and not ruff.

@eerovaher All your remarks make sense to me, thank you for looking into this and helping me understand stubs better! Maybe we can split the CI/.gitignore-related issues into a separate discussion, since this PR will only handle adding the script and putting it in pyproject.toml.

The standard way of distributing optional stubs annotations is by publishing them as a stub-only package.

Wonderful remark. Thank you! This seems to be the method chosen by pandas, see here. They also keep some internal stubs for development here.

As far as I understood, this approach can lead to a significant maintenance burden and double-maintenance (since two repos need to be kept in sync). In that respect, I think we'd be clever here to choose a lightweight solution that auto-generates only astropy.units for now so as not to make the same mistake. (Also, as you suggested, use other development strategies like inline annotations and rethinking the __all__ approach down the line so we don't need to generate PYI files at all.)

It would be simplest to use stubtest that is bundled with Mypy.

I see pandas uses that approach for their internal stubs here. However, if I understood you correctly, their stubtests approach won't work here. So we'd need to figure out another clever solution if we want to test this at all. I'd be happy to handle this testing in another PR, so we can give units stubs to everyone who needs them... and if the approach turns out to be the wrong one, it's an easy fix.. just delete one line in pyproject.toml and the file in units/scripts/.

In short: experience has taught me, it's good to have a simple fix for now that's good enough for daily work (for those that want it) and then in parallel work on a more long-term solution that keeps our maintenance burden low.

My main wish would be not to have auto-generated stubs in the git parts (I'd like to similarly get rid of the auto-generated lextab, parstab files). I mind much less if the stubs are distributed along with the package on pypi, but a separate package is arguably better, so if that is the standard, that's great.

Anyway, as noted by @mikez, the nice part of the present approach is that for this PR, we do not have to worry about the details of how the stubs are distributed. Though obviously we should not do something that precludes standard methods.

As for tests, maybe spot checks that the types are present and work. @nstarman seems to have had a more concrete idea in #18547 (comment)

Thank you @eerovaher, for your patience. I didn't understand you at first, but now it clicked.
We could easily fix our current script to include def_unit and generate just __init__.pyi; however, why complicate the script even more, when we can make it easier... by leveraging what the type checker already is good at. :)

I hope, for my health and teeth grinding, we can all agree on this version as a first step. 😅

@mhvk Thank you, again, for your guidance here. Much appreciate it. I slightly reworded the changelog and added it to the commit.

Generating type stubs should not be the responsibility of the users and the reason why the three options for distributing type stubs I listed in #18547 (comment) are the only ones worth considering is that those are the three options that don't make type stubs the user's problem. With options 1) or 2) type stubs would be the responsibility of astropy developers and with option 3) they would be the responsibility of the astropy-stubs developers. We decided not to choose option 1) because the type stubs for units are too large and according to our documentation we cannot choose option 2):

#17902 isn't good enough and this pull request isn't any better. The very best outcome from this pull request would be that it does not cause any confusion, but it already has.

@mikez, if you want to improve astropy annotations then why don't you add inline annotations to astropy/units/quantity.py? Quantity is a very important class, but that file only has a handful of annotations.

@mikez, if you want to improve astropy annotations then why don't you add inline annotations to astropy/units/quantity.py? Quantity is a very important class, but that file only has a handful of annotations.

@eerovaher I'd like you to hear me differently. My aim was not to improve annotations... my aim was to make using astropy easier for beginners like myself without seeing all those type checker errors on u.deg, u.m and so on. My aim was not to criticize your approach, rather to do a small incremental "good enough" improvements, rather than a perfect solution. I'm grateful for the insights you've given, thank you again, they've made the code better.

@mhvk @nstarman Maybe you can help me take it from here since it seems @eerovaher isn't willing to meet in the middle. I tried my best, but I only have so much energy.

Introducing a script that the users are supposed to run to generate the type stubs is fundamentally a bad idea and, independently from the above, the script here does not address any of the shortcomings #17902 had. This pull request would have been very valuable if it had been opened before #17902, but it was instead opened 5 months after #17902 came to a better conclusion.