Pyright: Use LSP's standard server-initiated progress scheme

Created on 11 Dec 2020  路  8Comments  路  Source: microsoft/pyright

Is your feature request related to a problem? Please describe.
pyright may analyze a bunch of files when editing a file that's imported by a lot of other files. It currently uses three non-standard LSP notifications for this whose methods are named pyright/beginProgress, pyright/reportProgress, pyright/endProgress. Although there is probably some client glue code written for this in the VSCode client (indeed even for our ST client)

Describe the solution you'd like
It's probably better for the LSP ecosystem in the long run if this custom scheme were instead replaced by the standard server initiated progress scheme of the LSP spec.

  • pyright/beginProgress --> window/workDoneProgress/create + $/progress with kind begin
  • pyright/reportProgress --> $/progress with kind report
  • pyright/endProgress --> $/progress with kind end
addressed in next version

Most helpful comment

Yes, I definitely agree, I just know I can't test and fix it at the end of day on a Friday. 馃檪

All 8 comments

The main point of contention will be where VS Code puts its version of progress reports and what they look like as compared to the current UI we have for Python (which predates progress reporting in the LSP AFAIK, and may work differently). We do use progress reporting for long-running tasks, and my impression was that in VSC they were popups rather than a status bar (with the ability to cancel the work). I don't think we want popups for non-user-initiated progress reports like "the analysis is active".

The APIs are also different in that you need to create a specific progress report and reference it, rather than just marking the start/middle/end, but that's not hard to do code wise. It does have implications for some automated benchmarks we run which use these progress notifications to know when to move onto the next part of the benchmark.

Thanks for the suggestion. I didn't realize there was a standard protocol for server-initiated progress reporting. Perhaps it was added since I first implemented this in pyright. In any case, I think this is a good suggestion and a nice simplification.

@jakebailey, here's a PR for your review: https://github.com/microsoft/pyright/pull/1269

I don't see any tests that rely on the old implementation. They all pass after my change.

I confirmed that visual behavior in VS Code is the same as before. No visible difference.

There are no tests for this because we have no tests for the language server itself outside manual testing and benchmarking. This will definitely break the benchmarker, which relies on the current notification mechanism to know when it should move through its test suite. I'd prefer not to merge this until we have the time to adapt it for the benchmarker or add some option to enable the old behavior, otherwise the benchmarker will fail until it's fixed and we'll lose perf data from any changes after it.

which predates progress reporting in the LSP AFAIK, and may work differently

Yes, it's a relatively new set of requests and notifications, since 3.15 I believe.

OK, I'm fine putting this on hold. I'm in no hurry to merge this. I do think it's a worthwhile change though, so I'd like to incorporate it eventually.

Yes, I definitely agree, I just know I can't test and fix it at the end of day on a Friday. 馃檪

Alright, I've merged a change that will use the window progress if the client has declared support for it in its capabilities. Otherwise, it'll use the old custom notifications. This'll be in the next release.

This is now addressed in Pyright 1.1.96, which I just published.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

tiangolo picture tiangolo  路  3Comments

tamuhey picture tamuhey  路  3Comments

giyyapan picture giyyapan  路  3Comments

giyyapan picture giyyapan  路  3Comments

christopher-hesse picture christopher-hesse  路  3Comments