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 LSP's standard server-initiated progress scheme #1268

Closed
rwols opened this issue Dec 11, 2020 · 8 comments
Closed

Use LSP's standard server-initiated progress scheme #1268

rwols opened this issue Dec 11, 2020 · 8 comments
Labels
addressed in next version Issue is fixed and will appear in next published version

Comments

@rwols
Copy link

rwols commented Dec 11, 2020

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
@jakebailey
Copy link
Member

jakebailey commented Dec 11, 2020

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.

@erictraut
Copy link
Collaborator

erictraut commented Dec 11, 2020

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: #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.

@jakebailey
Copy link
Member

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.

@rwols
Copy link
Author

rwols commented Dec 12, 2020

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.

@erictraut
Copy link
Collaborator

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.

@erictraut erictraut added the blocked Waiting on external fix label Dec 12, 2020
@jakebailey
Copy link
Member

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

@jakebailey
Copy link
Member

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.

@jakebailey jakebailey added addressed in next version Issue is fixed and will appear in next published version and removed blocked Waiting on external fix labels Dec 14, 2020
@erictraut
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addressed in next version Issue is fixed and will appear in next published version
Projects
None yet
Development

No branches or pull requests

3 participants