Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use pyright for static type checking of implemenation code #1403

Merged
merged 9 commits into from
Feb 28, 2024

Conversation

freider
Copy link
Contributor

@freider freider commented Feb 23, 2024

mypy can not and will not get functionality to be able to type check files that have both implemenation and type stubs (both .py and .pyi files). Pyright does seem to support it, so this adds it to the CI pipeline for checking implemenations.

This only adds functions.py to the whitelist of files to test, since it's quite an undertaking to fix all of the client code at once. Once we have fixes for all files we can let pyright traverse the entire repo instead of using this whitelist.

If I understand things correctly, pyright is also what's used as the python language server by vscode, so getting rid of type warnings for vscode devs is another benefit.

Initial batch of fixes for: #1372 and MOD-2387

@freider freider requested a review from mwaskom February 23, 2024 15:07
Copy link
Contributor

@mwaskom mwaskom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good example of why retroactive type annotation in Python can look so messy; I worry that some of the assert obj calls or secret defaults could end up leaking confusing failures to users. But I guess there's not much we can do?

Comment on lines +852 to +854
timeout_secs=timeout_secs or 0,
task_idle_timeout_secs=container_idle_timeout or 0,
concurrency_limit=concurrency_limit or 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC there was some discussion recently that container timeouts can't be set to 0. I don't know if that's really a problem here but seems kinda weird.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These defaults additions were previously implicit due to protobuf's serialization format, since nullity (?) isn't really a thing there unless you box your primitive types in a message type or similar. So for example task_idle_timeout_secs is currently determined to be unset by the backend when it's 0, which isn't great since it prevents us from setting an actual 0-value. According to the docs we have a minimum value of 2 seconds, which is arguably even weirder :D

@freider
Copy link
Contributor Author

freider commented Feb 26, 2024

Yeah it's not very pretty. Perhaps we can refactor the code at some point to make a few of the "connected" types like client have a separate type for their connected/initialized version and unconnected versions, and then make sure all "active" wrappers like FunctionCall only contain non-optional references to the connected versions, which would get rid of some of the assertions.

But this should at least not lead to any errors that weren't previously errors further down the call stack :)

@@ -19,7 +19,7 @@ packages = find:
python_requires = >=3.8
install_requires =
aiohttp
aiostream
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't trigger image rebuilds right @mwaskom ? Afaicr only the modal/requirements.txt file does that 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s my understanding!

@savarin
Copy link
Contributor

savarin commented Feb 27, 2024

@freider Interesting, I didn't realize pyright reviews both .py and .pyi files. I still don't have a good feel for what error it raises so I'll play around with it a bit more.

One thought - modal.functions is a bit further down the dependency tree, might be easier to start off at the top like in #1374.

@freider
Copy link
Contributor Author

freider commented Feb 28, 2024

@prbot approve

Copy link

@modal-pr-review-automation modal-pr-review-automation bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved 👍. @RohanArepally will follow-up review this.

@freider freider merged commit dabbfdc into main Feb 28, 2024
17 checks passed
@freider freider deleted the freider/pyright branch February 28, 2024 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants