pantsbuild / pants

The Pants Build System
https://www.pantsbuild.org
Apache License 2.0
3.35k stars 640 forks source link

python typechecking: add support for pyright #17141

Open smelc opened 2 years ago

smelc commented 2 years ago

Would it be possible to add support for Microsoft's pyright typechecker for python files?

It is available on pypi so it installs easily and has a straightforward CLI: pyright py_files_to_check. I have looked at the implementation of the backend for mypy but it doesn't seem so straightforward that I add the support on my own :slightly_smiling_face:

Describe the solution you'd like

Ideally, solving this issue would amount to making the following backend possible:

backend_packages = [
  "pants.backend.python.typecheck.pyright",
]

Describe alternatives you've considered

Using mypy, but it's not really an option since our existing monorepo uses pyright in its CI and in local development workflows.

thejcannon commented 2 years ago

So the challenge (and difference from mypy) is pyright requires npm to run, as it is written in JS (TS?).

@sureshjoshi added support for npx-based npm-based tool runs, so I suspect it's feasible. Just lacking some good ol' fashioned elbow grease :smile:

benjyw commented 2 years ago

The PyPI package (https://pypi.org/project/pyright/) installs npm for you if it doesn't detect it. But I'm hesitant to use that since we already know how to do that via the nascent javascript backend at src/python/pants/backend/javascript/subsystems/nodejs.py

thejcannon commented 2 years ago

Yeah that really scares me, lol. Caching be damned if we're not careful :sweat_smile:

benjyw commented 2 years ago

I'm not familiar enough with pyright to say how hard or easy this would be. The closer it is to being a drop-in replacement for mypy, the easier it might be. I'll do a little timeboxed reading.

sureshjoshi commented 2 years ago

I ran npx pyright on my machine, and it worked - which is fundamentally how we're running prettier - with a bunch of the Pants stuff around it.

sureshjoshi commented 2 years ago

I can take a 30 minute crack at it tonight, which should give us a start - either for someone to take over, or "oh man, this is impossible"

smelc commented 2 years ago

Wow, blown-away by the responsiveness! Happy to be the first beta tester :+1:

benjyw commented 2 years ago

@sureshjoshi thanks!

From my 10 minutes of reading, I think this might be straightforward to do naively, but if you have complex pyright config, with multiple execution environments, things might get hairy:

""" Pyright allows multiple “execution environments” to be defined for different portions of your source tree. For example, a subtree may be designed to run with different import search paths or a different version of the python interpreter than the rest of the source base. """

This config really seems like it can't be dragged into a sandbox and expected to work as-is.

For example, we might have to override the venvPath setting.

@smelc how complicated is your pyright config? Can you post a (possibly redacted) version of it here?

benjyw commented 2 years ago

Hmm, on closer look, most of the suspect configs, like root, extraPaths, stubPath, typeshedPath are intended to be relpaths from the config file location, so as long as we don't strip source roots in the sandbox (and there is no reason to), it should all work as long as we bring all the right things into the sandbox (which will require parsing the config file, I guess).

Unlike mypy, it looks like pyright will partition internally, based on the config environments. So we can get away with a single invocation, if we set up venvPath and venv config keys appropriately. If they are already set, we either error or warn and override them I guess?

Alternatively, we can partition by resolve ourselves, and have the sandbox "default Python interpreter" be the VenvPex for the resolve.

Anyway, lots of nuance, but getting something simple going, if the repo has a single resolve, seems straightforward. So I'd say start there?

benjyw commented 2 years ago

@sureshjoshi I can take a look at anything you have on Sunday.

smelc commented 2 years ago

@smelc how complicated is your pyright config? Can you post a (possibly redacted) version of it here?

It is really simple. We solely declare [tool.pyright] in our pyproject.toml files and use pyright $(git ls-files "*.py") to run pyright in the CI [0]. We don't plan to use pyright's execution environments, because pyright is only one of the tools we run in the CI, and so the CI is responsible for setting up the environment (python version, installing dependencies), before handing over to pyright in a single GitHub pipeline step.

Also we don't use stubs, in our experience so far, pyright's inference is good enough so that we didn't need to specify stubs.

[0] So we don't need pyright exclude or ignore configuration to be honored, we do it ourselves in the pipeline.

sureshjoshi commented 2 years ago

High level concept works - need to pick up configs and ignores correctly, but running the tool is a non-issue. Took under 5 minutes to get it in place.

11:55:34.61 [ERROR] 1 Exception encountered:

  ProcessExecutionFailure: Process 'Run Pyright on 8 files.' failed with exit code 1.
stdout:
No configuration file found.
No pyproject.toml file found.
Assuming Python platform Darwin
Searching for source files
Found 8 source files
pyright 1.1.274
/private/var/folders/18/q1r7phps28nc9rx5j_0t3jmm0000gp/T/pants-sandbox-G31wI5/helloworld/greet/greeting.py
  /private/var/folders/18/q1r7phps28nc9rx5j_0t3jmm0000gp/T/pants-sandbox-G31wI5/helloworld/greet/greeting.py:9:8 - warning: Import "pkg_resources" could not be resolved from source (reportMissingModuleSource)
/private/var/folders/18/q1r7phps28nc9rx5j_0t3jmm0000gp/T/pants-sandbox-G31wI5/helloworld/main.py
  /private/var/folders/18/q1r7phps28nc9rx5j_0t3jmm0000gp/T/pants-sandbox-G31wI5/helloworld/main.py:4:6 - error: Import "colors" could not be resolved (reportMissingImports)
/private/var/folders/18/q1r7phps28nc9rx5j_0t3jmm0000gp/T/pants-sandbox-G31wI5/helloworld/translator/translator_test.py
  /private/var/folders/18/q1r7phps28nc9rx5j_0t3jmm0000gp/T/pants-sandbox-G31wI5/helloworld/translator/translator_test.py:4:8 - error: Import "pytest" could not be resolved (reportMissingImports)
2 errors, 1 warning, 0 informations 
benjyw commented 2 years ago

OK, nice! So that should be very straightforward to support.

benjyw commented 2 years ago

@sureshjoshi this is using your js backend npm plumbing?

sureshjoshi commented 2 years ago

this is using your js backend npm plumbing?

Yup - copied/pasted the Prettier plugin basically - so I pretended this was a lint goal, just as a proof-of-concept.

Edit: npx plumbing - which still needs some caching work before I would recommend we rely on it. I wrote this before I knew anything about the Pants cache world