Add initial pyodide support #218

alcarney · 2021-11-12T00:16:13Z

Description

With the recent releases of github.dev and vscode.dev I wanted to see if it was possible to use Pyodide to have a pygls powered language server running in a web browser. - Turns out it is! 😄

It's worth noting that due to the multiprocessing module not being available in Pyodide, @server.thread() methods will not work in the browser, but so far everything else appears to work.

This PR makes the few tweaks required to get pygls running in a browser context (see commit messages for details) as well as adding a new example extension that demonstrates how to get a simple language server up and running in a web version of VSCode.

This isn't quite ready to be merged yet as ~~mypy isn't happy about the import from Pyodide's js module and~~ I should probably update the docs with some notes on Pyodide, but I thought I'd open it now so that there's an opportunity for feedback 😄

Code review checklist (for code reviewer to complete)

Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
Title summarizes what is changing
Commit messages are meaningful (see this for details)
Tests have been included and/or updated, as appropriate
Docstrings have been included and/or updated, as appropriate
Standalone docs have been updated accordingly
CONTRIBUTORS.md was updated, as appropriate
Changelog has been updated, as needed (see CHANGELOG.md)

alcarney · 2021-11-12T00:19:37Z

pygls/protocol.py

@@ -378,6 +379,11 @@ def _send_data(self, data):
        if not data:
            return

+        if IS_PYODIDE:
+            from js import post_message


This requires there be a JavaScript post_message method be defined on the server's web worker.
See here

willemkokke · 2022-02-15T16:11:45Z

I would love to help out on this one, I want a to hook up a python LS running in pyodide to a monaco editor using Monaco-Language, and I've identified pygls as the path of least resistance.

Thanks for the pull request @alcarney, I was just about to rip multiprocessing out myself when I thought to check open PRs ;)

If there's anything I can do to help this land please let me know!

dgreisen · 2022-02-15T19:01:43Z

@willemkokke (and everybody else! :) - @danixeee doesn't have as much time for pygls right now as we'd all like. Open Law Library is looking for someone interested in a part-time (~40hrs/mo) paid position maintaining pygls. We are also looking for volunteer community maintainers. If you know anybody who might be interested, please let me know ([email protected]).

alcarney · 2022-02-23T22:17:45Z

I'd be interested in helping out as a volunteer where I can :)

tombh · 2022-05-30T16:40:05Z

This is such a cool PR! Thank you. We want to get this merged pretty much as is. Then we can start another issue or PR about adding some Pyodide-specific tests to make sure this keeps working. I've just added a couple of comments, if we can clear that up then LGTM.

alcarney · 2022-05-30T19:35:36Z

I've just added a couple of comments, if we can clear that up then LGTM.

Hmm.. the only comment I can see is this one - could you share a link to one of the others?

tombh · 2022-05-30T16:36:13Z

pygls/__init__.py


 IS_WIN = os.name == 'nt'
+IS_PYODIDE = 'pyodide' in sys.modules


Is there a chance of a false positive here? The only way that pyodide can be part of sys.modules is if the parent application explicitly imports it right?

@alcarney how about this? Do you see this?

I see them now.... not sure what happened there.

Is there a chance of a false positive here? The only way that pyodide can be part of sys.modules is if the parent application explicitly imports it right?

I'm assuming Pyodide is doing a little bit of magic to make it available?
It is the suggested method in the project's FAQ for detecting the runtime.

Ok, the suggested method must be pretty tried and tested then. Thanks

tombh · 2022-05-30T16:37:05Z

pygls/server.py

@@ -263,7 +267,7 @@ async def connection_made(websocket, _):
            self.shutdown()

    @property
-    def thread_pool(self) -> ThreadPool:
+    def thread_pool(self) -> "ThreadPool":


Do you recall the reason for the quote marks? "ThreadPool" is not the same type as ThreadPool right?

@alcarney and this?

I believe "ThreadPool" will be resolved to ThreadPool by tools like mypy during type checking.

If I remember rightly, the issue is that importing ThreadPool from multiprocessing crashed under Pyodide so this line disables the import (multiprocessing is not available under Pyodide anyway) meaning we have to switch to using quotes

That makes sense. So would this work too?

if not IS_PYODIDE: from multiprocessing.pool import ThreadPool else: ThreadPool = "ThreadPool" . . def thread_pool(self) -> ThreadPool:

