[ZEPPELIN-3645] Add LSP Protocol completion support #3090

oxygen311 · 2018-07-23T11:48:15Z

What is this PR for?

Add support of request autocomplete with LSP Protocol through TCP.

What type of PR is it?

Feature

What is the Jira issue?

ZEPPELIN-3645

Demo

Questions:

Does the licenses files need update? Yes, updated
Is there breaking changes for older versions? No
Does this needs documentation? Yes, updated

zjffdu · 2018-07-23T13:49:31Z

Is there any document of how to use this ? Like how to configure and start the LSP server

oxygen311 · 2018-07-23T13:59:42Z

Now this feature is enabled only in python interpreter. So you can specify zeppelin.python.lspHost and zeppelin.python.lspPort properties and use it.
Server is a separate process. I have been using python-language-server. But there are much more server implementations that can be used with any language.

zjffdu · 2018-07-23T14:05:33Z

Could you add instruction in python.md ? Otherwise it is hard for users to know how to enable this feature except you.

oxygen311 · 2018-07-23T16:02:07Z

@zjffdu Sound like good idea)
Done

jongyoul · 2018-07-23T23:42:23Z

zeppelin-interpreter/src/main/java/org/apache/zeppelin/completer/LSPUtils.java

+          server.getTextDocumentService().completion(getCompletionParams(buf, cursor));
+
+      Either<List<CompletionItem>, CompletionList> either =
+          (Either<List<CompletionItem>, CompletionList>) future.get(3, TimeUnit.SECONDS);


BTW, if not set LSP, it waits for 3 seconds every time, doesn't it?

In my opinion, how about adding one more option to enable/disable it?

Right, agree wth @jongyoul Interpreter should know whether LSP is enabled or not.

what if the user try to auto complete before the LSP connects? "hanging" for 3 seconds when the user press tab seems pretty bad

This "hanging" for 3 seconds only exists only if the connection to the server is successful. But i think it's good idea to add property for enable/disable this whole feature.

jongyoul · 2018-07-23T23:43:51Z

zeppelin-interpreter/src/test/java/org/apache/zeppelin/completer/LSPUtilsTest.java

