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

Allow specifying Pyright #5207

Open
felixm-stripe opened this issue Dec 4, 2023 · 20 comments
Open

Allow specifying Pyright #5207

felixm-stripe opened this issue Dec 4, 2023 · 20 comments
Assignees
Labels
enhancement New feature or request

Comments

@felixm-stripe
Copy link

felixm-stripe commented Dec 4, 2023

Currently, it is hard to instrument Pyright, and upgrading it at a different cadence than Pylance is impossible.

At Stripe, we run certain aspects of our CI infrastructure through Pyright and we want that to as closely as possible match the experience our developers have when using VSCode with Python.

It would be immensely useful for us to be able to provide a wrapper around Pyright that adds telemtry and custom context. Today, it is not possible, since Pylance dictates how Pyright is invoked without allowing its customization.

If we had something like:

{
    "python.pyright.entry": "/path/to/my/pyright.js"
}

we'd be able to instrument Pyright, and also potentially move at a different release cadence than Pylance.

@github-actions github-actions bot added the needs repro Issue has not been reproduced yet label Dec 4, 2023
@rchiodo
Copy link
Contributor

rchiodo commented Dec 4, 2023

Pyright isn't really consumed this way. We don't just run Pyright inside of Pylance. The only solution for now is to pin your copy of Pylance to match the Pyright you use in CI.

@rchiodo rchiodo added enhancement New feature or request and removed needs repro Issue has not been reproduced yet labels Dec 4, 2023
@rchiodo
Copy link
Contributor

rchiodo commented Dec 4, 2023

@felixm-stripe, just to be clear, you want to be able to have the latest Pylance, but decide that it uses an older version of Pyright?

What's the reason for needing an older Pyright?

@rchiodo
Copy link
Contributor

rchiodo commented Dec 4, 2023

If you wanted to use a different instrumented version of Pyright for CI, are you saying you want this inside of Pylance as well?

Perhaps there's a separate feature there that we could add to both. What's the instrumentation used for?

@Avasam
Copy link

Avasam commented Dec 4, 2023

Using an older pyright version than bundled with pylance is useful when you can't immediately fix new diagnostics (whether true or false positives)

Using a newer version of pyright than bundled with pylance is useful because pylance lags behind and you end up with a desynced Editor vs CLI. Using pre-release versions of pylance mitigates that a bit, but comes with its own stability issues (like accidental breakage, more update notifications, etc.)

@felixm-stripe
Copy link
Author

Thanks for getting back to us so quickly ☺️

Primarily, we're interested in instrumenting the developer experience. How fast are certain files typed, symbol resolution times etc. In general we want to be able to answer the question of where should we improve the developer experience when it comes to writing code. We'd like to see which users hit the most issues and which files they are editing. We also want to be able to inject more context for certain LSP queries etc

For TypeScript, we already do this by wrapping the language server, we could probably write our own language server using Pyright as some form of backend, but like you're alluding to -- Pylance probably does more here than just consume Pyright.

@erictraut
Copy link
Contributor

Yes, Pylance does more than just consume pyright. It effectively "forks" the pyright code base and modifies the behavior of pyright by overriding internal classes, calling internal interfaces, installing hooks, etc. It would be a massive undertaking to separate the Pylance functionality from pyright and allow the two to be versioned independently. It would also slow down the development of both pyright and pylance because we'd need to develop and maintain a formal and stable interface between the two. This is unlikely to happen any time soon, if ever. For that reason, we should brainstorm alternative solutions.

@felixm-stripe, is this instrumentation functionality focused on internal stripe development (i.e. devs who work for stripe), or are you looking to get instrumentation data for consumers of your SDKs and libraries? If it's the latter, how would you propose to get this modified version of pyright/pylance onto these developers' machines? Wouldn't any functionality of this type need to be incorporated into the standard distribution of pylance? Or were you thinking that you could instruct these devs to manually download and install some extra components?

@felixm-stripe
Copy link
Author

It's the former ☺️

@felixm-stripe
Copy link
Author

I assume Pylance speaks LSP with VSCode -- could we instrument at that level instead?

@rchiodo
Copy link
Contributor

rchiodo commented Dec 4, 2023

I assume Pylance speaks LSP with VSCode -- could we instrument at that level instead?

It would certainly be easier but not with our current architecture. There's no way for you to inject middleware into our client code.

I wonder if VS code has something for this already though.

@DetachHead
Copy link

