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

Remove 'Implicit Async', Don't Await runtime.run() #928

Merged
merged 17 commits into from Nov 16, 2022

Conversation

JeffersGlass
Copy link
Member

@JeffersGlass JeffersGlass commented Nov 9, 2022

This removes the ability to write "implicitly scheduled coroutine" code in <py-script> tags, and requires users to use known asyncio methods to run coroutines, like ensure_future() or create_task(). It reverts from the use of Pyodide's runPythonAsync() to runPython().

Fixes #879. See discussion in that issue.

This PR has a completed todo list and is ready for review.

  • Primary code changes
  • Run typescript, py-unit tests
  • Fix async integration tests
  • Adjust examples to remove implicit coroutine scheduling
    • bokeh_interactive
    • numpy_canvas_fractals
    • say_hello (?)
    • webgl/raycaster
  • Add page to docs explaining deprecation and new strategies, add link in UserError

To be clear, users still absolutely still have the ability to run code asynchronously. All of these still work:

<!-- Basic coroutine running using asyncio.ensure_future() -->
<py-script>
import asyncio

async def main():
    display("I see you shiver with anticip", target="rocky-horror-quote")
    asyncio.sleep(5)
    display("...pation", target="rocky-horror-quote")

asyncio.ensure_future(main())
</py-script>
<!-- Running a coroutine forever -->
<py-script>
from js import console
import asyncio
from datetime import datetime

async def clock():
  while True:
    display(datetime.now().strftime("%m/%d/%Y, %H:%M:%S"), target="clock-output", append=False)
    await asyncio.sleep(1)

asyncio.ensure_future(clock())
console.log("This is logged right away, even though the clock() coroutine runs forever")
</py-script>
<!-- Using asyncio.gather() to run many coroutines -->
<py-script>
import asyncio
import random

async def individual_counter(label, delay):
  local_count = 0
  while local_count <= 10:
    await asyncio.sleep(delay)
    console.log(f"{label}-{local_count}")
    local_count += 1

  console.log(f"{label} finished!")

labels = ["A", "B", "C", "D"]
asyncio.gather(*[individual_counter(label, random.random() + 1) for label in labels])
</py-script>

What this PR removes is the old "top-level await, implicit coroutine scheduling" style permitted by runPythonAsync:

<py-script>
import asyncio
display("First")
await asyncio.sleep(1)
display("second")
</py-script>

The above code now fails, and shows a UserError explaining why this code is no longer permitted:
image

@antocuni
Copy link
Contributor

antocuni commented Nov 9, 2022

woooo 🎉🎉🎉

@JeffersGlass JeffersGlass added the status: WIP PR that is not yet ready for review label Nov 9, 2022
@JeffersGlass
Copy link
Member Author

JeffersGlass commented Nov 9, 2022

Interesting error in test_py_config: "DeprecationWarning: There is no current event loop'" shows up in the console, causing an assert to fail.

Some googling suggests this is related to deprecation of "asyncio.get_event_loop()". Will investigate.
pytest asyncio issue
Docs: asyncio.get_event_loop

@JeffersGlass
Copy link
Member Author

I added docs explaining the deprecation of implicit coroutine scheduling; the added Asyncio/Await page in the docs could use some fleshing out, but I think that should be a separate PR.

@JeffersGlass JeffersGlass marked this pull request as ready for review November 10, 2022 19:54
@JeffersGlass JeffersGlass added status: ready PR that is ready for review and removed status: WIP PR that is not yet ready for review labels Nov 10, 2022
@marimeireles
Copy link
Member

Hey @JeffersGlass just to confirm. This is not a WIP anymore, right?
Maybe you can morph it into a "real" PR?

@JeffersGlass JeffersGlass changed the title WIP: Remove 'Implicit Async', Don't Await runtime.run() Remove 'Implicit Async', Don't Await runtime.run() Nov 11, 2022
@JeffersGlass
Copy link
Member Author

JeffersGlass commented Nov 11, 2022

This is not a WIP anymore, right?

Whoops! You're correct - I've removed "WIP". Thanks!

I didn't see that pre-commit.ci is failing here (though passing locally) - let me see if I can rectify that.

Docs build upload is failing too... but that's just the upload, not the build, which I think is a known issue of AWS permissions not being present for other-users' branches?

@madhur-tandon
Copy link
Member

Docs build upload is failing too... but that's just the upload, not the build, which I think is a known issue of AWS permissions not being present for other-users' branches?

Yes the AWS issue is known and can be ignored for now.

Copy link
Member

@marimeireles marimeireles left a comment

Choose a reason for hiding this comment

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

I'm a +1 thank you for your work here, this is great improvement! :)

Copy link
Member

@madhur-tandon madhur-tandon left a comment

Choose a reason for hiding this comment

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

Great PR!

@JeffersGlass
Copy link
Member Author

JeffersGlass commented Nov 12, 2022

Thanks y'all! I do want to give @antocuni a chance to weigh in here before I hit merge, since he's been deep into the lifecycle (and the issues await runtime.run() has been causing there).

Copy link
Contributor

@antocuni antocuni left a comment

Choose a reason for hiding this comment

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

Very good and solid PR, I love it!
Goodbye implicit async ☠️ 👋 🔥
Left some minor comments, but I'm approving the PR already.

pyscriptjs/src/pyodide.ts Show resolved Hide resolved
pyscriptjs/src/python/pyscript.py Show resolved Hide resolved
docs/guides/asyncio.md Show resolved Hide resolved
pyscriptjs/tests/integration/test_py_repl.py Show resolved Hide resolved
@JeffersGlass
Copy link
Member Author

🚀

This breaks the brand new test_importmap from #938 in exactly the way explained there - because we await fetching importmaps (but with this PR no longer await runtime.run) not all importmaps may be loaded before run execute Python scripts. After talking with @antocuni, I've marked that test xfail for now.

@JeffersGlass JeffersGlass merged commit 0b23310 into pyscript:main Nov 16, 2022
JeffersGlass added a commit that referenced this pull request Nov 16, 2022
*Cleanup several spots where runtime.run() no longer needs to be awaited, now that #928 is merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready PR that is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PyodideRuntime.run should not await
5 participants