Not sure…. I don’t know the specifics of typing enough to say either way.
Is there a particular reason for doing it that way? Can’t say it’s something I’ve seen before

So I tried the above form out and mypy didn't like it.

pygls/server.py:39: error: Cannot assign to a type pygls/server.py:39: error: Incompatible types in assignment (expression has type "str", variable has type "Type[ThreadPool]")

tombh · 2022-06-06T13:50:57Z

Thanks. So what about this?:

if IS_PYODIDE:
    ThreadPoolType = "ThreadPool"
else:
    from multiprocessing.pool import ThreadPool
    ThreadPoolType = ThreadPool
.
.

def thread_pool(self) -> ThreadPoolType:

I don't really know enough about mypy either. It just seems a shame to lose typing information when not using Pyodide

davelopez · 2022-06-06T14:33:53Z

Maybe you can use typing.TYPE_CHECKING see docs here.

alcarney · 2022-06-06T17:57:13Z

Thanks. So what about this?:

That also fails - but with slightly different error messages

pygls/server.py:41: error: Incompatible types in assignment (expression has type "Type[ThreadPool]", variable has type "str")
pygls/server.py:274: error: Variable "pygls.server.ThreadPoolType" is not valid as a type

It just seems a shame to lose typing information when not using Pyodide

I wouldn't say that the typing information is lost... VSCode understands the type fine even when it is quoted

And it's even possible to get the proper type at runtime

>>> import inspect
>>> import typing
>>> from pygls.server import Server
>>> print(inspect.getsource(Server.thread_pool.fget))
    @property
    def thread_pool(self) -> "ThreadPool":
        """Returns thread pool instance (lazy initialization)."""
        if not self._thread_pool:
            self._thread_pool = ThreadPool(processes=self._max_workers)

        return self._thread_pool

>>> typing.get_type_hints(Server.thread_pool.fget)
{'return': <class 'multiprocessing.pool.ThreadPool'>}
>>>

Maybe you can use typing.TYPE_CHECKING

Thanks for the suggestion! If we used typing.TYPE_CHECKING the original form could be changed to

if typing.TYPE_CHECKING:
    from multiprocessing.pool import ThreadPool
.
.

def thread_pool(self) -> "ThreadPool":

However this would break the typing.get_type_hints() step above

tombh · 2022-06-06T19:48:28Z

Thanks for all that info! Ok, so quoting is just another valid way of expressing a type, it doesn't mean the type is a literal.

