Add signatureHelp and hover to Python autocomplete #3607

mattrunyon · 2023-03-27T22:06:53Z

Corresponds with deephaven/web-client-ui#1178

This addresses the first 3 items in #3453 except deprecation tags.

Adds signatureHelp and hover requests to the Python autocomplete server

Requires you build and force reinstall the Python server locally unless you are using pip -e flag for your local Python server.

Put the plumbing in place for autocomplete cancellation and diagnostic requests

You can either use the web-client-ui main branch dev server or change web/client-ui/Dockerfile to WEB_VERSION=0.32.1-lsp.3 locally to test the web UI.

proto/proto-backplane-grpc/src/main/proto/deephaven/proto/console.proto

server/src/main/java/io/deephaven/server/console/completer/JavaAutoCompleteObserver.java

server/src/main/java/io/deephaven/server/console/completer/PythonAutoCompleteObserver.java

web/client-api/src/main/java/io/deephaven/web/client/ide/IdeSession.java

web/client-api/src/main/java/io/deephaven/web/shared/ide/lsp/ParameterInformation.java

web/client-api/src/main/java/io/deephaven/web/shared/ide/lsp/SignatureInformation.java

Corresponds with deephaven/deephaven-core#3607 This adds signatureHelp and hover providers if the JS API supports the corresponding requests SignatureHelp is provided when you type a method and then type the `(` character. There's also probably a keybinding in monaco to trigger it, but I'm not sure what it is. Hover provided when you hover anything Needs to use [my core branch](https://github.com/mattrunyon/deephaven-core/tree/lsp-improvements) to test. After pulling that branch, will need to build/install the Python server using the following commands ``` ./gradlew :py-server:assemble :py-embedded-server:assemble pip install --force-reinstall py/server/build/wheel/deephaven_core-0.23.0-py3-none-any.whl py/embedded-server/build/wheel/deephaven_server-0.23.0-py3-none-any.whl ``` Then start using one of these commands. Groovy provided just to show it doesn't break anything in Groovy. There is no autocomplete in Python notebooks if you are using a Groovy worker. ``` ./gradlew server-jetty-app:run # Python START_OPTS="-Ddeephaven.console.type=groovy" ./gradlew server-jetty-app:run # Groovy ```

web/client-api/src/main/java/io/deephaven/web/client/ide/LspTranslate.java

web/client-api/src/main/java/io/deephaven/web/shared/ide/lsp/Hover.java

web/client-api/src/main/java/io/deephaven/web/shared/ide/lsp/MarkupContent.java

devinrsmith · 2023-03-31T16:53:16Z

Filed deephaven/web-client-ui#1191

py/server/deephaven_internal/auto_completer/_completer.py

jmao-denver

Small quibbles

jmao-denver

The Python changes LGTM

devinrsmith · 2023-03-31T17:17:02Z

I could have swore we didn't throw any huge errors when jedi is not installed, but it looks like that is not the case. This is not this PRs fault, but definitely something we should cleanup. Will create follow-up ticket. Should also plan to either close #3174 or re-do.

devinrsmith · 2023-03-31T17:19:50Z

Follow up #3640

devinrsmith

I did not get deep into the LSP side of things. My main concerns are about server/client semantics.

devinrsmith · 2023-03-31T17:35:06Z

server/src/main/java/io/deephaven/server/console/completer/JavaAutoCompleteObserver.java

