Fix types, run typechecker in CI #1393

yorickvP · 2023-11-21T14:54:19Z

I fell into a typing hole while setting up pyright-lsp. Nevertheless, I think these are good changes, mostly preventing some annoying crashes (ast_openapi_scha + bytes defaults, cpus being uncountable).

Fixes #1360

technillogue · 2023-11-21T18:27:57Z

hmm https://github.com/replicate/cog/blob/main/pyproject.toml#L50-L87

yorickvP · 2023-11-21T20:32:45Z

mypy finds largely the same errors. But lsp-pyright is really good. I can optionally add a tool.pyright section with some config.

extra stuff found by mypy (mostly untyped defs):

python/cog/command/ast_openapi_schema.py:9: error: Function "unparse" could always be true in boolean context  [truthy-function]
python/cog/command/ast_openapi_schema.py:12: error: Incompatible types in assignment (expression has type "Callable[[object], str]", variable has type "Callable[[AST], str]")  [assignment]
python/cog/types.py:264: error: Function is missing a type annotation for one or more arguments  [no-untyped-def]
python/cog/types.py:268: error: Function is missing a type annotation for one or more arguments  [no-untyped-def]
python/cog/server/http.py:77: error: Unsupported dynamic base class "schema.PredictionRequest.with_types"  [misc]
Found 5 errors in 3 files (checked 23 source files)

erbridge · 2023-11-22T17:42:20Z

pyproject.toml

@@ -54,6 +55,21 @@ disallow_untyped_defs = true
 no_implicit_optional = false
 exclude = ["python/tests/"]

+[tool.pyright]


Should we pick one of pyright or mypy and drop the other?

would agree, would kinda lean towards sticking to mypy rather than making changes. pylsp-mypy exists though I assume it's slower

As a long-time mypy user, I like pyright more:

can check untyped defs

these things: https://github.com/microsoft/pyright/blob/main/docs/mypy-comparison.md

has way more options to warn/error on things we weren't checking for

Notably pyright actually passes the pydantic stuff :)

...I've had check-untyped-defs on the entire time I've used mypy. but I have to agree that this is fairly compelling even if the javascript stuff is weird.

@yorickvP Thanks for the nudge. I just gave Pyright a look, and found it to be an improvement over mypy. I'd be very happy for us to adopt it across the board. (To that end, I just opened this PR in our Python client)

python/cog/command/ast_openapi_schema.py

technillogue · 2023-11-22T19:56:15Z

python/cog/server/http.py

+    class PredictionRequest(schema.PredictionRequest.with_types(input_type=InputType)):
+        pass


tldr this allows typingchecking PredictionRequest usage by pyright (still not mypy, though)

python/cog/server/http.py

Signed-off-by: Yorick van Pelt <[email protected]>

nickstenning · 2023-11-23T16:52:42Z

Love it!

I'd like to request that if we go all in on pyright (which I fully support) then let's also get rid of all mypy references in the codebase (pyproject.toml/Makefile/etc.)

nickstenning