So my worry is that somebody in the future will see this break in convention (of expressing the type with quotes when all other types are without quotes) and innocently remove the quotes, breaking Pyodide support (which doesn't have tests yet). More broadly speaking, I think it's best to strive to maintain the precedent that the core code dictates to non-core features. In that sense, here Pyodide should bend to the core, not the other way around. So quoting "ThreaPool" certainly isn't a big deal, but its break in precedent could be interpreted as a big deal.

I like the IS_PYODIDE guards, it'd be perfect if that last little issue of the type could live behind a guard too.

So here are the options I see:

At the very least that line should be commented, explaining the surprising break in precedent.
Better would be if the type had something in its name eg; "ThreadPoolAliasType" (comments are beyond the reach of tests and linters, and tend to get stale) or something that provided a clue for the quoting.
A complete test suite for Pyodide running on CI.
What about guarding the whole thread_pool() function(s)!? Eg;

if IS_PYODIDE:
    @property
    def thread_pool(self) -> ThreadPool:
        ...

    @property
    def thread_pool_executor(self) -> ThreadPoolExecutor:
        ...

alcarney · 2022-06-06T22:04:19Z

What about guarding the whole thread_pool() functions?

I like that idea, I imagine it should work and makes it a lot clearer that they can't be relied on in a Pyodide context as they simply won't exist.

A complete test suite for Pyodide running on CI.

Do you want to tackle that as part of this PR? After some very hasty googling I see a couple of options

We can try using @vscode/test-web to test the example VSCode extension
Pyodide looks like it uses selenium and/or playwright to test itself, I don't see an obvious reusable component in the repo but we might be able to use their test suite as inspiration

tombh · 2022-06-08T12:13:53Z

Personally I think the tests can be worked on afterwards, once the PR is merged. We can make a separate issue for that as not having tests for a feature should be considered a bug. You've already done so much cool work here, it's high time it made it into master so others can kick its tyres.

So do you want to put that guard at the function-level, then LGTM?

This mostly involves disabling some things that are not supported by the Pyodide runtime. - Add a constant `IS_PYODIDE` that can be used to check if we're running in the Pyodide runtime. - The `multiprocessing` module is not available so we don't import `ThreadPool`. This does mean that `@server.thread()` methods will not work. - Pyodide provides its own `asyncio` event loop implementation so, setting it in the server's constructor is unecessary. Most of the heavy lifting of the server's main loop can be handled by the web platform. Message objects are serialized to/from JSON to keep the Python <-> JavaScript translation simple. Client -> server messages are delivered by calling the `_procedure_handler` on the protocol class directly. e.g. ```ts self.client_message = JSON.stringify(event.data) await pyodide.runPythonAsync(` from js import client_message message = json.loads(client_message, object_hook=deserialize_message) server.lsp._procedure_handler(message) `) ``` Server -> client messages are delivered by calling a `post_message` JavaScript function that is made available to the Pyodide runtime, where the `post_message` function could be implemented as follows. ```ts self.post_message = (json) => { let obj = JSON.parse(json) postMessage(obj) } ``` See following commits for a more complete pyodide example.

alcarney · 2022-06-09T18:27:31Z

So do you want to put that guard at the function-level

Done, I've also rebased the branch on the latest master and bumped the version of pyodide in the example extension

tombh · 2022-06-09T19:17:41Z

Merged! 🥳 I'll add an issue to add tests...

alcarney commented Nov 12, 2021

View reviewed changes

alcarney force-pushed the pyodide-support branch from f6d1f9e to 629e6a0 Compare November 13, 2021 19:44

dgreisen mentioned this pull request Mar 4, 2022

Full-system automated integration tests #228

Open

tombh reviewed Jun 1, 2022

View reviewed changes

alcarney added 2 commits June 9, 2022 19:15

Add example web extension

83ad508

alcarney force-pushed the pyodide-support branch from 629e6a0 to 83ad508 Compare June 9, 2022 18:20

tombh approved these changes Jun 9, 2022

View reviewed changes

tombh merged commit 6e7bdf5 into openlawlibrary:master Jun 9, 2022

alcarney deleted the pyodide-support branch June 10, 2022 10:23

alcarney mentioned this pull request Jun 10, 2022

Add Pyodide Test Suite #239

Merged

8 tasks

alcarney mentioned this pull request Aug 21, 2023

Update example vscode extension #357

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial pyodide support #218

Add initial pyodide support #218

alcarney commented Nov 12, 2021 •

edited

Loading

alcarney Nov 12, 2021

willemkokke commented Feb 15, 2022

dgreisen commented Feb 15, 2022

alcarney commented Feb 23, 2022

tombh commented May 30, 2022

alcarney commented May 30, 2022

tombh May 30, 2022

tombh May 30, 2022

alcarney Jun 1, 2022

tombh Jun 2, 2022

tombh May 30, 2022

tombh May 30, 2022

alcarney Jun 1, 2022

tombh Jun 2, 2022

alcarney Jun 2, 2022

alcarney Jun 5, 2022

tombh commented Jun 6, 2022 •

edited

Loading

davelopez commented Jun 6, 2022

alcarney commented Jun 6, 2022 •

edited

Loading

tombh commented Jun 6, 2022

alcarney commented Jun 6, 2022

tombh commented Jun 8, 2022

alcarney commented Jun 9, 2022

tombh commented Jun 9, 2022


		IS_WIN = os.name == 'nt'
		IS_PYODIDE = 'pyodide' in sys.modules

Add initial pyodide support #218

Add initial pyodide support #218

Conversation

alcarney commented Nov 12, 2021 • edited Loading

Description

Code review checklist (for code reviewer to complete)

Choose a reason for hiding this comment

willemkokke commented Feb 15, 2022

dgreisen commented Feb 15, 2022

alcarney commented Feb 23, 2022

tombh commented May 30, 2022

alcarney commented May 30, 2022

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

tombh commented Jun 6, 2022 • edited Loading

davelopez commented Jun 6, 2022

alcarney commented Jun 6, 2022 • edited Loading

tombh commented Jun 6, 2022

alcarney commented Jun 6, 2022

tombh commented Jun 8, 2022

alcarney commented Jun 9, 2022

tombh commented Jun 9, 2022

alcarney commented Nov 12, 2021 •

edited

Loading

tombh commented Jun 6, 2022 •

edited

Loading

alcarney commented Jun 6, 2022 •

edited

Loading