Fix mypy support #337

l0b0 · 2021-05-09T07:08:08Z

Related Issue(s): #332

Description: Fixes most typing issues which mypy complain about in current main branch. I'm unsure about the remaining issue:

$ mypy pystac/
pystac/extensions/base.py:29: error: Incompatible default for argument "typ" (default has type "object", argument has type "Type[P]")  [assignment]
pystac/extensions/base.py:30: error: Variable "typ" is not valid as a type  [valid-type]
pystac/extensions/base.py:30: note: See https://mypy.readthedocs.io/en/latest/common_issues.html#variables-vs-type-aliases
Found 2 errors in 1 file (checked 37 source files)

Since types aren't enforced at runtime, and I don't believe mypy will assert the type of result at the various call sites (verified by changing return self._get_property(CLOUD_COVER_PROP, float) to return self._get_property(CLOUD_COVER_PROP, str) and seeing no change in the mypy output), the _get_property typ argument should probably be removed, unless it is enforced by Pyright. I'd like to hear your opinion on this before going forward with that change though.

PR Checklist:

Code is formatted (run scripts/format)
Tests pass (run scripts/test)
This PR maintains or improves overall codebase code coverage.
Changes are added to the CHANGELOG. See the docs for information about adding to the changelog.

codecov-commenter · 2021-05-09T07:10:19Z

Codecov Report

Merging #337 (fb1aef1) into main (016e56c) will decrease coverage by 0.01%.
The diff coverage is 73.52%.

@@            Coverage Diff             @@
##             main     #337      +/-   ##
==========================================
- Coverage   88.59%   88.57%   -0.02%     
==========================================
  Files          36       36              
  Lines        4629     4632       +3     
==========================================
+ Hits         4101     4103       +2     
- Misses        528      529       +1

Impacted Files	Coverage Δ
pystac/stac_io.py	`72.38% <0.00%> (ø)`
pystac/extensions/item_assets.py	`69.23% <16.66%> (ø)`
pystac/extensions/file.py	`82.39% <25.00%> (-1.18%)`	⬇️
pystac/collection.py	`93.35% <100.00%> (+0.34%)`	⬆️
pystac/extensions/base.py	`89.13% <100.00%> (ø)`
pystac/extensions/datacube.py	`55.87% <100.00%> (ø)`
pystac/extensions/eo.py	`92.01% <100.00%> (+0.07%)`	⬆️
pystac/extensions/hooks.py	`86.44% <100.00%> (ø)`
pystac/extensions/pointcloud.py	`80.08% <100.00%> (ø)`
pystac/extensions/projection.py	`96.11% <100.00%> (ø)`
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 016e56c...fb1aef1. Read the comment docs.

lossyrob · 2021-05-11T01:38:26Z

The typ argument is recognized by Pyright; passing in the type as an argument forces Pyright (and Pylance) to recognize it as the return type of the method, and can for instance catch errors when the type one passes into _get_property as a declared type is not the same as the return type of the property method that uses it. I'd label this as mildly useful; otherwise you have to assume that the Optional[T] returned from _get_property is Any, and there's no way to input a type restriction on that return type.

The mypy compliance changes are fine, but are mostly working around mypy not being able to infer types that shouldn't necessarily be stated (e.g. all of the ExtensionHooks additions). I was however suprised to see that Pyright missed some lack of return statements in the item_assets extension. I wonder if there's some settings to fiddle around with for Pyright to not let something like that pass.

I don't feel super strongly about the need to pass in types, and am mildly against having to satisfy mypy's lack of ability to infer types or handle passing runtime types; however seeing that pyright missed something pretty obvious it does feel like having the addition of mypy may be worth having to work around these things.

If we want to move forward, then I'll ask that you add mypy to the requirements-dev.txt and to the scripts/test script so that it becomes part of the CI testing.

duckontheweb · 2021-05-19T14:30:23Z

