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

Make a pyproxy of an awaitable python object an awaitable javascript object #1170

Merged
merged 53 commits into from Feb 14, 2021

Conversation

hoodmane
Copy link
Member

This PR together with #1158 together finish async/await support. I merged #1158 here even though the change sets are independent because some sort of event loop is necessary in order to test this PR.

This implements await on pyproxy whenever the pyproxy is awaitable. It uses _PyCoro_GetAwaitableIter to check whether the object is awaitable just like in the GET_AWAITABLE opcode. Normally in implementing the await keyword in Python, Python would next use the YIELD_FROM command, but this assumes that Python is yielding into an enclosing coroutine. Instead we use asyncio.ensure_future to schedule the awaitable if necessary and wrap in a future, then we create a promise and connect the promise handles to the future. This could be implemented rather more concisely in Python rather than C but the current bootstrapping scheme makes using Python code to implement PyProxy a bit annoying.

This mirrors the semantics of the underlying python awaitable, so if it is a coroutine then awaiting it multiple times will raise an error, if it is a future-like object then awaiting multiple times is fine.

The error formatting is a bit limited, these PythonError wrappers don't print particularly useful messages in Javascript by default. To improve this requires better javascript / python error integration, something like #1154.

@hoodmane hoodmane mentioned this pull request Feb 4, 2021
@hoodmane
Copy link
Member Author

hoodmane commented Feb 9, 2021

@rth @dalcde @phorward Would appreciate review on this one. It'd be nice to get this merged to finish the async/io support. After merging this, I think we should add a section to the usage docs explaining how asyncio works because discoverability is currently poor.

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.

Thanks @hoodmane ! A few minor comments below.

Also

- Switched from ̀Jedi to rlcompleter for completion in
`pyodide.console.InteractiveConsole` and so in `console.html`. This fixes
some completion issues (see
[#821](https://github.com/iodide-project/pyodide/issues/821) and
[#1160](https://github.com/iodide-project/pyodide/issues/821)
[#1160](https://github.com/iodide-project/pyodide/issues/1160)
Copy link
Member

Choose a reason for hiding this comment

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

I think we should re-organize it by submodule as we did for the last release as now it's getting a bit difficult to find things. No action needed in this PR.

src/core/pyproxy.c Show resolved Hide resolved
@hoodmane
Copy link
Member Author

@rth I think I addressed all the comments in your review.

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.

A minor formatting comment otherwise looks good. Thanks!

src/core/pyproxy.c Show resolved Hide resolved
src/core/pyproxy.c Show resolved Hide resolved
@rth rth merged commit 547753b into pyodide:master Feb 14, 2021
@hoodmane hoodmane deleted the await-pyproxy branch February 14, 2021 17:56
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

2 participants