Add more type annotations to config#17546
Conversation
That the annotations are self-consistent can be verified with `mypy --follow-imports silent astropy/config/*.py`. Furthermore, the annotations in `path.py` are good enough that `mypy --strict --follow-imports silent astropy/config/paths.py` succeeds.
|
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.
|
nstarman
left a comment
There was a problem hiding this comment.
LGTM.
We should have a future discussion about turning off TC003 and fixing that Astropy-wide.
See beartype/beartype#440 (comment) for reasons why.
neutrinoceros
left a comment
There was a problem hiding this comment.
Thanks for doing this !
I think your additional annotations are making it very clear how I should have designed the fallback mechanism in my latest refactor (#17188), and I think now is as good a time as any to make the correct decision. Other than this detail, LGTM !
| ) | ||
|
|
||
| def __str__(self): | ||
| def __str__(self) -> str: |
There was a problem hiding this comment.
is this needed ? it seems that mypy is happy with leaving this annotation implicit: there's the same method, left un-annotated, in Conf. I don't mind the extra annotation, but I guess it should at least be applied (or left out) consistently.
There was a problem hiding this comment.
it seems that mypy is happy with leaving this annotation implicit:
Mypy does not check unannotated functions unless it is configured to do so, so if the return type annotation is removed then Mypy will be happy with this function regardless of what its contents are.
there's the same method, left un-annotated, in
Conf.
This is how Conf.__str__() begins:
astropy/astropy/config/configuration.py
Lines 91 to 94 in 2f43420
Mypy doesn't like that because the type of
self.__doc__ is str | None, so in the None case trying to access the strip() method will cause an AttributeError. That's not really a problem for this method because it will catch the AttributeError, but type checkers don't like using exceptions for control flow like that.
I don't mind the extra annotation, but I guess it should at least be applied (or left out) consistently.
The majority of astropy has been written without taking type checking into account at all. This makes annotating astropy is complicated enough even without any additional consistency or coverage requirements.
There was a problem hiding this comment.
The majority of astropy has been written without taking type checking into account at all. This makes annotating astropy is complicated enough even without any additional consistency or coverage requirements.
definitely fair enough ! I didn't mean to make your experience even more painful.
|
|
||
| def _get_dir_path(rootname: str, cls: type, fallback: str) -> Path: | ||
| def _get_dir_path( | ||
| rootname: str, cls: type[_SetTempPath], fallback: Literal["cache", "config"] |
There was a problem hiding this comment.
I think your new annotations are revealing a small design issue with these functions:
if fallback can only be exactly 'cache' or 'config', shouldn't this mean that cls can only be exactly set_temp_cache or set_temp_config ? We could also naturally exclude invalid combinations like cls=set_temp_cache, fallback='config' by simply removing the fallback argument and making it a class attribute instead (in which case we can also keep the generalized annotation you used for cls)
There was a problem hiding this comment.
It may indeed be the case that the code can be refactored, but I'd prefer to keep the typing updates separate from code changes.
There was a problem hiding this comment.
Okay then, I'll merge this now and propose the refactor myself. Thanks !
|
FYI: this broke the astroquery docs build as the types are not intersphinx resolveable once you use it all downstream. |
|
For more context, from #18144 (comment), the relevant astroquery issue is astropy/astroquery#3326. The reason for this seems to be that sphinx has problems resolving types, and that this implies that when we add typing to things that get directly used downstream, like the config system, we are causing problems. This has an important implication: we cannot just work around sphinx issues locally, we have to be sure that this work-around is such that downstream doesn't suffer - they shouldn't have to put astropy stuff in nitpick-exceptions (as in astropy/astroquery#3327). Not sure how to actually go about it (or even how to guard against it), though I guess generally it implies that if one of our dependencies (sphinx in this case) causes problems, we really should aim to solve that in upstream rather than work around it. |
|
@Cadair , SunPy uses astropy config, right? Did you get bitten by this as with astroquery? |
|
Interestingly enough for https://github.com/spacetelescope/synphot_refactor/blob/master/synphot/config.py With nitpicky on: And the doc still builds fine with astropy v7.1 https://app.readthedocs.org/projects/synphot/builds/28338631/ 🤷♀️ |
|
are you doing |
|
Re: inherited-members Ah, maybe not. I thought at some point inherited became true by default but I am unsure. Just for clarity, where exactly is the astroquery doc affected by this, so I can compare with my own code. Thanks! |
|
I don't have an RTD link for you, but if locally you build the docs in e.g. 85d1cd3d7a8e5c82ebf3f899275f2724400fb57b you will see the warnings triggered by mast, ukidss, and vsa. The issue is on e.g. |
|
Ah okay, it looks like my Past Self ™️ did not bother to put whereas astroquery does document it, e.g.: https://astroquery.readthedocs.io/en/latest/api/astroquery.mast.Conf.html#astroquery.mast.Conf |
|
yeap. Actually we don't really need to show the inherited members for Conf(), but we do need that for other parts of the module, and afaik there is no way to selectively pick and choose. |
To do that, I think you have to be more fine-grained with the API doc directive, like specifically calling something like this: ... automodapi:: astroquery.mast.Conf
:no-inherited-members:Which also means you have to document the other parts separately, which is probably not worth the trouble. |
|
That is another workaround that would not provide what we have now, e.g. not exposing structure for the different parts of the module. Bottom line, type annotations in the config module is a net negative as it doesn't address anything but creates issues downstream. |
I worry this is a mischaracterization of what's going wrong. Sphinx has issues, which mean that typing, even where it's extremely beneficial, can be irksome. So I would suggest we instead prioritize a) improving how we type in Astropy and b) raising Issues / PRs in Sphinx to help them find and address our pain points.
We aren't doing either in Astropy. Now stringified type annotations is a bad idea for other reasons, but changing our practice regarding run-time-resolvable type annotations would be good. I think there are other things we can improve Astropy-side, but I'm not entirely certain what they are, only that I don't encounter these problems in other projects that use Sphinx, but not Astropy's documentation infrastructure stack. We should fix (/request fixes for) broken things. Edit: thanks @eerovaher for a rapid response in #18191. Hopefully this fully resolves the issue and/or points the way to the next thing that needs fixing. |
|
This is not mischaracterization but the downstream experience and at this point I don't really care if you can blame sphinx or something else instead of the typing work in astropy. Some random work in astropy forced us downstream to add workarounds for things that worked for years prior. And this is not the first occurrence, and as I see there is no inclination for avoiding it in the future either but pushing the blame onto other parts of the infrastructure. |
Yeah, it totally sucks when something breaks. I think this a good example of a difference in paradigms: I think when the language changes then what we consider "working" changes as well — e.g. the upcoming free-threading, ordered-dicts, etc. Typing is a core, multi-PEP part of Python. While optional, if something that's statically describable (not everything is in such a dynamic language) has issues being statically described, then something's not working. In this case it's not the annotations directly, but the tooling like Sphinx to generate documentation from them. We can and should fix things so that downstream projects have a minimum of problems. But it's important to understand what is actually breaking and also that not adapting to new features is a sure path to obsolescence. |
|
Thanks for explaining how a language evolves, it was absolutely uncalled for though. And no matter how many PEPs it went though, it's still optional, even you admit that. So, let's keep it optional and stop generating uncalled work for downstream. You know, I just wish I see half the enthusiasm for fixing the hundreds of bugs and enhancement issues, most of which are challenging as addressing those would move fwd the package and improve user experience way way more than this constant stream of typing PRs that keep breaking downstream. And you know, a "we are sorry that we merged something breaking, next time we will be more careful" would have done a lot to sweeten this up, but apparently pointing fingers to the tooling, to downstream, or to explain the new Python features is your preferred way to go about this. Thank you so much! |
|
A polite referral to https://www.astropy.org/code_of_conduct.html |
|
Please both assume good intent! It is important to think through how to avoid similar issues in the future. If my memory serves me right, there were problems uncovered with sphinx and typing, and it seems fair to think that if such problems occur when using new language features, we do not try to work around them in astropy, but rather fix things upstream before using them. |
|
I agree with @mhvk . astropy core library, being widely used as it is, "go fast and break things" should not be the first option even if typing is all the rage now. We must learn from this and proceed with caution. Thanks, all! |
|
FYI, also bit specutils |
Description
astropy/config/paths.pywas already partially annotated, but I've improved the annotations so thatnow succeeds. I also added some annotations to
astropy/config/configuration.py. The annotations there are not good enough formypy --strictand the coverage is not complete, both becauseastropy.extern.configobjis not annotated and because some code would have to be rewritten to be more friendly towards type-checkers. However, all the annotations that are present inconfigare self-consistent, as can be verified withhttps://www.astropy.org/team does not list who the maintainers for
configare, so I'm asking quite a few people to review in the hope that at least one of them actually does.