Use language server implementation instead of language for URLs #199

bollwyvl · 2020-02-15T21:52:15Z

References

fixes Use implementation name rather than language for WebSocket URLs #192

Code changes

backend
- uses implementation name for websocket key, e.g. /lsp/ws/pyls
- moves status to /lsp/status
- move reading/writing to stdio language servers to threads
- move reading/writing to shadow files to threads
fix frontend
- introduce ILanguageServerManager
- use ILSM in ConnectionManager and StatusBarItem

User-facing changes

None, other than launching a document with a language already supported by another language will be faster, as there's only one actual WebSocket/process behind them

Backwards-incompatible changes

bumps schema to version 2
- changes session payload to be a dict, e.g. {"session": {"pyls": {}}}

Chores

linted
tested
documented
changelog entry

bollwyvl · 2020-02-15T21:53:06Z

starting point for getting the frontend to compile:
../jupyterlab-lsp/src/adapters/jupyterlab/components/statusbar.tsx(341,7): error TS2740: Type '{ [k: string]: LanguageServerSession; }' is missing the following properties from type 'LanguageServerSession[]': length, pop, push, concat, and 26 more.
here we go...

bollwyvl · 2020-02-15T21:56:47Z

py_src/jupyter_lsp/tests/conftest.py

-    "typescript-jsx",
-    "typescript",
-    "yaml",
+KNOWN_SERVERS = [


nice thing here is it is now slightly faster...

bollwyvl · 2020-02-18T01:07:54Z

Turns out I didn't have to do #190, so I didn't. Otherwise seems pretty good!

bollwyvl · 2020-02-18T01:09:47Z

atest/01_Editor.robot

@@ -19,7 +19,7 @@ CSS

 Docker
    ${def} =    Set Variable    xpath://span[contains(@class, 'cm-string')][contains(text(), 'PLANET')]
-    Editor Shows Features for Language    Docker    Dockerfile    Diagnostics=Instruction has no arguments    Jump to Definition=${def}    Rename=${def}
+    Wait Until Keyword Succeeds  3x  100ms  Editor Shows Features for Language    Docker    Dockerfile    Diagnostics=Instruction has no arguments    Jump to Definition=${def}    Rename=${def}


So this "fixes" the problem, but the fact remains that you don't reliably get diagnostics back from a dockerfile on the first file opened until you change the file (then it seems to work every time). I tried sending more change events, but it didn't have an effect. I'm willing to deal with it if it makes the tests pass for now.

bollwyvl · 2020-02-18T12:57:29Z

py_src/jupyter_lsp/stdio.py

@@ -34,6 +40,7 @@ def __repr__(self):  # pragma: no cover
    def __init__(self, **kwargs):
        super().__init__(**kwargs)
        self.log.debug("%s initialized", self)
+        self.executor = ThreadPoolExecutor(max_workers=1)


I thought all of the jiggery pokery i did with the streams was sufficient to handle this, but I've still hit some slowdown. This should Make Sure we don't block the main thread on IO below

bollwyvl · 2020-02-18T13:00:13Z

py_src/jupyter_lsp/stdio.py

@@ -87,7 +94,7 @@ def wake(self):
                else:
                    self.wake()

-                await self.queue.put(message)
+                IOLoop.current().add_callback(self.queue.put_nowait, message)


again, I don't think this was causing issues, but better be safe...

bollwyvl · 2020-02-18T13:00:33Z

py_src/jupyter_lsp/stdio.py

@@ -98,12 +105,12 @@ def wake(self):
        message = ""
        headers = HTTPHeaders()

-        line = self._readline()
+        line = await convert_yielded(self._readline())


there are other wrappers that wouldn't require converting, but this works...

bollwyvl · 2020-02-18T13:01:38Z

py_src/jupyter_lsp/stdio.py

+
+    @run_on_executor
+    def _write_one(self, message) -> None:
+        self.stream.write(message)


could have moved more string operations here, but keeps the critical region more clear

packages/jupyterlab-lsp/src/virtual/editor.spec.ts

bollwyvl · 2020-02-22T17:33:01Z

I'll have a look at fixing the merge conflicts presently...

krassowski

LGTM. I note two debug calls that would be best moved to a console script the future, but they are not a big hurdle right now. Will merge in 2 hours.

krassowski · 2020-02-22T17:36:25Z

Ok, I will wait!

bollwyvl · 2020-02-22T19:28:24Z

Ok, provided docker doesn't flake out on us, this is good to go if CI passes.

bollwyvl · 2020-02-22T20:46:24Z

py_src/jupyter_lsp/virtual_documents_shadow.py

@@ -16,11 +23,19 @@ def extract_or_none(obj, path):


 class EditableFile:
+    executor = ThreadPoolExecutor(max_workers=MAX_WORKERS)


following the same approach as stdio, but in this case we have one executor for all instances of EditableFile. 4 seems like enough for now.

bollwyvl · 2020-02-22T20:46:55Z

py_src/jupyter_lsp/virtual_documents_shadow.py

+        self.lines = await convert_yielded(self.read_lines())
+
+    async def write(self):
+        return await convert_yielded(self.write_lines())


these just wrap the underlying tornado yield-style async stuff.

bollwyvl · 2020-02-22T20:48:27Z

py_src/jupyter_lsp/virtual_documents_shadow.py

-        file = EditableFile(path)
+        editable_file = EditableFile(path)
+
+        await editable_file.read()


renaming is cosmetic, just don't need to go re-defining builtins without a good reason.

It's a little ugly to have to do the read: could add an async class method to EditableFile to do both in one go...

It's a small hill, but file is not a builtin in Python3 and it is ok to use it. It was in Python2, and yes some syntax highlighters did not catch up, but we can use it now.

bollwyvl · 2020-02-22T20:49:42Z

have definitely still been getting the dockerfile issue locally... very challenging to reproduce, apparently...

bollwyvl · 2020-02-22T21:32:00Z

ha, fair.

…

On Sat, Feb 22, 2020 at 4:25 PM Michał Krassowski ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In py_src/jupyter_lsp/virtual_documents_shadow.py <https://github.com/krassowski/jupyterlab-lsp/pull/199#discussion_r382942639> : > @@ -125,7 +141,9 @@ def setup_shadow_filesystem(virtual_documents_uri): return path = file_uri_to_path(uri) - file = EditableFile(path) + editable_file = EditableFile(path) + + await editable_file.read() It's a small hill, but file is not a builtin in Python3 <https://stackoverflow.com/a/24942363/6646912> and it is ok to use it. It was in Python2, and yes some syntax highlighters did not catch up, but we can use it now. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <https://github.com/krassowski/jupyterlab-lsp/pull/199?email_source=notifications&email_token=AAALCRGSJZJPJAAIQXKAOPLREGJ3XA5CNFSM4KV4STL2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCWR3VKY#discussion_r382942639>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAALCRGS7KTUEWXL4PBDSXLREGJ3XANCNFSM4KV4STLQ> .

bollwyvl · 2020-02-22T21:37:44Z

I swear I'm done tinkering now.

krassowski · 2020-02-22T22:23:34Z

Now let's hope we will have CI pass on master ;)

bollwyvl · 2020-02-22T23:36:51Z

Hooray merge! Boo ci fail! We could mark it as non-critical, but those have a tendency to get ignored. looks like there's a new version we could try. I guess we need an issue. Aside from fixing the docker issue, what's the next thing we want to get to? With this PR in, per-server configuration is unblocked, and would help a lot, as is the mimetype-based detection stuff. I'm also interested in the multiple connection providers: being able to install js-only servers as lazy loading labextensions would help the install complexity around run-time node without negatively impacting performance much: and json and markdown would be great initial candidates, especially if more things like settings manager/cell metadata and markdown cells could be extended.

…

start refactor of language server URLs

d6a8394

bollwyvl commented Feb 15, 2020

View reviewed changes

bollwyvl added 5 commits February 17, 2020 00:29

start language server manager

a1e37cd

mock language server manager for python

b1acd07

wuks docker

f2f8ae5

update changelog

61bf62d

more changelog

b0b3571

bollwyvl changed the title ~~[wip] use language server implementation instead of language for URLs~~ Use language server implementation instead of language for URLs Feb 18, 2020

bollwyvl commented Feb 18, 2020

View reviewed changes

This was referenced Feb 18, 2020

Use implementation name rather than language for WebSocket URLs #192

Closed

Some docs cleanup #194

Merged

bollwyvl added 2 commits February 18, 2020 07:27

move queue puts to main loop

371eedf

do stdio read/write in threadpool

35eaa1f

bollwyvl commented Feb 18, 2020

View reviewed changes

use put_nowait for queue reading for consistency

536790d

bollwyvl commented Feb 18, 2020

View reviewed changes

krassowski reviewed Feb 22, 2020

View reviewed changes

packages/jupyterlab-lsp/src/virtual/editor.spec.ts Show resolved Hide resolved

krassowski approved these changes Feb 22, 2020

View reviewed changes

bollwyvl added 3 commits February 22, 2020 14:18

merge master

67a593a

promote connection/language server managers to test environment

99dc206

add listener api changes to docs notebook

45da449

bollwyvl added 2 commits February 22, 2020 14:55

some updating of the architecture docs

6078327

revisit frontend arch a touch

5bfe3a2

bollwyvl added 3 commits February 22, 2020 15:03

ignore graphviz mypy errors

61e633d

also make shadow file ops async

5bc7b5f

lint architecture

aaf623a

bollwyvl commented Feb 22, 2020

View reviewed changes

add note about threading concerns

d8966e0

krassowski merged commit f3fd809 into jupyter-lsp:master Feb 22, 2020

bollwyvl mentioned this pull request Feb 23, 2020

Partial freeze of jupyterlab when typing "pd.read_pickle" #200

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use language server implementation instead of language for URLs #199

Use language server implementation instead of language for URLs #199

bollwyvl commented Feb 15, 2020 •

edited

Loading

bollwyvl commented Feb 15, 2020 •

edited

Loading

bollwyvl Feb 15, 2020

bollwyvl commented Feb 18, 2020

bollwyvl Feb 18, 2020

bollwyvl Feb 18, 2020

bollwyvl Feb 18, 2020

bollwyvl Feb 18, 2020

bollwyvl Feb 18, 2020

bollwyvl commented Feb 22, 2020

krassowski left a comment

krassowski commented Feb 22, 2020

bollwyvl commented Feb 22, 2020

bollwyvl Feb 22, 2020

bollwyvl Feb 22, 2020

bollwyvl Feb 22, 2020

krassowski Feb 22, 2020

bollwyvl commented Feb 22, 2020

bollwyvl commented Feb 22, 2020 via email

bollwyvl commented Feb 22, 2020

krassowski commented Feb 22, 2020

bollwyvl commented Feb 22, 2020 via email

		@@ -16,11 +23,19 @@ def extract_or_none(obj, path):


		class EditableFile:
		executor = ThreadPoolExecutor(max_workers=MAX_WORKERS)

Use language server implementation instead of language for URLs #199

Use language server implementation instead of language for URLs #199

Conversation

bollwyvl commented Feb 15, 2020 • edited Loading

References

Code changes

User-facing changes

Backwards-incompatible changes

Chores

bollwyvl commented Feb 15, 2020 • edited Loading

Choose a reason for hiding this comment

bollwyvl commented Feb 18, 2020

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

bollwyvl commented Feb 22, 2020

krassowski left a comment

Choose a reason for hiding this comment

krassowski commented Feb 22, 2020

bollwyvl commented Feb 22, 2020

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

bollwyvl commented Feb 22, 2020

bollwyvl commented Feb 22, 2020 via email

bollwyvl commented Feb 22, 2020

krassowski commented Feb 22, 2020

bollwyvl commented Feb 22, 2020 via email

bollwyvl commented Feb 15, 2020 •

edited

Loading

bollwyvl commented Feb 15, 2020 •

edited

Loading