since pylance seems to be too tightly coupled with its own version of pyright, a solution i tried was to add an importStrategy option to the pyright extension instead (microsoft/pyright#6644) to allow it to use the version of pyright installed in the project, which make it consistent with the same setting present in the microsoft's other python extensions (mypy, flake8, black, etc.)

if i understand correctly your use case is using a modified version of pyright, but i think the far more common use case is to pin the version of pyright used in vscode to the one used in the CI, eg. to stop extension auto-updates from introducing new errors that aren't present in the ci. i think this is a pretty important feature which is keeping me from switching from mypy to pyright

@rchiodo
Copy link
Contributor

rchiodo commented Dec 5, 2023

Idea to solve this problem:

  • Pylance disables diagnostics
  • Pylance forwards all requests to an internal Pyright server and has its diagnostics come back
  • User specifies Pyright version in settings.json (or maybe even pyproject.toml/requirements.txt)

Issue with that solution:

  • Hover text wouldn't match error text - but maybe this is okay

@heejaechang
Copy link
Contributor

also, we might let users to use different type checker such as this - https://github.com/microsoft/vscode-mypy with pylance LS.

but it also means some type tooltip pylance shows might not agree with what type checker says such as hover tooltip or completion and etc.

@felixm-stripe
Copy link
Author

Hover text wouldn't match error text - but maybe this is okay

Is this because you get hover text not from Pyright? Personally, I'd be OK with this if they're not 1:1 but still carry the same meaning. Is that what you're getting at?

@erictraut
Copy link
Contributor

Is this because you get hover text not from Pyright?

Hover text is based on type evaluations, which come from pyright. If we're instantiating one copy of pyright for Pylance's language server features and a separate copy (using a different version) for diagnostics, you could see differences.

You will also see double the memory and CPU usage for two instances of pyright.

I'm not convinced this is a good solution.

@Avasam
Copy link

Avasam commented Dec 6, 2023

You will also see double the memory and CPU usage for two instances of pyright.

I'm not convinced this is a good solution.

Good point. I wouldn't want to impose this drawback on all devs of the workspace (let them choose if their machine can handle it). And with the goal of synchronising workspace/project configurations with CLI tooling, it kindof defeats the purpose for me.

In the long term, I'd still want a way to sync the CLI/CI pyright version with Pylance.

@DetachHead
Copy link

DetachHead commented Jan 25, 2024

since this issue prevented me from switching from mypy to pyright, i made my own fork which fixes it as well as several other problems: https://github.com/DetachHead/basedpyright

by default, the basedpyright vscode extension uses the version of the basedpyright package installed in your project, using the importStrategy setting:

// .vscode/settings.json
{
    "pyright.importStrategy": "fromEnvironment"
}

this makes it consistent with how the vscode extensions for other tools work (mypy, ruff, black, etc.).

@felixm-stripe admittedly this doesn't fully address your use case though since you can't specify the path to a custom pyright LSP, but since my issue was closed (#5201) idk where else to post this.

my fork should make it easier for you to point the vscode extension to your own version by creating your own version of the pypi package with a basedpyright-langserver script in it. happy to try to help if you have any questions

@debonte
Copy link
Contributor

debonte commented Jan 25, 2024

i made my own fork which fixes it...

But this is a fork of Pyright, not Pylance, so I don't see how it could address the request to allow Pylance to run on top of a specified version of Pyright.

@DetachHead
Copy link

yeah i forgot to mention that tradeoff. but 99% of pylance's functionality comes from pyright anyway. in my experience i'm not really missing out on much by using pyright on its own instead of pylance.

@debonte
Copy link
Contributor

debonte commented Jan 26, 2024

in my experience i'm not really missing out on much by using pyright on its own instead of pylance.

Really? Semantic highlighting, indexing, IntelliCode, notebook support, folding, inlay hints, semantic auto indent, etc... Pyright is focused on type checking. Pylance adds a ton of productivity / language server features that you'd lose by switching to the Pyright VS Code extension.

Some people don't care about those things I suppose. But I wanted to provide some context for people reading this issue since this seems like a pretty big loss in functionality to me.

@DetachHead
Copy link

DetachHead commented Jan 26, 2024

i know it's not ideal to have to ditch pylance, but as i said i considered this issue significant enough to prevent me from switching from mypy to pyright/pylance, and since pylance is not open source the only way for me to solve this problem myself was to fork pyright and forget about pylance.

i agree that this problem should be solved in pylance, so this issue should remain open. i'm just baffled that the maintainers don't recognize how big of a problem this is and rejected my issue (#5201) as well as my attempt to work with them to fix it (microsoft/pyright#6644)

having your IDE use random versions of linters/type checkers is a nightmare which causes confusion for any developer working on your project. type errors should not just randomly appear when you haven't changed any code or dependencies. this is why the vscode extensions for all other tools have an importStrategy option.

type checking is very important but it seems like users end up just turning it off or ignoring the errors when using pylance, because most people use mypy in their CI and pylance locally (since mypy is way too slow and many users probably don't even know about pyright and its cli) and using two different type checkers like that just doesn't make any sense (unless you're developing a library and need it to be compatible with multiple type checkers. but in that case you'd want pyright in the CI as well)


EDIT: basedpyright now re-implements some pylance features such as suggested import code actions, semantic highlighting and inlay hints and i intend to add more in the future

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

8 participants