Based on the mypy docs for thehas-type error it seems like the need for type annotations on things like DATACUBE_EXTENSION_HOOKS might be due to an import cycle. I also couldn't find anything in the pyright docs about stopping pyright from inferring a NoReturn type, so I think if we want to continue to catch those errors we will need to use mypy. I agree that it's less than ideal to have to add those annotations where they should be inferred, but given that mypy caught some other issues that pyright missed, I think the trade-off is worth it.

As for the issue regarding the typ argument to _get_property, it looks like we can resolve the mypy errors and retain the type checking on the return value by making two changes:

Remove the default from the typ parameter (we never take advantage of this default in the code)
Remove the variable type annotation on result

- def _get_property(self, prop_name: str, typ: Type[P] = Type[Any]) -> Optional[P]:
-     result: Optional[typ] = self.properties.get(prop_name)
+ def _get_property(self, prop_name: str, typ: Type[P]) -> Optional[P]:
+     result = self.properties.get(prop_name)

I tested this by passing a str type instead of float to the _get_property call in the eo extension and mypy correctly raised a [return-value] error (and all tests continue to pass).

I would support @lossyrob 's suggestion that we move forward by adding mypy to requirements-dev.txt and the test script .

lossyrob · 2021-05-23T17:28:41Z

👍 @l0b0 do you want to add mypy to the CI and move this from RFC to ready to merge? Thanks!

l0b0 · 2021-05-23T22:27:48Z

Thank you @duckontheweb, that worked!

l0b0 · 2021-05-23T22:29:17Z

@lossyrob I tried using the non-deprecated importlib rather than imp, but ran into an issue with the "vanilla" job. Not sure what to do about that, so I've just ignored the setup.py type issue.

lossyrob · 2021-05-24T14:22:10Z

Not type checking setup.py is totally reasonable to me - I think we'll probably want to change that import in the future anyway.

lossyrob · 2021-05-24T14:24:15Z

There was a conflict with a recent PR around test_pointcloud, resolved and merged that via the GitHub UI

duckontheweb · 2021-05-24T14:49:52Z

The CI failures are due to type errors in the pointcloud tests introduced from the merge. It looks like a couple of the functions just need their return types annotated.

The successful builds on Windows are due to a problem with the CI. The CI script doesn't seem to be running at all, so it isn't returning a non-zero exit code. I'll fix that in a separate PR.

lossyrob · 2021-05-24T14:51:45Z

Yep, the merge conflict didn't account for some of the missing return types; I just added them and pushed that change so this should be ready to go soon 🤞

lossyrob

Looking good. I have one request to avoid changing library behavior, and a question around the possibility maintaining the use of lambda.

lossyrob · 2021-05-24T14:54:01Z

pystac/collection.py

-            self.intervals = [cast(List[Optional[Datetime]], intervals)]
-        else:
-            self.intervals = cast(List[List[Optional[Datetime]]], intervals)
+    def __init__(self, intervals: List[List[Optional[Datetime]]]):


This changes behavior; I'd prefer to keep this PR to the type changes. If we want to remove this nicety, which was put in place based on user requests, we can discuss in a follow up PR or issue.

lossyrob · 2021-05-24T14:55:48Z

pystac/extensions/file.py

