Skip to content
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

Add simple web loop #1158

Merged
merged 24 commits into from Feb 5, 2021
Merged

Add simple web loop #1158

merged 24 commits into from Feb 5, 2021

Conversation

hoodmane
Copy link
Member

@hoodmane hoodmane commented Jan 21, 2021

This is a simple webloop aimed at enabling asyncio support as simply and painlessly as possible. It defers all work to the browser using setTimeout. Because the browser event loop has no lifecycle (well no lifecycle visible from javascript), there is no reason for us to have a lifecycle either, and all lifecycle methods are dummied out. I didn't implement call_exception_handler or it's friends at all because they are normally invoked during the teardown of an event loop, but we never do teardown.
Also, like in #958, run_forever and run_until_complete cannot block because we only have one thread.

I think this event loop will have better performance than the one in #958. It may well have worse compatibility, but I stole @oeway's test suite so any difference in compatibility is not caught by the tests. I'm not really sure what the tradeoffs involved are, there might be reason to have a simple event loop with better performance for normal purposes and a more complicated event loop with worse performance but with better compatibility.

Anyways I think this is a good starting point for now and if there is need for something more elaborate we can find out and deal with that down the road. Many thanks to @oeway for his work on this, I used a lot of his ideas here.

After we add an event loop, the only thing left that we need for asyncio support is to implement then on pyproxies for awaitable python objects, so that a pyproxy of a python awaitable is a javascript awaitable.

@hoodmane hoodmane force-pushed the simple-loop branch 4 times, most recently from d0240e6 to 1adf37e Compare January 21, 2021 01:30
@rth
Copy link
Member

rth commented Feb 1, 2021

I'm not really sure what the tradeoffs involved are, there might be reason to have a simple event loop with better performance for normal purposes and a more complicated event loop with worse performance but with better compatibility.

I also don't have a good understanding of the tradeoffs, but I would tend to go with this simpler solution first, if it's sufficient for most use cases.

Copy link
Contributor

