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.

Sign up for GitHub

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

Jedi cleanup and fixes #3174

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Jedi cleanup and fixes #3174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bb58ac0
Jedi cleanup and fixes
devinrsmith Dec 7, 2022
ea9ca03
Merge branch 'main' into jedi2
devinrsmith Dec 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,10 @@ public Object unwrapObject(Object object) {
return object;
}

public PythonScope<PyObject> scope() {
return scope;
}

interface PythonScriptSessionModule extends Closeable {
PyObject create_change_list(PyObject from, PyObject to);

Expand Down
4 changes: 2 additions & 2 deletions py/server/deephaven_internal/auto_completer/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@
later, we may add slow mode, which uses both static and interpreted completion modes.
"""

from ._completer import Completer, Mode

from ._completer import Completer, Mode, jedi_settings
from jedi import preload_module, Interpreter

jedi_settings = Completer()
Comment on lines -19 to -22
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just curious... why move the instance location? the object is designed such that if customer wants to turn off autocomplete, they need to access this instance to set the mode to off. Just want to make sure that is clear when you are making decisions about where things should live.

# warm jedi up a little. We could probably off-thread this.
preload_module("deephaven")
Interpreter("", []).complete(1, 0)
50 changes: 21 additions & 29 deletions py/server/deephaven_internal/auto_completer/_completer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,32 +14,29 @@ def __str__(self) -> str:
return self.value


class Completer(object):
class Settings:
def __init__(self):
self._docs = {}
self._versions = {}
# we will replace this w/ top-level globals() when we open the document
self.__scope = globals()
# might want to make this a {uri: []} instead of []
self.pending = []
try:
import jedi

self.__can_jedi = True
self.mode = Mode.STRONG
except ImportError:
self.__can_jedi = False
self.mode = Mode.OFF
self._mode = Mode.STRONG

@property
def mode(self) -> Mode:
return self.__mode
return self._mode

@mode.setter
def mode(self, mode) -> None:
if type(mode) == "str":
mode = Mode[mode]
self.__mode = mode
def mode(self, mode: Mode):
self._mode = mode


jedi_settings = Settings()


class Completer(object):
def __init__(self, scope: dict):
self._docs = {}
self._versions = {}
self.__scope = scope
# might want to make this a {uri: []} instead of []
self.pending = []

def open_doc(self, text: str, uri: str, version: int) -> None:
self._docs[uri] = text
Expand All @@ -61,18 +58,13 @@ def close_doc(self, uri: str) -> None:
for pending in self.pending:
pending.set()

def is_enabled(self) -> bool:
return self.__mode != Mode.OFF

def can_jedi(self) -> bool:
return self.__can_jedi

def set_scope(self, scope: dict) -> None:
self.__scope = scope

def do_completion(
self, uri: str, version: int, line: int, col: int
) -> list[list[Any]]:
global jedi_settings
if jedi_settings.mode == Mode.OFF:
return []

if not self._versions[uri] == version:
# if you aren't the newest completion, you get nothing, quickly
return []
Expand Down
56 changes: 30 additions & 26 deletions server/src/main/java/io/deephaven/server/console/ConsoleServiceGrpcImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,15 @@
import io.deephaven.proto.backplane.grpc.TypedTicket;
import io.deephaven.proto.backplane.script.grpc.*;
import io.deephaven.server.console.completer.JavaAutoCompleteObserver;
import io.deephaven.server.console.completer.JediCompleter;
import io.deephaven.server.console.completer.PythonAutoCompleteObserver;
import io.deephaven.server.session.SessionCloseableObserver;
import io.deephaven.server.session.SessionService;
import io.deephaven.server.session.SessionState;
import io.deephaven.server.session.SessionState.ExportBuilder;
import io.deephaven.server.session.TicketRouter;
import io.grpc.stub.StreamObserver;
import org.jpy.PyModule;
import org.jpy.PyObject;

import javax.inject.Inject;
Expand Down Expand Up @@ -109,14 +111,13 @@ public void startConsole(StartConsoleRequest request, StreamObserver<StartConsol
.onError(responseObserver)
.submit(() -> {
final ScriptSession scriptSession = new DelegatingScriptSession(scriptSessionProvider.get());

final StartConsoleResponse response = StartConsoleResponse.newBuilder()
.setResultId(request.getResultId())
.build();
safelyExecute(() -> {
responseObserver.onNext(StartConsoleResponse.newBuilder()
.setResultId(request.getResultId())
.build());
responseObserver.onNext(response);
responseObserver.onCompleted();
});

return scriptSession;
});
});
Expand Down Expand Up @@ -255,25 +256,29 @@ public StreamObserver<AutoCompleteRequest> autoCompleteStream(
if (AUTOCOMPLETE_DISABLED) {
return new NoopAutoCompleteObserver(session, responseObserver);
}
if (PythonDeephavenSession.SCRIPT_TYPE.equals(scriptSessionProvider.get().scriptType())) {
PyObject[] settings = new PyObject[1];
safelyExecute(() -> {
final ScriptSession scriptSession = scriptSessionProvider.get();
scriptSession.evaluateScript(
"from deephaven_internal.auto_completer import jedi_settings ; jedi_settings.set_scope(globals())");
settings[0] = (PyObject) scriptSession.getVariable("jedi_settings");
});
boolean canJedi = settings[0] != null && settings[0].call("can_jedi").getBooleanValue();
log.info().append(canJedi ? "Using jedi for python autocomplete"
: "No jedi dependency available in python environment; disabling autocomplete.").endl();
return canJedi ? new PythonAutoCompleteObserver(responseObserver, scriptSessionProvider, session)
: new NoopAutoCompleteObserver(session, responseObserver);
final ScriptSession scriptSession = scriptSessionProvider.get();
if (PythonDeephavenSession.SCRIPT_TYPE.equals(scriptSession.scriptType())) {
final PyObject scope = ((PythonDeephavenSession) scriptSession).scope().mainGlobals().unwrap();
// noinspection EmptyTryBlock,unused
try (final JediCompleter _unused = jedi(scope)) {
// ensure we can create it
} catch (RuntimeException e) {
log.debug(e).append("Unable to create autocomplete; is jedi installed?").endl();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will anybody ever see this?
if someone expects autocomplete but it doesn't work, I doubt they are going to start w/ debug logging, or even notice this amid the logspam..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we raise the log level so it's easily noticed, we can include a message about how to disable autocomplete altogether.

return new NoopAutoCompleteObserver(session, responseObserver);
}
log.debug().append("Using jedi for python autocomplete").endl();
return new PythonAutoCompleteObserver(responseObserver, session, () -> jedi(scope));
}

return new JavaAutoCompleteObserver(session, responseObserver);
});
}

private static JediCompleter jedi(final PyObject scope) {
try (final PyModule pyModule = PyModule.importModule("deephaven_internal.auto_completer")) {
return pyModule.call("Completer", scope).createProxy(JediCompleter.class);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a code comment here would be nice

}
}

private static class NoopAutoCompleteObserver extends SessionCloseableObserver<AutoCompleteResponse>
implements StreamObserver<AutoCompleteRequest> {
public NoopAutoCompleteObserver(SessionState session, StreamObserver<AutoCompleteResponse> responseObserver) {
Expand All @@ -284,13 +289,12 @@ public NoopAutoCompleteObserver(SessionState session, StreamObserver<AutoComplet
public void onNext(AutoCompleteRequest value) {
// This implementation only responds to autocomplete requests with "success, nothing found"
if (value.getRequestCase() == AutoCompleteRequest.RequestCase.GET_COMPLETION_ITEMS) {
safelyExecuteLocked(responseObserver, () -> {
responseObserver.onNext(AutoCompleteResponse.newBuilder()
.setCompletionItems(
GetCompletionItemsResponse.newBuilder().setSuccess(true)
.setRequestId(value.getGetCompletionItems().getRequestId()))
.build());
});
final AutoCompleteResponse response = AutoCompleteResponse.newBuilder()
.setCompletionItems(GetCompletionItemsResponse.newBuilder()
.setSuccess(true)
.setRequestId(value.getGetCompletionItems().getRequestId()))
.build();
safelyExecuteLocked(responseObserver, () -> responseObserver.onNext(response));
}
}

Expand Down
21 changes: 21 additions & 0 deletions server/src/main/java/io/deephaven/server/console/completer/JediCompleter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package io.deephaven.server.console.completer;

import org.jpy.PyObject;

import java.io.Closeable;

public interface JediCompleter extends Closeable {

void open_doc(String text, String uri, int version);

String get_doc(String uri);

void update_doc(String document, String uri, int version);

void close_doc(String uri);

PyObject do_completion(String uri, int version, int line, int character);

@Override
void close();
}
89 changes: 50 additions & 39 deletions server/src/main/java/io/deephaven/server/console/completer/PythonAutoCompleteObserver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,33 @@
import io.deephaven.io.logger.Logger;
import io.deephaven.lang.completion.ChunkerCompleter;
import io.deephaven.lang.parse.CompletionParser;
import io.deephaven.proto.backplane.script.grpc.*;
import io.deephaven.proto.backplane.script.grpc.AutoCompleteRequest;
import io.deephaven.proto.backplane.script.grpc.AutoCompleteResponse;
import io.deephaven.proto.backplane.script.grpc.ChangeDocumentRequest;
import io.deephaven.proto.backplane.script.grpc.CloseDocumentRequest;
import io.deephaven.proto.backplane.script.grpc.CompletionItem;
import io.deephaven.proto.backplane.script.grpc.DocumentRange;
import io.deephaven.proto.backplane.script.grpc.GetCompletionItemsRequest;
import io.deephaven.proto.backplane.script.grpc.GetCompletionItemsResponse;
import io.deephaven.proto.backplane.script.grpc.OpenDocumentRequest;
import io.deephaven.proto.backplane.script.grpc.Position;
import io.deephaven.proto.backplane.script.grpc.TextDocumentItem;
import io.deephaven.proto.backplane.script.grpc.TextEdit;
import io.deephaven.proto.backplane.script.grpc.VersionedTextDocumentIdentifier;
import io.deephaven.server.console.ConsoleServiceGrpcImpl;
import io.deephaven.server.session.SessionCloseableObserver;
import io.deephaven.server.session.SessionState;
import io.deephaven.util.SafeCloseable;
import io.grpc.stub.StreamObserver;
import org.jpy.PyObject;

import javax.inject.Provider;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.WeakHashMap;
import java.util.function.Function;
import java.util.function.Supplier;

import static io.deephaven.extensions.barrage.util.GrpcUtil.safelyExecuteLocked;

Expand All @@ -33,12 +49,29 @@ public class PythonAutoCompleteObserver extends SessionCloseableObserver<AutoCom
* We only log timing for completions that take longer than, currently, 100ms
*/
private static final long HUNDRED_MS_IN_NS = 100_000_000;
private final Provider<ScriptSession> scriptSession;

public PythonAutoCompleteObserver(StreamObserver<AutoCompleteResponse> responseObserver,
Provider<ScriptSession> scriptSession, final SessionState session) {
/** Track parsers by their session state, to ensure each session has its own, singleton, parser */
private static final Map<SessionState, JediCompleter> parsers = Collections.synchronizedMap(new WeakHashMap<>());
Comment on lines +53 to +54
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extremely unsafe to do this.

jedi is NOT threadsafe and should NOT be accessed by more than one caller at a time.
I plan to eventually put in some kind of threading and locking that ensures we don't accidentally use it in a parallelized manner, and having only a single completer would help with that...


private JediCompleter ensureParserForSession(SessionState session, Supplier<JediCompleter> supplier) {
return parsers.computeIfAbsent(session, s -> {
final JediCompleter jedi = supplier.get();
s.addOnCloseCallback(() -> {
parsers.remove(s);
jedi.close();
});
return jedi;
});
}

private final JediCompleter jedi;

public PythonAutoCompleteObserver(
StreamObserver<AutoCompleteResponse> responseObserver,
SessionState session,
Supplier<JediCompleter> supplier) {
super(session, responseObserver);
this.scriptSession = scriptSession;
this.jedi = ensureParserForSession(session, supplier);
}

@Override
Expand All @@ -48,27 +81,22 @@ public void onNext(AutoCompleteRequest value) {
case OPEN_DOCUMENT: {
final OpenDocumentRequest openDoc = value.getOpenDocument();
final TextDocumentItem doc = openDoc.getTextDocument();
PyObject completer = (PyObject) scriptSession.get().getVariable("jedi_settings");
completer.callMethod("open_doc", doc.getText(), doc.getUri(), doc.getVersion());
jedi.open_doc(doc.getText(), doc.getUri(), doc.getVersion());
break;
}
case CHANGE_DOCUMENT: {
ChangeDocumentRequest request = value.getChangeDocument();
final VersionedTextDocumentIdentifier text = request.getTextDocument();

PyObject completer = (PyObject) scriptSession.get().getVariable("jedi_settings");
String uri = text.getUri();
int version = text.getVersion();
String document = completer.callMethod("get_doc", text.getUri()).getStringValue();

String document = jedi.get_doc(text.getUri());
final List<ChangeDocumentRequest.TextDocumentContentChangeEvent> changes =
request.getContentChangesList();
document = CompletionParser.updateDocumentChanges(uri, version, document, changes);
if (document == null) {
return;
}

completer.callMethod("update_doc", document, uri, version);
jedi.update_doc(document, uri, version);
break;
}
case GET_COMPLETION_ITEMS: {
Expand All @@ -85,13 +113,11 @@ public void onNext(AutoCompleteRequest value) {
}
case CLOSE_DOCUMENT: {
CloseDocumentRequest request = value.getCloseDocument();
PyObject completer = (PyObject) scriptSession.get().getVariable("jedi_settings");
completer.callMethod("close_doc", request.getTextDocument().getUri());
jedi.close_doc(request.getTextDocument().getUri());
break;
}
case REQUEST_NOT_SET: {
throw GrpcUtil.statusRuntimeException(Code.INVALID_ARGUMENT,
"Autocomplete command missing request");
throw GrpcUtil.statusRuntimeException(Code.INVALID_ARGUMENT, "Autocomplete command missing request");
}
}
}
Expand All @@ -101,33 +127,18 @@ private void getCompletionItems(GetCompletionItemsRequest request,
StreamObserver<AutoCompleteResponse> responseObserver) {
final ScriptSession scriptSession = exportedConsole.get();
try (final SafeCloseable ignored = scriptSession.getExecutionContext().open()) {

PyObject completer = (PyObject) scriptSession.getVariable("jedi_settings");
boolean canJedi = completer.callMethod("is_enabled").getBooleanValue();
if (!canJedi) {
log.trace().append("Ignoring completion request because jedi is disabled").endl();
// send back an empty, failed response...
safelyExecuteLocked(responseObserver,
() -> responseObserver.onNext(AutoCompleteResponse.newBuilder()
.setCompletionItems(GetCompletionItemsResponse.newBuilder()
.setSuccess(false)
.setRequestId(request.getRequestId()))
.build()));
return;
}
Comment on lines -105 to -117
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if client uses jedi_settings.mode = 'OFF' to disable autocomplete in their session?

final VersionedTextDocumentIdentifier doc = request.getTextDocument();
final Position pos = request.getPosition();
final long startNano = System.nanoTime();

if (log.isTraceEnabled()) {
String text = completer.call("get_doc", doc.getUri()).getStringValue();
String text = jedi.get_doc(doc.getUri());
log.trace().append("Completion version ").append(doc.getVersion())
.append(" has source code:").append(text).endl();
}
final PyObject results = completer.callMethod("do_completion", doc.getUri(), doc.getVersion(),
// our java is 0-indexed lines, 1-indexed chars. jedi is 1-indexed-both.
// we'll keep that translation ugliness to the in-java result-processing.
pos.getLine() + 1, pos.getCharacter());
// our java is 0-indexed lines, 1-indexed chars. jedi is 1-indexed-both.
// we'll keep that translation ugliness to the in-java result-processing.
final PyObject results =
jedi.do_completion(doc.getUri(), doc.getVersion(), pos.getLine() + 1, pos.getCharacter());
if (!results.isList()) {
throw new UnsupportedOperationException(
"Expected list from jedi_settings.do_completion, got " + results.call("repr"));
Expand Down Expand Up @@ -214,7 +225,7 @@ private void getCompletionItems(GetCompletionItemsRequest request,
}
}

private String toMillis(final long totalNanos) {
private static String toMillis(final long totalNanos) {
StringBuilder totalNano = new StringBuilder(Long.toString(totalNanos));
while (totalNano.length() < 7) {
totalNano.insert(0, "0");
Expand Down