@@ -188,10 +188,11 @@ def data_type(self) -> Optional[List[FileDataType]]:
        Returns:
            FileDataType
        """
-        return map_opt(
-            lambda x: [FileDataType(t) for t in x],


Is it the case that mypy doesn't allow lambdas? Is there a flag for this? I find this style of short lambda useful and more readable

No, it allows lambdas; it's just simpler to type a function. I'm not sure you can type lambdas, but I expect it'd be a mess.

Ah right, I think Pyright is OK with inferring types in lambdas without annotations where mypy isn't...a bit of inconvenience but worth it.

I think this may actually be an indicator that we're not adequately typing something in the second argument to map_opt. I left a comment on pystac.extensions.eo to illustrate this in more detail, but I think we may want to keep the untyped lambdas here and track down the typing issues in more detail.

I think the solution from the other comment will also solve this case and allow us to keep the untyped lambda here.

lossyrob · 2021-05-24T14:56:34Z

pystac/extensions/sar.py

@@ -59,7 +58,7 @@ class ObservationDirection(enum.Enum):


 class SarExtension(
-    Generic[T], ProjectionExtension[T], ExtensionManagementMixin[pystac.Item]
+    Generic[T], PropertiesExtension, ExtensionManagementMixin[pystac.Item]


l0b0 · 2021-05-24T23:58:40Z

How's this @lossyrob?

lossyrob · 2021-05-25T01:51:48Z

pystac/extensions/file.py

@@ -188,10 +188,11 @@ def data_type(self) -> Optional[List[FileDataType]]:
        Returns:
            FileDataType
        """