Modulo failing tests (I think we'll have to use "old" ways of defining union types for a while to support <3.10) this seems great, particularly the bit where we typecheck in CI!

pyproject.toml

technillogue

this PR probably shouldn't change async functionality

python/cog/server/http.py

Signed-off-by: Yorick van Pelt <[email protected]>

yorickvP requested review from nickstenning, mattt and technillogue November 21, 2023 14:54

yorickvP force-pushed the yorickvp/pyright-validate branch from 1ca8b79 to bebf2b5 Compare November 21, 2023 15:44

yorickvP force-pushed the yorickvp/pyright-validate branch from 6800f9d to a1d1130 Compare November 22, 2023 11:11

yorickvP changed the title ~~Fix bugs, make typechecker (pyright) happy~~ Fix types, run typechecker in CI Nov 22, 2023

yorickvP force-pushed the yorickvp/pyright-validate branch 2 times, most recently from 5912594 to 6af971f Compare November 22, 2023 14:23

erbridge reviewed Nov 22, 2023

View reviewed changes

technillogue reviewed Nov 22, 2023

View reviewed changes

yorickvP force-pushed the yorickvp/pyright-validate branch from 721d32f to a2971ec Compare November 23, 2023 16:15

yorickvP added 17 commits November 23, 2023 17:30

python: add trivial type casts to make typechecker happy

bf6b23c

Signed-off-by: Yorick van Pelt <[email protected]>

worker.cancel: don't crash on missing child

38313b9

Signed-off-by: Yorick van Pelt <[email protected]>

webhook_caller_filtered: expects as filter set()

109a900

Signed-off-by: Yorick van Pelt <[email protected]>

cog.server.http: don't crash when cpu_count couldn't be determined

bd0bffd

Signed-off-by: Yorick van Pelt <[email protected]>

ast_openapi_schema: deal with bytes() defaults

b96e171

Signed-off-by: Yorick van Pelt <[email protected]>

cog.server.http: change PredictionRequest to make pyright happy

e7c273f

Signed-off-by: Yorick van Pelt <[email protected]>

cog.predictor: more possible types

2bf8de9

Signed-off-by: Yorick van Pelt <[email protected]>

Fix: UnionType is new in 3.10

9946ffc

Signed-off-by: Yorick van Pelt <[email protected]>

Fix: | syntax is only available in 3.10

0b23405

Signed-off-by: Yorick van Pelt <[email protected]>

cog.types: fix untyped definition

0ba9855

Signed-off-by: Yorick van Pelt <[email protected]>

Add tool.pyright config to pyproject.toml

ab66b2a

Signed-off-by: Yorick van Pelt <[email protected]>

python: error on missing type arguments, add them everywhere

6264eaf

Signed-off-by: Yorick van Pelt <[email protected]>

ast_openapi_schema.extract_info: make type checker actually happy

8dfe4e6

Signed-off-by: Yorick van Pelt <[email protected]>

Add typecheck python github action

ddc9333

Signed-off-by: Yorick van Pelt <[email protected]>

cog.server.http: quote typenames

5dfd4f3

Signed-off-by: Yorick van Pelt <[email protected]>

python: process some review comments

38f0222

Signed-off-by: Yorick van Pelt <[email protected]>

async runner/http: fix types

2c8a65c

Signed-off-by: Yorick van Pelt <[email protected]>

yorickvP force-pushed the yorickvp/pyright-validate branch from a2971ec to 2c8a65c Compare November 23, 2023 16:50

nickstenning approved these changes Nov 23, 2023

View reviewed changes

nickstenning reviewed Nov 23, 2023

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

technillogue reviewed Nov 23, 2023

View reviewed changes

python/cog/server/http.py Outdated Show resolved Hide resolved

python/cog/server/http.py Outdated Show resolved Hide resolved

python/cog/server/http.py Outdated Show resolved Hide resolved

python/cog/server/http.py Outdated Show resolved Hide resolved

yorickvP added 4 commits November 24, 2023 11:50

python: mypy -> pyright everywhere

4c9b8d9

Signed-off-by: Yorick van Pelt <[email protected]>

pyproject: unneccesary -> unnecessary

d18c755

Signed-off-by: Yorick van Pelt <[email protected]>

resolve threading|asyncio.Event difference the other way

c8f3636

Signed-off-by: Yorick van Pelt <[email protected]>

python: make tests pass for PredictionResponse

9aca73b

Signed-off-by: Yorick van Pelt <[email protected]>

yorickvP force-pushed the yorickvp/pyright-validate branch from 982b2b8 to 9aca73b Compare November 24, 2023 12:43

Fix test in python 3.7

068f6a7

Signed-off-by: Yorick van Pelt <[email protected]>

mattt approved these changes Nov 27, 2023

View reviewed changes

technillogue approved these changes Nov 27, 2023

View reviewed changes

yorickvP merged commit 78caaa3 into main Nov 28, 2023

yorickvP deleted the yorickvp/pyright-validate branch November 28, 2023 14:13

		class PredictionRequest(schema.PredictionRequest.with_types(input_type=InputType)):
		pass

Fix types, run typechecker in CI #1393

Fix types, run typechecker in CI #1393

Uh oh!

Conversation

yorickvP commented Nov 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

technillogue commented Nov 21, 2023

Uh oh!

yorickvP commented Nov 21, 2023

Uh oh!

erbridge Nov 22, 2023

Choose a reason for hiding this comment

Uh oh!

technillogue Nov 22, 2023

Choose a reason for hiding this comment

Uh oh!

yorickvP Nov 23, 2023

Choose a reason for hiding this comment

Uh oh!

technillogue Nov 23, 2023

Choose a reason for hiding this comment

Uh oh!

mattt Nov 27, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

technillogue Nov 22, 2023

Choose a reason for hiding this comment

Uh oh!

yorickvP Nov 23, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nickstenning commented Nov 23, 2023

Uh oh!

nickstenning left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

technillogue left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yorickvP commented Nov 21, 2023 •

edited

Loading