Jedi cleanup and fixes #3174
Jedi cleanup and fixes #3174
Conversation
Uses PyObject proxy for more explicit methods. Fixes state management, uses per-session instead of per stream state. Fixes SAFE mode. Related to deephaven#3173
devinrsmith
added
bug
|
Will need merge conflict fix after #3176 |
| from ._completer import Completer, Mode | ||
|
|
||
| from ._completer import Completer, Mode, jedi_settings | ||
| from jedi import preload_module, Interpreter | ||
|
|
||
| jedi_settings = Completer() |
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
a code comment here would be nice
| 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(); |
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
if we raise the log level so it's easily noticed, we can include a message about how to disable autocomplete altogether.
| /** 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<>()); |
There was a problem hiding this comment.
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...
| 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; | ||
| } |
There was a problem hiding this comment.
what happens if client uses jedi_settings.mode = 'OFF' to disable autocomplete in their session?
There was a problem hiding this comment.
Don found an autocomplete bug the other day: deephaven/web-client-ui#969
Essentially if the UI calls closeDocument without calling openDocument first, it kills the connection. While this is an error, it probably shouldn't kill the connection in that case, just throw an error for that issue.
|
@devinrsmith Any chance this could be cleaned up and pushed over the line soonish? I'd like to plumb param help and hover help through Jedi, but I think this should be merged first |
|
@mattrunyon - there have been some other changes wrt autocomplete; I'll sync up w/ @niloc132 and @rcaudy to see how we should proceed. |
|
@devinrsmith Are those changes merged or in review? Just want to know if there's anything else I should look out for when implementing the other LSP functions |
|
The simpler-fixes were merged in (sometime after this PR was "abandoned" as more complex than needed for release). That said, I don't remember the full context, and if we want to revive some of the improvements from this PR, we'll need to prioritize. I'm not sure exactly how it fits in w/ LSP improvements. |
|
I'm looking to add support of more request types. Would involve modifying the proto message to include more than just completion requests. And then modifying the Java/Python that wires the completion requests currently to expand to other requests. I don't think this cleanup is needed for me to wire it through, it just might cause this cleanup to have more conflicts. |
pete-petey
modified the milestones:
August 2023,
2023 tickets than need to be 'milestone-ed' and assigned
pete-petey
modified the milestones:
Year 2023 tickets that need to be 'milestone-ed' and assigned,
5. Backlog
Uses PyObject proxy for more explicit methods.
Fixes state management, uses per-session instead of per-stream state.