+    try (ServerSocket serverSocket = new ServerSocket(0)) {
+      final int port = serverSocket.getLocalPort();
+
+      Thread t = new Thread(() -> {


Making thread has timing issue sometimes. How about using @Mock?

@Mock does not work with static methods. For this, powermock library is needed which will be additional dependency.

Even if we should powermock, it looks better than making tests fragile, in my opinion.

You are right, fragile test is bad practice. I changed it to PowerMock.

jongyoul · 2018-07-23T23:51:03Z

zeppelin-interpreter/pom.xml

@@ -51,6 +52,21 @@

  <dependencies>

+    <!-- https://mvnrepository.com/artifact/org.eclipse.lsp4j/org.eclipse.lsp4j -->
+    <dependency>
+      <groupId>org.eclipse.lsp4j</groupId>


It has guava as a dependency. I'm worried that it might make a problem for other interpreters.

We should not put lsp4j in zeppelin-interpreter. It is better to put it into python module

I agree to move it under python. Even if LSP supports most of the languages, zeppelin-interpreter influences many of interpreters. We should be careful when we add code in zeppelin-interpreter.

It's right of course.
But this code seems reusable in other interpreters. And it would be strange to add it in python module and use it in other interpreters like method from python module.

zjffdu · 2018-07-24T01:35:29Z

zeppelin-interpreter/pom.xml

+    <dependency>
+      <groupId>org.eclipse.lsp4j</groupId>
+      <artifactId>org.eclipse.lsp4j</artifactId>
+      <version>${lsp4j.version}</version>


What's the licence of this dependency ?

LSP4J is published under two licenses:

Eclipse Public License v1.0

Eclipse Distribution License - v 1.0 A BSD 3-clause License

zjffdu · 2018-07-24T01:36:37Z

zeppelin-interpreter/src/main/java/org/apache/zeppelin/completer/LSPUtils.java

+    Logger.info("Trying to complete on {} with cursor {}", buf, cursor);
+
+    List<InterpreterCompletion> list = Collections.emptyList();
+    try (Socket socket = new Socket(host, port)) {


It doesn't seems efficient to create new connection for each getCompletion call. getCompletion will be a very frequent call sometimes.

Yes, but we can't keep this connection because we can connect to server from different JVMs. And if one JVM process occupies connection, other JVMs will not be able to connect to server.

zjffdu · 2018-07-24T01:38:30Z

zeppelin-interpreter/src/main/java/org/apache/zeppelin/completer/LSPUtils.java

+    final String beforeCursor = buf.substring(0, actualCursor);
+    final int line = countLines(beforeCursor) - 1;
+    final int character = beforeCursor.length() - beforeCursor.lastIndexOf("\n") - 1;
+    Logger.info("Line: {}, character: {} from actual cursor: ", line, character, cursor);


change it to debug level

zjffdu · 2018-07-24T01:57:05Z

@oxygen311 I can not install it. Let me know if there any other dependency I need to install

➜  zeppelin pr/3090 ✗ pip install pyls
Collecting pyls
  Using cached https://files.pythonhosted.org/packages/de/b7/5a056c2cac8fd19c905ae7451c2a60b9195531959c3770a49d6be4d905e3/pyls-0.1.3.zip
    Complete output from command python setup.py egg_info:
    Downloading http://pypi.python.org/packages/source/d/distribute/distribute-0.6.14.tar.gz
    Traceback (most recent call last):
      File "/private/var/folders/dp/hmchg5dd3vbcvds26q91spdw0000gp/T/pip-install-7veml3im/pyls/setup_utils/distribute_setup.py", line 143, in use_setuptools
        raise ImportError
    ImportError

    During handling of the above exception, another exception occurred:

    Traceback (most recent call last):
      File "<string>", line 1, in <module>
      File "/private/var/folders/dp/hmchg5dd3vbcvds26q91spdw0000gp/T/pip-install-7veml3im/pyls/setup.py", line 31, in <module>
        main()
      File "/private/var/folders/dp/hmchg5dd3vbcvds26q91spdw0000gp/T/pip-install-7veml3im/pyls/setup.py", line 11, in main
        distribute_setup.use_setuptools()
      File "/private/var/folders/dp/hmchg5dd3vbcvds26q91spdw0000gp/T/pip-install-7veml3im/pyls/setup_utils/distribute_setup.py", line 145, in use_setuptools
        return _do_download(version, download_base, to_dir, download_delay)
      File "/private/var/folders/dp/hmchg5dd3vbcvds26q91spdw0000gp/T/pip-install-7veml3im/pyls/setup_utils/distribute_setup.py", line 124, in _do_download
        to_dir, download_delay)
      File "/private/var/folders/dp/hmchg5dd3vbcvds26q91spdw0000gp/T/pip-install-7veml3im/pyls/setup_utils/distribute_setup.py", line 193, in download_setuptools
        src = urlopen(url)
      File "/Users/jzhang/anaconda3/lib/python3.6/urllib/request.py", line 223, in urlopen
        return opener.open(url, data, timeout)
      File "/Users/jzhang/anaconda3/lib/python3.6/urllib/request.py", line 532, in open
        response = meth(req, response)
      File "/Users/jzhang/anaconda3/lib/python3.6/urllib/request.py", line 642, in http_response
        'http', request, response, code, msg, hdrs)
      File "/Users/jzhang/anaconda3/lib/python3.6/urllib/request.py", line 570, in error
        return self._call_chain(*args)
      File "/Users/jzhang/anaconda3/lib/python3.6/urllib/request.py", line 504, in _call_chain
        result = func(*args)
      File "/Users/jzhang/anaconda3/lib/python3.6/urllib/request.py", line 650, in http_error_default
        raise HTTPError(req.full_url, code, msg, hdrs, fp)
    urllib.error.HTTPError: HTTP Error 403: SSL is required

    ----------------------------------------
Command "python setup.py egg_info" failed with error code 1 in /private/var/folders/dp/hmchg5dd3vbcvds26q91spdw0000gp/T/pip-install-7veml3im/pyls/

zjffdu · 2018-07-24T01:57:06Z

@oxygen311 I can not install it. Let me know if there any other dependency I need to install

➜  zeppelin pr/3090 ✗ pip install pyls
Collecting pyls
  Using cached https://files.pythonhosted.org/packages/de/b7/5a056c2cac8fd19c905ae7451c2a60b9195531959c3770a49d6be4d905e3/pyls-0.1.3.zip
    Complete output from command python setup.py egg_info:
    Downloading http://pypi.python.org/packages/source/d/distribute/distribute-0.6.14.tar.gz
    Traceback (most recent call last):
      File "/private/var/folders/dp/hmchg5dd3vbcvds26q91spdw0000gp/T/pip-install-7veml3im/pyls/setup_utils/distribute_setup.py", line 143, in use_setuptools
        raise ImportError
    ImportError

    During handling of the above exception, another exception occurred:

    Traceback (most recent call last):
      File "<string>", line 1, in <module>
      File "/private/var/folders/dp/hmchg5dd3vbcvds26q91spdw0000gp/T/pip-install-7veml3im/pyls/setup.py", line 31, in <module>
        main()
      File "/private/var/folders/dp/hmchg5dd3vbcvds26q91spdw0000gp/T/pip-install-7veml3im/pyls/setup.py", line 11, in main
        distribute_setup.use_setuptools()
      File "/private/var/folders/dp/hmchg5dd3vbcvds26q91spdw0000gp/T/pip-install-7veml3im/pyls/setup_utils/distribute_setup.py", line 145, in use_setuptools
        return _do_download(version, download_base, to_dir, download_delay)
      File "/private/var/folders/dp/hmchg5dd3vbcvds26q91spdw0000gp/T/pip-install-7veml3im/pyls/setup_utils/distribute_setup.py", line 124, in _do_download
        to_dir, download_delay)
      File "/private/var/folders/dp/hmchg5dd3vbcvds26q91spdw0000gp/T/pip-install-7veml3im/pyls/setup_utils/distribute_setup.py", line 193, in download_setuptools
        src = urlopen(url)
      File "/Users/jzhang/anaconda3/lib/python3.6/urllib/request.py", line 223, in urlopen
        return opener.open(url, data, timeout)
      File "/Users/jzhang/anaconda3/lib/python3.6/urllib/request.py", line 532, in open
        response = meth(req, response)
      File "/Users/jzhang/anaconda3/lib/python3.6/urllib/request.py", line 642, in http_response
        'http', request, response, code, msg, hdrs)
      File "/Users/jzhang/anaconda3/lib/python3.6/urllib/request.py", line 570, in error
        return self._call_chain(*args)
      File "/Users/jzhang/anaconda3/lib/python3.6/urllib/request.py", line 504, in _call_chain
        result = func(*args)
      File "/Users/jzhang/anaconda3/lib/python3.6/urllib/request.py", line 650, in http_error_default
        raise HTTPError(req.full_url, code, msg, hdrs, fp)
    urllib.error.HTTPError: HTTP Error 403: SSL is required

    ----------------------------------------
