Repository navigation
TYP: set up typechecking for units with basedpyright - #17918
neutrinoceros wants to merge 11 commits into
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.
|
|
👋 Thank you for your draft pull request! Do you know that you can use |
neutrinoceros
changed the title
units\ with \basedpyright\units with basedpyright
|
Ok, CI appears to fail as expected (good) with a whooping 27706 errors. Now the one issue is to fix tox's behavior. |
Also limit it to a set of astropy/units/format files that actually are types correctly (for "basic" mode).
|
OK, I found a fix - pushing up shortly |
| { | ||
| "typeCheckingMode": "basic", | ||
| "include": [ | ||
| "astropy/units/format" |
There was a problem hiding this comment.
I thought we'd give it a set of files that actually work for this first test.
| changedir = {tox_root} # restore default | ||
| dependency_groups = typecheck | ||
| commands = | ||
| basedpyright --stats |
There was a problem hiding this comment.
Added stats here so one gets at least an idea of how many files are actually checked...
| @@ -0,0 +1,17 @@ | |||
| { | |||
| "typeCheckingMode": "basic", | |||
There was a problem hiding this comment.
I guess a worry with this is that I think we may want different modes for different parts of astropy...
There was a problem hiding this comment.
We could have more than one configuration file if it comes to that, but it's a long shot.
|
Hmm, weird, things passed locally before I pushed, but I cannot seem to reproduce that any more. My general worry about the configuration is that it seems very hard to do it piece-by-piece -- and in particular to move towards better typing incrementally at different speeds/modes in different parts. |
| ], | ||
| "executionEnvironments": [ | ||
| { | ||
| "root": "astropy/units", |
There was a problem hiding this comment.
the idea here was to explicitly create subpackage-level settings. I'm not sure why you changed it @mhvk, can you explain ?
There was a problem hiding this comment.
I thought I just got errors if I left it in-place, but will admit I was slightly randomly changing things...
Anyway, I just checked and that is actually not a problem, so fine to revert that change! The one question then is how to do parts of a package, since the include and exclude seem to be astropy wide rather than per execution environment...
There was a problem hiding this comment.
I think for now we'll have to share include and exclude globally. If it becomes a problem, we might consider having specialized configuration file per-subpackage, but setting this up will be more involved so it's probably best to not do it until it's actually needed ?
|
thanks ! I also cannot seem to reproduce the 14 remaining errors locally, but it's not the only difference I'm seeing: while locally, I have |
|
About the number of errors: weird, indeed, since I now got 0 errors again too. ... experimenting ... It seems to depend on whether one uses Note that if I remove the Given that without |
|
Nice catch, I didn't think of playing with editable installs at all.
If I'm reading |
|
turning off |
|
I fixed one of these 4 of 5 errors. Here's the remaining one: the problem is, I can't find where the |
| def to_string( | ||
| cls, unit: UnitBase, fraction: bool | Literal["inline", "multiline"] = False | ||
| cls, | ||
| unit: CompositeUnit | NamedUnit, |
There was a problem hiding this comment.
This parameter makes the signature inconsistent with Base.to_string().
There was a problem hiding this comment.
Much more importantly, the updated signature is inconsistent with UnitBase.to_string(). So this change is not solving a type error, it's just pushing the error to astropy.units.core, but you can't tell that because you are configuring the type checker to ignore that module for now.
There was a problem hiding this comment.
Hum, this seems like a strong argument to re-expand the initial target to the whole units subpackage. What do you think, @mhvk ?
There was a problem hiding this comment.
Indeed, perhaps we should go back to astropy.units as a whole. Though that beats my goal here of making it small enough that we can actually get some CI going in a short timeframe. (I quickly checked whether something else might work, but as of now, not even astropy/units/utils.py is OK...)
There was a problem hiding this comment.
How about starting with stats then (#16562) ?
There was a problem hiding this comment.
Yes, for settting it up, anything that passes is good enough.
There was a problem hiding this comment.
Running mypy --follow-imports silent astropy/stats/ reports 471 errors. The exact number might be different if I'd install scipy-stubs, but even then I wouldn't get my hopes up. My understanding is that in stats type information from docstrings was rewritten as type annotations, but there were no attempts to ensure that the resulting annotations were correct, or even self-consistent, or even valid.
Running mypy --follow-imports silent astropy/config/*.py doesn't report any errors, but not everything in config is annotated and Mypy skips unannotated functions, so basedpyright might report many errors even in config.
| ) -> str: | ||
| # Remove units that aren't known to the format | ||
| unit = cls._decompose_to_known_units(unit) | ||
| base_unit = cls._decompose_to_known_units(unit) |
There was a problem hiding this comment.
Why do you have to rename this?
There was a problem hiding this comment.
The error that is solved this way is:
/data/mhvk/packages/astropy/astropy/units/format/cds.py:281:46 - error: Argument of type "UnitBase" cannot be assigned to parameter "unit" of type "CompositeUnit | NamedUnit" in function "_decompose_to_known_units"
Type "UnitBase" is not assignable to type "CompositeUnit | NamedUnit"
"UnitBase" is not assignable to "CompositeUnit"
"UnitBase" is not assignable to "NamedUnit" (reportArgumentType)
/data/mhvk/packages/astropy/astropy/units/format/fits.py
I just tried and this can be solved also by changing
diff --git a/astropy/units/format/base.py b/astropy/units/format/base.py
index 648cc630be..82e6dd9441 100644
--- a/astropy/units/format/base.py
+++ b/astropy/units/format/base.py
@@ -255,7 +255,7 @@ class _ParsingFormatMixin:
return did_you_mean(unit, cls._units, fix=cls._fix_deprecated)
@classmethod
- def _validate_unit(cls, unit: str, detailed_exception: bool = True) -> UnitBase:
+ def _validate_unit(cls, unit: str, detailed_exception: bool = True) -> CompositeUnit | NamedUnit:
try:
return cls._units[unit]
except KeyError:
@@ -271,7 +271,7 @@ class _ParsingFormatMixin:
)
@classmethod
- def _decompose_to_known_units(cls, unit: CompositeUnit | NamedUnit) -> UnitBase:
+ def _decompose_to_known_units(cls, unit: CompositeUnit | NamedUnit) -> CompositeUnit | NamedUnit:
"""
Partially decomposes a unit so it is only composed of units that
are "known" to a given format.
This probably is better.
It does make one wonder if we should have an alias for CompositeUnit | NamedUnit as basically being the only two subclasses of UnitBase that will end up existing... Or, perhaps conversely, loosen the types elsewhere.
There was a problem hiding this comment.
More to the point here, I personally prefer for this PR to make, as far as possible, only changes to the typing, not to the code.
There was a problem hiding this comment.
Given #17918 (comment), perhaps the safer thing to do here is to broaden the typing for now.
There was a problem hiding this comment.
More to the point here, I personally prefer for this PR to make, as far as possible, only changes to the typing, not to the code.
me too. I'm treating the draft stage of the PR as a sandbox where all changes are allowed, but plan to actually split them out and have them merged before this one.
There was a problem hiding this comment.
This probably is better.
I would argue that we should avoid type unions in return annotations as much as possible, as it usually moves the difficulty to call sites.
It does make one wonder if we should have an alias for
CompositeUnit|NamedUnitas basically being the only two subclasses ofUnitBasethat will end up existing..
This would make sense indeed, but maybe a protocol would be better. What's the shared interface between these two that is not part of UnitBase ?
There was a problem hiding this comment.
Actually, looking more closely, I admit being confused why we did the NamedUnit | CompositeUnit combo: basically, UnitBase explicitly states it is meant to be an abstract base class, though it doesn't inherit from that (no surprise, didn't exist when this code was written!). NamedUnit is, effectively, also an abstract base class, for all units that have a name. But it doesn't define some of the attributes required to have a string representation, those are defined in its non-abstract subclasses, IrreducibleUnit, UnrecognizedUnit, Unit itself, and PrefixUnit. And CompositeUnit is for composites without a name.
It may make most sense to broaden the type back to UnitBase, at least just to get going.
| # Can't use np.log10 here, because p[0] may be a Python long. | ||
| if math.log10(unit.scale) % 1.0 != 0.0: | ||
| if ( | ||
| not isinstance(base_unit.scale, float) |
There was a problem hiding this comment.
Have you checked if the OGIP format even allows complex scales?
There was a problem hiding this comment.
I think for this PR perhaps just ensuring that a warning is raised if the scale is not float would be appropriate. It is a good question, though, whether it shouldn't be an error...
There was a problem hiding this comment.
I'm taking the existing warning at face value here: if the scale needs to be a power of 10, it cannot be complex.
That's sad! It is so darn slow to work via a non-editable install... |
|
@neutrinoceros - I can get rid of the last problem with a clarification of |
|
The problem is that In order to type check |
|
@eerovaher - that is a sad statement, but also fine - happy to focus more on the code than on its annotations. I would note, though, that from a user perspective, I don't think there are that many backward compatibility issues. In the end, at least in the rest of astropy, almost all use of units is indirect, through |
|
Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then. In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary. If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock. If you believe I commented on this pull request incorrectly, please report this here. |
|
for what it's worth I'm not convinced this PR is worth keeping around; |
|
I'm going to close this pull request as per my previous message. If you think what is being added/fixed here is still important, please remember to open an issue to keep track of it. Thanks! If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here. |
Description
Close #16312
I propose using
basedpyrightas our type checker from the get go since it's been blessed by numpy devs, contrary to mypy.Here I'm just laying out a fundation to build upon: a basic configuration with CI, outputing a reduced and hopefully manageable number of errors. The intention isn't to address said errors here but to provide a shared baseline for what to tackle next. This PR shouldn't be merged until we converge to a state where no errors are reported (either because they are fixed, or explicitly ignored, albeit temporarily).
notes
pyproject.toml, although I note that in this instance this means no comments can be used (not a feature in.json)basictype checking mode (matchingpyright's default) instead ofrecommended(basedpyight's default) because the number of errors and warnings is already in the thousands. Usingrecommendedwould add about 200 errors and 20k warnings...astropy.units):5752 errors, 55 warnings, 0 notes4401 errors, 60 warnings, 0 notesreportAttributeAccessIssue) makes the baseline much more manageable (less than 200 errors)I haven't managed to properly configure tox so far; at the time of opening it seems that pyright's config file is completely ignored when run from tox. As a results, the number of errors reported is 2 orders of magnitudes beyond what I just called "manageable". I'm still opening as a draft to see if CI is properly set up.