-        return map_opt(
-            lambda x: [FileDataType(t) for t in x],


Ah right, I think Pyright is OK with inferring types in lambdas without annotations where mypy isn't...a bit of inconvenience but worth it.

duckontheweb

@l0b0 Thanks so much for this PR, it's uncovered some subtle typing issues that we never would have caught otherwise.

My only requested changes are related to the change from untyped lambdas to defined functions that @lossyrob commented on, other than that this looks great and thanks for bearing with all of the changes.

duckontheweb · 2021-05-25T12:38:16Z

.gitignore

@@ -14,6 +14,7 @@ stdout*
 .idea/
 docs/_build/
 coverage.xml
+venv/


Since this environment location is specific to the developer and not the project, I think it would be best to handle these in the developer's global gititnore rather than something committed to the project.

Since we don't mention anything about this in the Contributing docs and we already had the .idea line creep in above, this shouldn't be a blocker, but if you're making changes related to my other comments then we might as well do this as well.

I'll also make a note to update our Contributing docs to make this clear.

I'll remove it, but I consider using the global .gitignore for anything is a bad idea. You never know when it will conflict with the needs of some project, at which point you need a custom !foo line in the project .gitignore or !/path/to/project/foo in the global .gitignore.

duckontheweb · 2021-05-25T13:19:47Z

pystac/extensions/eo.py

-            lambda bands: [Band(b) for b in bands],
+            construct_bands,


It looks like the reason that mypy can infer a type for the lambda in the _get_bands method above but not here is due to the way the 2nd argument to map_opt is typed.

In this case, we didn't have a type annotation on SummariesExtension for the summaries property. Adding the following type annotation in pystac.extensions.base.SummariesExtension allows us to keep an untyped lambda here:

from pystac.collection import Collection, Summaries ... class SummariesExtension: summaries: Summaries

Since it seems like mypy is actually catching cases where we haven't adequately typed things, this might be a better solution that creating a typed function.

duckontheweb · 2021-05-25T13:23:53Z

pystac/extensions/file.py

@@ -188,10 +188,11 @@ def data_type(self) -> Optional[List[FileDataType]]:
        Returns:
            FileDataType
        """
-        return map_opt(
-            lambda x: [FileDataType(t) for t in x],


I think this may actually be an indicator that we're not adequately typing something in the second argument to map_opt. I left a comment on pystac.extensions.eo to illustrate this in more detail, but I think we may want to keep the untyped lambdas here and track down the typing issues in more detail.

I think the solution from the other comment will also solve this case and allow us to keep the untyped lambda here.

Allows ignoring more specific errors as in `# type: ignore[ERROR]` rather than just `# type: ignore`.

l0b0 · 2021-05-25T21:41:25Z

I think this latest version should cover your comments, @duckontheweb. Thanks for working out how to keep the original lambda code!

duckontheweb

Looks great, thanks again for contributing this!

l0b0 marked this pull request as draft May 9, 2021 07:08

l0b0 changed the title ~~WIP: Fix mypy support~~ RFC: Fix mypy support May 9, 2021

l0b0 marked this pull request as ready for review May 10, 2021 20:50

duckontheweb added the PySTAC 1.0 label May 18, 2021

l0b0 force-pushed the fix-mypy-support branch 5 times, most recently from d40c711 to fb1aef1 Compare May 23, 2021 22:27

l0b0 changed the title ~~RFC: Fix mypy support~~ Fix mypy support May 23, 2021

lossyrob requested changes May 24, 2021

View reviewed changes

l0b0 force-pushed the fix-mypy-support branch 5 times, most recently from 2cd0a8c to 163286e Compare May 24, 2021 23:57

lossyrob approved these changes May 25, 2021

View reviewed changes

duckontheweb requested changes May 25, 2021

View reviewed changes

Sort settings

2a0e330

Show error codes in mypy output

c5575f2

Allows ignoring more specific errors as in `# type: ignore[ERROR]` rather than just `# type: ignore`.

l0b0 force-pushed the fix-mypy-support branch from 163286e to 860dacd Compare May 25, 2021 21:37

l0b0 and others added 5 commits May 26, 2021 09:39

Fix some mypy issues

acaf0e0

Use latest mypy version

72b1fb7

Fix several more type annotations

01c924a

Add missing return types to pointcloud tests

918ad5e

Check types with mypy in pipeline

d2643e7

l0b0 force-pushed the fix-mypy-support branch from 860dacd to d2643e7 Compare May 25, 2021 21:39

duckontheweb self-requested a review May 27, 2021 12:20

duckontheweb approved these changes May 27, 2021

View reviewed changes

duckontheweb merged commit 0091aa5 into stac-utils:main May 27, 2021

l0b0 deleted the fix-mypy-support branch May 27, 2021 21:06

This was referenced Jun 1, 2021

pyright setup #374

Closed

Drop pyright type checking #381

Closed

Stricter type hint linting #332

Closed

More thorough gitignore #405

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix mypy support #337

Fix mypy support #337

l0b0 commented May 9, 2021 •

edited

Loading

codecov-commenter commented May 9, 2021 •

edited

Loading

lossyrob commented May 11, 2021

duckontheweb commented May 19, 2021

lossyrob commented May 23, 2021

l0b0 commented May 23, 2021

l0b0 commented May 23, 2021

lossyrob commented May 24, 2021

lossyrob commented May 24, 2021

duckontheweb commented May 24, 2021

lossyrob commented May 24, 2021

lossyrob left a comment

lossyrob May 24, 2021

lossyrob May 24, 2021

l0b0 May 24, 2021

lossyrob May 25, 2021

duckontheweb May 25, 2021

lossyrob May 24, 2021

l0b0 commented May 24, 2021

lossyrob May 25, 2021

duckontheweb left a comment

duckontheweb May 25, 2021

l0b0 May 25, 2021

duckontheweb May 25, 2021

duckontheweb May 25, 2021

l0b0 commented May 25, 2021

duckontheweb left a comment

Fix mypy support #337

Fix mypy support #337

Conversation

l0b0 commented May 9, 2021 • edited Loading

codecov-commenter commented May 9, 2021 • edited Loading

Codecov Report

lossyrob commented May 11, 2021

duckontheweb commented May 19, 2021

lossyrob commented May 23, 2021

l0b0 commented May 23, 2021

l0b0 commented May 23, 2021

lossyrob commented May 24, 2021

lossyrob commented May 24, 2021

duckontheweb commented May 24, 2021

lossyrob commented May 24, 2021

lossyrob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l0b0 commented May 24, 2021

Choose a reason for hiding this comment

duckontheweb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l0b0 commented May 25, 2021

duckontheweb left a comment

Choose a reason for hiding this comment

l0b0 commented May 9, 2021 •

edited

Loading

codecov-commenter commented May 9, 2021 •

edited

Loading