+            switch (request.getRequestCase()) {
+                case GET_COMPLETION_ITEMS: {
+                    response.setCompletionItems(getCompletionItems(request.getGetCompletionItems(), exportedConsole,
+                            parser, responseObserver));
+                    break;
+                }
+                case GET_SIGNATURE_HELP: {
+                    // Not implemented. Return empty signature list
+                    response.setSignatures(GetSignatureHelpResponse.getDefaultInstance());
+                    break;
+                }
+                case GET_HOVER: {
+                    // Not implemented. Return empty hover help
+                    response.setHover(
+                            GetHoverResponse.newBuilder().setContents(MarkupContent.getDefaultInstance()).build());
+                    break;
+                }
+                case GET_DIAGNOSTIC: {
+                    // Not implemented. Return empty diagnostics
+                    response.setDiagnostic(GetPullDiagnosticResponse.getDefaultInstance());
+                    break;


It looks like the pattern here is to return success if the server knows about the type, but doesn't implement it. That said, I wonder if it's better to not set any response on the GET_SIGNATURE_HELP / GET_HOVER / GET_DIAGNOSTIC cases.

The way the implementation works now, for example, on a GET_DIAGNOSTIC request, the client needs to know that an empty kind field means it's not implemented?

message GetPullDiagnosticResponse { string kind = 1; optional string result_id = 2; repeated Diagnostic items = 3; }

Note: as the server is previously implemented, and implemented now, a newer client won't be able to talk to an older server (REQUEST_NOT_SET will throw an exception, which seems acceptable as long as the client knows how to properly handle it).

I think if we ever see the default case (in the way the code is currently structured), we should throw an illegal argument exception.

The LSP spec is to first negotiate what the client and server support. Then the client should not send requests or expect results if the server does not support the request type in that negotiation. The client can also tell the server what it supports (like markdown or plaintext only). For now we are just assuming the web IDE which supports markdown

We could instead add a NOT_SUPPORTED response type to be more explicit in these cases. In that case we probably wouldn't throw in the default and instead just return a NOT_SUPPORTED response

On the web client I'm checking if the JS API has either of the new requests before registering the provider, so the web IDE won't try to send requests unless the JS API has the method we expect.

devinrsmith · 2023-03-31T17:52:18Z

server/src/main/java/io/deephaven/server/console/completer/PythonAutoCompleteObserver.java

+                // Maintain compatibility with older clients
+                // The only autocomplete type supported before the consoleId was moved to the parent request was
+                // GetCompletionItems
+                final io.deephaven.proto.backplane.grpc.Ticket consoleId =
+                        value.hasConsoleId() ? value.getConsoleId() : value.getGetCompletionItems().getConsoleId();


Should we have a safer version of this? If console_id is not set, ensure that request type is get_completion_items? (Could extract code and use on the java side too)

devinrsmith · 2023-03-31T17:52:59Z

server/src/main/java/io/deephaven/server/console/completer/PythonAutoCompleteObserver.java

-                            .append("\tbuild_response_time=").append(toMillis(totalResponseBuildNanos))
-                            .append("\ttotal_complete_time=").append(toMillis(totalCompletionNanos))
-                            .endl();
+            switch (request.getRequestCase()) {


I think if we ever see the default case (in the way the code is currently structured), we should throw an illegal argument exception.

devinrsmith · 2023-03-31T17:54:43Z

proto/proto-backplane-grpc/src/main/proto/deephaven/proto/console.proto

    }
 }

 message BrowserNextResponse {
 }

 message OpenDocumentRequest {
-    io.deephaven.proto.backplane.grpc.Ticket console_id = 1;
+    io.deephaven.proto.backplane.grpc.Ticket console_id = 1 [deprecated=true];


Should we have a server safety layer that ensures if the deprecated fields and new fields are set, they are equal; otherwise blowup? Alternatively, we could say either the new fields must be set, or the deprecated fields must be set, but not both.

I think the only reason for deprecation here (as opposed to just ripping out these fields) is if a user had an older web client but newer engine version. Realistically I think the only place that may happen right now is with web developers, so I would be fine just removing these fields instead of deprecating, but I know @niloc132 had other thoughts on that

Although one main reason to not rip this out would be in DnD where at the moment we might have the enterprise JS API talking to the community engine still

mattrunyon and others added 10 commits March 14, 2023 09:37

Update proto file for more LSP requests

31cb6a4

Adjust protobuf to not be breaking

8a828cc

Promote more fields to top AutoCompleteRequest type

13ed808

Generated gwt bindings

edaa7ba

Add cancel, diagnostic, hover to proto

6bdfa2b

regenerated gwt bindings

73f0c3b

Proto changes for cancel token and diagnostics

cd06bc4

Regenerate gwt bindings

405e3fe

Generate AuthWiring for CancelAutoComplete

af65bda

Add signature help and hover support to python

b715e02

mattrunyon added py-autocomplete NoDocumentationNeeded enhancement labels Mar 27, 2023

mattrunyon requested review from niloc132, JamesXNelson and devinrsmith March 27, 2023 22:06

mattrunyon self-assigned this Mar 27, 2023

mattrunyon requested review from chipkent, jmao-denver and rcaudy as code owners March 27, 2023 22:06

mattrunyon mentioned this pull request Mar 27, 2023

feat: Add signatureHelp and hover providers to monaco deephaven/web-client-ui#1178

Merged

Spotless

ad47e89

niloc132 reviewed Mar 28, 2023

View reviewed changes

nbauernfeind and others added 4 commits March 28, 2023 14:42

Update CPP Protobuf

c0fb064

Update Python Protobuf

671fb45

Update Go Protobuf

84a57c4

Address review comments and pull in Nate proto generations

2729e20

mattrunyon added the ReleaseNotesNeeded Release notes are needed label Mar 28, 2023

Revert to throwable in python observer

7478368

mattrunyon requested a review from niloc132 March 28, 2023 22:55

Update Go Protobuf per Comment Change

5ab3b2e

niloc132 reviewed Mar 30, 2023

View reviewed changes

Address review comment and filter path completions

0b7ac8c

mattrunyon requested a review from niloc132 March 31, 2023 16:34

niloc132 previously approved these changes Mar 31, 2023

View reviewed changes

jmao-denver reviewed Mar 31, 2023

View reviewed changes

py/server/deephaven_internal/auto_completer/_completer.py Outdated Show resolved Hide resolved

jmao-denver reviewed Mar 31, 2023

View reviewed changes

py/server/deephaven_internal/auto_completer/_completer.py Outdated Show resolved Hide resolved

jmao-denver requested changes Mar 31, 2023

View reviewed changes

Address Jinafeng comments

f24449d

mattrunyon dismissed niloc132’s stale review via f24449d March 31, 2023 17:11

mattrunyon requested a review from jmao-denver March 31, 2023 17:11

jmao-denver approved these changes Mar 31, 2023

View reviewed changes

devinrsmith reviewed Mar 31, 2023

View reviewed changes

mattrunyon mentioned this pull request Mar 31, 2023

Make unsupported LSP responses better #3645

Open

devinrsmith merged commit 05fcdad into deephaven:main Mar 31, 2023

github-actions bot locked and limited conversation to collaborators Mar 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add signatureHelp and hover to Python autocomplete #3607

Add signatureHelp and hover to Python autocomplete #3607

mattrunyon commented Mar 27, 2023 •

edited

Loading

devinrsmith commented Mar 31, 2023

jmao-denver left a comment

jmao-denver left a comment

devinrsmith commented Mar 31, 2023

devinrsmith commented Mar 31, 2023

devinrsmith left a comment

devinrsmith Mar 31, 2023

devinrsmith Mar 31, 2023

mattrunyon Mar 31, 2023

devinrsmith Mar 31, 2023

devinrsmith Mar 31, 2023

devinrsmith Mar 31, 2023

mattrunyon Mar 31, 2023 •

edited

Loading

Add signatureHelp and hover to Python autocomplete #3607

Add signatureHelp and hover to Python autocomplete #3607

Conversation

mattrunyon commented Mar 27, 2023 • edited Loading

devinrsmith commented Mar 31, 2023

jmao-denver left a comment

Choose a reason for hiding this comment

jmao-denver left a comment

Choose a reason for hiding this comment

devinrsmith commented Mar 31, 2023

devinrsmith commented Mar 31, 2023

devinrsmith left a comment

Choose a reason for hiding this comment

devinrsmith Mar 31, 2023

Choose a reason for hiding this comment

devinrsmith Mar 31, 2023

Choose a reason for hiding this comment

mattrunyon Mar 31, 2023

Choose a reason for hiding this comment

devinrsmith Mar 31, 2023

Choose a reason for hiding this comment

devinrsmith Mar 31, 2023

Choose a reason for hiding this comment

devinrsmith Mar 31, 2023

Choose a reason for hiding this comment

mattrunyon Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

mattrunyon commented Mar 27, 2023 •

edited

Loading

mattrunyon Mar 31, 2023 •

edited

Loading