@oeway oeway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I also like simpler solution!

Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks reasonable (from someone who hasn't worked much with asyncio). A few minor mostly docstring related comments below. Also please add a changelog entry.

src/pyodide-py/pyodide/__init__.py Outdated Show resolved Hide resolved
src/pyodide-py/pyodide/simple_web_loop.py Outdated Show resolved Hide resolved
src/pyodide-py/pyodide/simple_web_loop.py Outdated Show resolved Hide resolved
src/pyodide-py/pyodide/simple_web_loop.py Outdated Show resolved Hide resolved
src/pyodide-py/pyodide/simple_web_loop.py Outdated Show resolved Hide resolved
src/pyodide-py/pyodide/simple_web_loop.py Outdated Show resolved Hide resolved
src/pyodide-py/pyodide/simple_web_loop.py Outdated Show resolved Hide resolved
src/pyodide-py/pyodide/simple_web_loop.py Outdated Show resolved Hide resolved
src/pyodide-py/pyodide/simple_web_loop.py Outdated Show resolved Hide resolved
src/pyodide-py/pyodide/simple_web_loop.py Outdated Show resolved Hide resolved
hoodmane and others added 4 commits February 3, 2021 15:27
Co-authored-by: Roman Yurchak <rth.yurchak@gmail.com>
Co-authored-by: Roman Yurchak <rth.yurchak@gmail.com>
Co-authored-by: Roman Yurchak <rth.yurchak@gmail.com>
Co-authored-by: Roman Yurchak <rth.yurchak@gmail.com>
@rth
Copy link
Member

rth commented Feb 3, 2021

FYI, you can add all review suggestions in one commit/batch if you do it from the "Files changed" tab.

@hoodmane
Copy link
Member Author

hoodmane commented Feb 3, 2021

Thanks for the tip.

@hoodmane
Copy link
Member Author

hoodmane commented Feb 4, 2021

I also copied more docstrings from BaseEventLoop, as much as the original docstrings are applicable.

@rth
Copy link
Member

rth commented Feb 4, 2021

Also could you please add the added classes to https://github.com/iodide-project/pyodide/blob/master/docs/usage/api-reference.md and check that that the corresponding documentation is rendering fine?

@hoodmane
Copy link
Member Author

hoodmane commented Feb 4, 2021

Also could you please add the added classes to https://github.com/iodide-project/pyodide/blob/master/docs/usage/api-reference.md and check that that the corresponding documentation is rendering fine?

I guess it's rendering fine, but it doesn't look great. The autodoc adds a lot of methods from AbstractEventLoop that we don't implement. In general, the autodoc config could use a fair amount of work. I don't think there's anything appropriate to do about it in this PR though.

Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need a changelog entry

@hoodmane
Copy link
Member Author

hoodmane commented Feb 4, 2021

Oops, will add it.

@hoodmane
Copy link
Member Author

hoodmane commented Feb 4, 2021

I also added a separate entry for #880.

@hoodmane
Copy link
Member Author

hoodmane commented Feb 4, 2021

Seems like create_task behaves differently than ensure_future? I thought that when the argument to ensure_future was a coroutine that it would just call create_task...

@hoodmane
Copy link
Member Author

hoodmane commented Feb 4, 2021

So asyncio.create_task uses get_running_loop():

loop = events.get_running_loop() 
task = loop.create_task(coro)

whereas asyncio.ensure_future uses get_event_loop():

if coroutines.iscoroutine(coro_or_future):
    if loop is None:
        loop = events.get_event_loop()
    task = loop.create_task(coro_or_future)

The difference in behaviors occurs only when there is no currently running event loop. In that case, ensure_future uses the EventLoopPolicy to start an event loop automatically, whereas create_task throws an error. I suppose I should just automatically start the event loop during pyodide initialization and that'll make everything simpler.

@hoodmane
Copy link
Member Author

hoodmane commented Feb 5, 2021

Well automatically starting the event loop causes trouble with one CPython test. I guess I'll revert the create_task commits.

@rth
Copy link
Member

rth commented Feb 5, 2021

OK, thanks for investigating.

@rth rth merged commit a4fad9b into pyodide:master Feb 5, 2021
@hoodmane hoodmane deleted the simple-loop branch February 5, 2021 16:33
@mfripp
Copy link

mfripp commented Feb 9, 2021

I'm not sure if I should be commenting here or opening a new issue. But I think my question is pretty closely related to this, so I'll put it here.

I'm building a web page where students can edit Python code, then click "Run" to run it in pyodide. I started with all of this running in the main browser thread, which worked pretty well. As part of this, I added some glue code so that input() in Python became a prompt() in the browser.

However, when pyodide runs in the main thread, my code cannot update the display (e.g., show output), and controls are locked, so I can't have a "Stop" button to kill Python code that runs out of control. So I am working on a version where pyodide runs as a web worker. I've got that working for most of what I want, except for getting input back from the user.

Specifically, I send messages to from the worker to the main browser thread to launch the prompt, then send a message back from the main thread to the worker with the result. The problem is, I can't see any way to get the Python code to wait for the return message. I think that may be possible in general, if I write all-async code. The problem is, the incoming code is not written for an async environment (e.g., the students will use input(), not await input()). So even if I patch input(), my new version has to be a sync function to give a result back to the main code. So then my patched input() can't await an async function, e.g., a JavaScript promise that clears when the message comes back from the main browser thread.

I tried using asyncio.get_running_loop().run_until_complete() to await the promise, but this doesn't do anything, as noted above. I've investigated using emscripten's Asyncify framework, and I'm pretty sure I can get that to create a synchronous C function that will await a JavaScript promise. But I'm not sure if I can call the C function from Python in a way that is compatible with the Asyncify system. And even if I can, it will be pretty kludgy.

I'm now thinking about using shared memory between the browser thread and the worker, then running a busy loop in the worker that polls for a response via the shared memory (since I don't think sleep is possible). But I'd rather not use such a narrow solution and peg the processor that way.

So I just wanted to check, is there currently any path to get synchronous Python code to wait for a JavaScript promise or async function?

@hoodmane
Copy link
Member Author

hoodmane commented Feb 9, 2021

@mfripp I moved this over to #1219.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants