-
Notifications
You must be signed in to change notification settings - Fork 80
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
Jedi Autocomplete #3114
Jedi Autocomplete #3114
Conversation
@@ -10,6 +10,7 @@ def targetArch = Architecture.targetArchitecture(project) | |||
|
|||
def baseMapAmd64 = [ | |||
'server-base': 'server-jetty', | |||
'all-ai-base': 'server-all-ai-jetty', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is what I've been doing for community deployments. Does not really belong in this branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to leave it in unless someone complains. Just makes doing deployments that build deephaven-core easier / possible.
class Completer(object): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be a 'private' class for creating a singleton, if so, change it to _Completer ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Java actually holds onto this object and calls into its methods, but the consumer would probably only care about jedi_settings
and not Completer
. Will rename it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still need direction on this.
Also, should I move it to its own file, or leave things in __init__.py
. Most others, init seems very sparse, if you'd rather structure it differently, please lmk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't think there will likely be more modules in the completer sub-package, I would just have _completer.py and keep the class name as it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
jedi_settings = Completer() | ||
|
||
if jedi_settings.mode == CompleterMode.off: | ||
print('No jedi library installed on system path, disabling autocomplete') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this safe? Does it go into the server log?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should probably just move the logging out of python and into the calling code's logger
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It goes to the console log in the gui
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving the logging info into the calling code makes sense. The current code looks a little odd as it only logs in one case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. I just deleted it since there was a similar method already printing effectively the same message to java logs.
String totalNano = Long.toString(System.nanoTime() - startNano); | ||
// lets track how long completions take, as it's known that some | ||
// modules like numpy can cause slow completion, and we'll want to know what was causing them | ||
log.info().append("Jedi completions took ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debug probably, or guarded with a flag or a "this was shorter than 100ms, so we don't want to bother logging an INFO for it"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, logging should be pulled out of here, since we probably a) don't want to count IO in the jedi time, and b) don't want to fail to log time if IO blew up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cleaned up the logging and set it to TRACE.
Also, if I'm only logging > 100ms, I can probably make stronger guesses about string length and simplify the toString ugliness too...
Also, where should I move logging to make it not-count?
Also, who is counting / cares about how long this method takes?
I did the logging part after the work part completed, so if there is a better location / way to do this, you'll have to point me at it explicitly.
server/src/main/java/io/deephaven/server/console/completer/PythonAutoCompleteObserver.java
Outdated
Show resolved
Hide resolved
server/src/main/java/io/deephaven/server/console/completer/PythonAutoCompleteObserver.java
Show resolved
Hide resolved
server/src/main/java/io/deephaven/server/console/completer/PythonAutoCompleteObserver.java
Outdated
Show resolved
Hide resolved
List<CompletionItem.Builder> completionResults_ = new ArrayList<>(); | ||
List<CompletionItem.Builder> completionResults__ = new ArrayList<>(); | ||
for (PyObject completion : completes) { | ||
String completionName = completion.getAttribute("name").getStringValue(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
future cleanup, i don't see an easy/obvious way to address this right away: with so many calls into and out of python, we're going to be taking and releasing the GIL a lot, plus N JNI calls per completion item (and i dont see anything obvious that limits the results passed back to java) though i'm more worried about the GIL.
Instead, it would be nice if we could translate each item into a dict while in python, then hijack the copyPythonDictToJavaMap or the like to translate the data to java before giving up the GIL to begin with.
Note that high GIL contention is also going to throw off the perf numbers collected below - seems likely we should measure just the .complete() call above, and possibly separately measure the marshalling, IO. I think that's the only action item I'd have about this...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option for GIL management is to use the new PyLib.ensureGil(() -> {...}); wrapped around all of the code that actually reads and builds results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually got about 85% of the way to moving it all into python. I'll just finish that and delete this mess. I knew crossing the gil like that was terrible, I just did it as proof of concept. The whole point of the python module in this PR is to move the result processing into python, and just send back a glob (arrays of arrays) of strings and ints for java to process all at once.
I'll wrap that up since it's close and this, as-is, is terrible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was completed, you can see the brains in completer/__init__.py
17004ad
to
e0249c3
Compare
Also tells docker it's not allowed to download jedi from pypi, as it is now included in base images.
log.trace().append("Ignoring completion request because jedi is disabled").endl(); | ||
// send back an empty response. We may want to include a "don't ask again" flag in the response... | ||
safelyExecuteLocked(responseObserver, | ||
() -> responseObserver.onNext(AutoCompleteResponse.newBuilder().build())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to have the completed items response in here, with success=false, and requestId for the client to match the failure to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
// in the future we may want to get more interesting info from jedi to pass back to client | ||
final List<PyObject> items = result.asList(); | ||
String completionName = items.get(0).getStringValue(); | ||
int start = items.get(1).getIntValue(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I think we're still paying for each GIL trip, to do the getStringValue, getIntValue calls. Plus, each List.get call is a python __getitem__
invoke.
I still think we can punt on this, but will want to revisit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strange that it wouldn't just say "sending a list of primitives, lets do this all at once".
Maybe if I sent two lists, one of string, one of int, I could convince jpy to give me String[], int[] in two shots...
But, yes... for another day. I'll open some issues to track next steps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've confirmed the timing is as you suspect, and will open a ticket w/ an idea how to improve it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
py/server/setup.py
needs to be updated with the appropriate jedi dependency.
I'm not seeing method completion like I would expect to see. Not sure if I'm doing something wrong.
scriptSession.evaluateScript("from deephaven.completer import jedi_settings"); | ||
settings[0] = (PyObject) scriptSession.getVariable("jedi_settings"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, we shouldn't be modifying the users' scope. By importing via script here we are modifying it. We can import with org.jpy.PyModule#importModule
, and get access specific to us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we discussed offline, your patch which handles this is ideal, but don't wanna pull in too much when we're this close to the finish line.
ChangeDocumentRequest request = value.getChangeDocument(); | ||
final VersionedTextDocumentIdentifier text = request.getTextDocument(); | ||
|
||
PyObject completer = (PyObject) scriptSession.get().getVariable("jedi_settings"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming this is set on the script session is fragile. del jedi_settings
breaks auto complete.
We should be able to pass in a PyObject
as part of the constructor, I would think, so PythonAutoCompleteObserver can own it.
May be worthwhile to get a proxy around it so you don't need to do .callMethod(...)
(org.jpy.PyObject#createProxy(java.lang.Class<T>
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The canonical location is deephaven.completer.jedi_settings
, but how I'm accessing it from the scope here IS dangerous / bad. Your work to wrap it in a proxy object looks way better.
I've got some suggestions from JamesXNelson#9 |
from deephaven.completer._completer import Completer | ||
from jedi import preload_module, Interpreter | ||
|
||
jedi_settings = Completer() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs sphinx doc string.
@@ -0,0 +1,25 @@ | |||
# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this needs to be public, I would prefer it is called autocomplete
, since that is more clear to the user looking at the package.
off = 'off' | ||
safe = 'safe' | ||
strong = 'strong' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume these should be caps, since they are enum values.
from jedi import Interpreter, Script | ||
|
||
|
||
class CompleterMode(Enum): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything here needs appropriate docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of follow ups we can handle in another PR:
- config to disable auto-complete
- move code to deephaven_internal
- add autocomplete dependency for deephaven-server
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix my comments in future commits
Labels indicate documentation is required. Issues for documentation have been opened: How-to: https://github.com/deephaven/deephaven.io/issues/1930 |
No description provided.