Import standard library types at runtime#18191
Conversation
|
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.
|
There was a problem hiding this comment.
This probably won't fully resolve Sphinx-caused issues, but it's bound to help!
And it will also help with runtime type checkers which is an added bonus
Edit: let me eat my words. Sphinx has many problems with resolving type annotations.
Edit: I think the error here is in Python, not Sphinx! types.EllipsisType isn't explicitly documented in the types module so intersphinx isn't seeing it.
import urllib.request
import zlib
url = "https://docs.python.org/3/objects.inv"
with urllib.request.urlopen(url) as f:
inv = f.read()
print(b"types.EllipsisType" in inv)
# FalseWhat are our options? And what solution might automatically propagate downstream?
|
I see this goes back to sphinx-doc/sphinx#9813 which is commented as being tricky to solve. The solution here seems just partial, though: if it were so easy, wouldn't |
|
Why were they in |
|
@pllim it was driven by 2 things: a ruff config to prefer to put type-time imports in a type-checking block and the thought that it could prevent increased import times. The former has come under flack for the difficulty it causes runtime type checkers (and sphinx). The latter can be true, but doesn't appear to be noticeable in most (any?) cases thus far. |
|
Thanks, @eerovaher and @nstarman, interesting to see that the whole |
|
Stdlib and 3rd party shouldn't be problematic, but yeah 1st party is interesting.
Do
|
So far imports that are needed only for type checking have been placed inside `if TYPE_CHECKING:` blocks to avoid importing them at runtime. However, because of bugs in Sphinx runtime imports should be preferred.
eb78e51 to
ac60737
Compare
|
I already had to fix a merge conflict and the longer this stays open, the more likely that I will have to fix more. To prevent this from getting stuck in review I am setting a deadline for next Thursday (2025.06.12). If this is not merged by that time then I will limit this pull request to the sub-packages with approvals and start opening separate pull requests for the remaining sub-packages, one sub-package per pull request. @nstarman, did you approve only for |
taldcroft
left a comment
There was a problem hiding this comment.
Looks good for io, utils. I also accidentally reviewed all the io/builtin/ files and they seemed fine.
|
@eerovaher. Both. |
|
Basically all subpackages have the same kind of fixes, so if we decide that runtime import isn't a problem, this is probably good to go. Any outstanding discussions left? |
|
Runtime imports from the standard library should not be controversial and this pull request is not concerned with third party or first party imports. |
pllim
left a comment
There was a problem hiding this comment.
Since this does fix downstream doc build, and there are 2 approvals, already, let's get this in. If more downstream packages (besides astroquery and specutils) report a problem, then at least we can just do a bugfix release then.
Thanks!
p.s. If you can think of a good way to avoid future typing stuff from breaking downstream before merge, please share your ideas. Even with #17861 , it does not check doc build. Having a CI to build all downstream doc from a PR branch here would be unexplored territory.
|
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free to suggest an improvement. |
This comment was marked as resolved.
This comment was marked as resolved.
Agreed this is a tricky problem. The issue isn't that the types aren't valid – they are - but that sphinx et al has a damnable time rendering some types under certain conditions. I'm not sure how we effectively test for that. |
Backport PR #18191 on branch v7.1.x (Import standard library types at runtime)
Description
So far imports that are needed only for type checking have been placed inside
if TYPE_CHECKING:blocks to avoid importing them at runtime. However, because of bugs in Sphinx runtime imports should be preferred.This pull request should be backported to solve some documentation build problems downstream packages might have (such as astropy/astroquery#3327).
Approvals by sub-package
configcoordinatescosmologyImport standard library types at runtime #18191 (review)ioImport standard library types at runtime #18191 (review)modelingstatsunitsImport standard library types at runtime #18191 (review)utilsImport standard library types at runtime #18191 (review)Sub-packages that have not been approved by 2025.06.12 will be updated in separate pull requests.