Command "python setup.py egg_info" failed with error code 1 in /private/var/folders/dp/hmchg5dd3vbcvds26q91spdw0000gp/T/pip-install-7veml3im/pyls/

felixcheung

maybe change the PR title to say python?

btw, why is it LSP?

felixcheung

how is connecting to LSP secured? connecting to a port on localhost without any authentication and then send all content from the paragraph to it sounds very dangerous?

felixcheung · 2018-07-24T06:36:01Z

python/src/main/java/org/apache/zeppelin/python/PythonInterpreter.java

@@ -464,12 +467,23 @@ public int getProgress(InterpreterContext context) throws InterpreterException {
  public List<InterpreterCompletion> completion(String buf, int cursor,
                                                InterpreterContext interpreterContext)
      throws InterpreterException {
+
+    final List <InterpreterCompletion> completions = LSPUtils.
+        getLspServerComplitions(buf, cursor,


this doesn;t seem like the style for multi - line code

LSPUtils. getLspServerComplitions

other places have . in the 2nd line

also Complitions?

felixcheung · 2018-07-24T06:36:30Z

zeppelin-interpreter/pom.xml

@@ -37,6 +37,7 @@

  <properties>
    <!--library versions-->
+    <lsp4j.version>0.4.1</lsp4j.version>


need to add to LICENSE?

felixcheung · 2018-07-24T06:37:49Z

zeppelin-interpreter/src/main/java/org/apache/zeppelin/completer/LSPUtils.java

+      server.initialize(new InitializeParams());
+
+      server.getTextDocumentService().didOpen(new DidOpenTextDocumentParams(
+          new TextDocumentItem(ANY_URI, langId, ANY_VERSION, buf)


what is version = ANY_VERSION for?

This is described in the const definition.

hm, it just says it is not used Zeppelin don't support file versions, so we can pass any version - but doesn't say if it does support version what it will do. Actually zeppelin does have notebook git support etc, and could have iteration or revision support.

felixcheung · 2018-07-24T06:39:15Z

zeppelin-interpreter/src/main/java/org/apache/zeppelin/completer/LSPUtils.java

+          server.getTextDocumentService().completion(getCompletionParams(buf, cursor));
+
+      Either<List<CompletionItem>, CompletionList> either =
+          (Either<List<CompletionItem>, CompletionList>) future.get(3, TimeUnit.SECONDS);


what if the user try to auto complete before the LSP connects? "hanging" for 3 seconds when the user press tab seems pretty bad

oxygen311 · 2018-07-24T08:29:32Z

@zjffdu
Sorry, correct line is pip install python-language-server, i will fix it.

oxygen311 · 2018-07-24T08:35:12Z

@felixcheung
I have added property zeppelin.python.useLsp which is disabled by default. It does not seem unsecured for me now. We also can specify host and port by ourselves with zeppelin.python.lspHost and zeppelin.python.lspPort properties.

zjffdu · 2018-07-25T08:34:39Z

Do you have any examples to show how much better LSP is compared to the current approach ?
Does the auto completion works across paragraphs ? e.g. I create string variable a in p1, can I get correct completion in p2 ?
What is the scope of LSP completion ? Is every call to LSP isolated ?

oxygen311 · 2018-07-25T09:20:43Z

@zjffdu

LSP is usable and powerful solution:

LSP can understand imports in the start of paragraph;
We can use already made solution for every language and don't write new one;
LSP server contains much more information than we process now.
E.g. it's able to show documentation of a method, reformat code, find references etc.

2-3. Now every paragraph is isolated, so a will not be shown.
It's made because in current implementation we can not keep connection to the server (because a lot of JVMs).
But maybe someone has better architecture ideas how to connect zeppelin to LSP.

zjffdu · 2018-07-25T09:42:34Z

Then it looks like LSP can only work in paragraph level. This seems not workable for users. Let's assume we also want to make PySpark support LSP. Then I guess if I type sc., I will get nothing for completion, Because sc is created by zeppelin not by any paragraph. Is that correct ?

zjffdu · 2018-07-25T09:43:36Z

Is there any session concept in LSP ? so that multiple LSP request could belong to the same session

oxygen311 · 2018-07-25T10:05:52Z

@zjffdu
If LSP has no complete for sc., iPython complete will be shown.
LSP does not support sessions, but it can keep connection.

Tagar · 2018-07-26T17:58:05Z

Very interesting improvements. Thanks for working on this.
Shouldn't LSP server be embedded into Python / PySpark interpreter itself and not be a separate process?
This would address both security concerns and ability to grasp code across paragraphs like in the above example.

zjffdu · 2018-07-27T02:01:41Z

@oxygen311 This is what I concern about the behavior consistency. Let's assume user use PySpark (ipython is disabled). User may find code completion works pretty well in p1, but works badly in p2 because LSP only works in paragraph level.

BTW, it is weird to me that LSP don't support concept of session. That would mean If I am writing a long code (1000 lines of code), I need to send all these code to LSP for code completion each time, this seems very inefficient. But I may be wrong. I am so familiar with LSP. Just want to confirm whether there's more efficient way to use LSP

felixcheung · 2018-07-27T06:07:44Z

open ip and port to connect to has become a huge problem recently, so unless LSP has some sort of authentication story, my vote would be "no" even if this is disabled by default, because this can become a support issue when users do have it enable.

#3090 (review)

mebelousov · 2018-07-27T09:34:40Z

@felixcheung I agree that support of LSP in Zeppelin is unmature idea.

Share please undesired cases with open ip and port.

oxygen311 · 2018-07-27T14:34:05Z

Solution must be improved to use by real users.

oxygen311 added 2 commits July 20, 2018 19:07

Add basic support of LSP Protocol

299fc9a

Fix variable name

f0066ad

Add documentation to python.md

028828b

jongyoul reviewed Jul 23, 2018

View reviewed changes

zjffdu reviewed Jul 24, 2018

View reviewed changes

felixcheung reviewed Jul 24, 2018

View reviewed changes

oxygen311 changed the title ~~[Zeppelin-3645] Add LSP Protocol completion support~~ [ZEPPELIN-3645] Add LSP Protocol completion support Jul 24, 2018

oxygen311 added 3 commits July 24, 2018 11:48

Fix PR

5a7a7ac

Change new Thread() to PowerMock in tests

f4b48f9

Update license

0b41cc9

oxygen311 closed this Jul 27, 2018

Savalek deleted the ZEPPELIN-3645 branch March 6, 2019 13:02

[ZEPPELIN-3645] Add LSP Protocol completion support #3090

[ZEPPELIN-3645] Add LSP Protocol completion support #3090

Conversation

oxygen311 commented Jul 23, 2018 • edited

What is this PR for?

What type of PR is it?

What is the Jira issue?

Demo

Questions:

zjffdu commented Jul 23, 2018

oxygen311 commented Jul 23, 2018

zjffdu commented Jul 23, 2018 • edited

oxygen311 commented Jul 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zjffdu commented Jul 24, 2018

zjffdu commented Jul 24, 2018

felixcheung left a comment

Choose a reason for hiding this comment

felixcheung left a comment

Choose a reason for hiding this comment

felixcheung Jul 24, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxygen311 commented Jul 24, 2018

oxygen311 commented Jul 24, 2018

zjffdu commented Jul 25, 2018

oxygen311 commented Jul 25, 2018 • edited

zjffdu commented Jul 25, 2018

zjffdu commented Jul 25, 2018

oxygen311 commented Jul 25, 2018

Tagar commented Jul 26, 2018

zjffdu commented Jul 27, 2018

felixcheung commented Jul 27, 2018

mebelousov commented Jul 27, 2018

oxygen311 commented Jul 27, 2018

oxygen311 commented Jul 23, 2018 •

edited

zjffdu commented Jul 23, 2018 •

edited

felixcheung Jul 24, 2018 •

edited

oxygen311 commented Jul 25, 2018 •

edited