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

async <py-script> tags are blocking the execution flow #751

Closed
antocuni opened this issue Sep 6, 2022 · 21 comments · Fixed by #796
Closed

async <py-script> tags are blocking the execution flow #751

antocuni opened this issue Sep 6, 2022 · 21 comments · Fixed by #796

Comments

@antocuni
Copy link
Contributor

antocuni commented Sep 6, 2022

Consider the following case:

    def test_multiple_async(self):
        self.pyscript_run(
        """
        <py-script>
            import js
            import asyncio
            for i in range(3):
                js.console.log('A', i)
                await asyncio.sleep(0.1)
        </py-script>

        <py-script>
            import js
            import asyncio
            for i in range(3):
                js.console.log('B', i)
                await asyncio.sleep(0.1)
        </py-script>

        """)

I would expect it to print A and B lines alternatively, i.e.:

A 0
B 0
A 1
B 1
A 2
B 2

However, pyscript awaits the end of each code tag before commencing to execute the next, so the output is A0 A1 A2 B0 B1 B2. I think the culprit is the await in the following line:

loader?.log('Initializing scripts...');
for (const script of scriptsQueue_) {
await script.evaluate();
}
scriptsQueue.set([]);

My gut feeling is that pyscript should probably start all scripts together and then asyncio.wait() for all of them at the end (but we need to check how this interacts with the JS event loop).
But also, I wonder whether this has any unintended and/or surprising consequences.

Related issues:

@JeffersGlass
Copy link
Member

I stumbled across this issue the other day, but convinced myself it was an issue with my environment. Glad to see I wasn't loosing my marbles. Especially since this issue isn't present in the 2022.06.1 release.

I believe you are correct in identifying source of the issue - doing a git bisect, the issue starts in PR #598, when that same line of code was added to what was then pyconfig.ts:

loader?.log('Initializing scripts...');
if (mode_ == 'play') {
    for (const script of scriptsQueue_) {
-        script.evaluate();
+        await script.evaluate();
    }
    scriptsQueue.set([]);
}

I believe this is simply a syntax issue - we should be using for await ... of instead of await ...

...
for await (const script of scriptsQueue_) {
    script.evaluate();
}
...

This gives what I think is the correct behavior: Python with top-level-await does not block, but synchronous Python code does.

That said, maybe that's not the desired behavior in PyScript. For example, if your example was preceded by another <py-script> tag with for _ in range(100_000_000): x = 1, the async code wouldn't begin running until the loop had finished. I think the "all code runs top to bottom of the page" is best, and it matches how <script> tags behave, but I could see the argument for "all asynchronous code is started, then synchronous code runs top to bottom" or something.

@antocuni
Copy link
Contributor Author

antocuni commented Sep 7, 2022

I believe you are correct in identifying source of the issue - doing a git bisect, the issue starts in PR #598, when that same line of code was added to what was then pyconfig.ts:

loader?.log('Initializing scripts...');
if (mode_ == 'play') {
    for (const script of scriptsQueue_) {
-        script.evaluate();
+        await script.evaluate();
    }
    scriptsQueue.set([]);
}

woooh, thanks for identifying the issue! It's ironic that the bug was introduced by PR which says "minor changes" 😂.
(it's also a good data point for my endless and lost battle against windmills linters)

I believe this is simply a syntax issue - we should be using for await ... of instead of await ...

...
for await (const script of scriptsQueue_) {
    script.evaluate();
}
...

This gives what I think is the correct behavior: Python with top-level-await does not block, but synchronous Python code does.

If I understand it correctly, it would start all async blocks together, and then wait until they all complete before moving on, is it correct?
This is already a big improvement over the current situation, although it would not solve e.g. #678, because in that case the problem is that the async block is an infinite loop.

That said, maybe that's not the desired behavior in PyScript. For example, if your example was preceded by another <py-script> tag with for _ in range(100_000_000): x = 1, the async code wouldn't begin running until the loop had finished. I think the "all code runs top to bottom of the page" is best, and it matches how <script> tags behave, but I could see the argument for "all asynchronous code is started, then synchronous code runs top to bottom" or something.

+1 for matching the behavior of <script> when possible.

@JeffersGlass
Copy link
Member

I may actually have made it more complicated than it needs to be. I think we can do:

for (const script of scriptsQueue_) {
    script.evaluate();	

Because evaluate() awaits runtime.run() (and for the Pyodide runtime, runPythonAsync() within it), we can iterate over this loop synchronously to "schedule" the execution of running the various scripts.

Unfortunately, this breaks any integration test which relies on pyscript_run() to run a <py-script> tag [1]. pyscript_run() (and wait_for_pyscript) assume that seeing the sentinel value "PyScript page fully initialized" in the console log means all the scripts have finished evaluating, but this isn't necessarily true since we're no longer awaiting their evaluation in the loop above. So those tests fail intermittently.

For the sake of the tests, I think may need a better method of detecting when all py-script tags have finished evaluating. There was an example in the Nucleus PyScript forum, but I think it assumes all scripts are synchronous.

[1] Hello, execution in order, angle brackets, nonblocking, runtime config, hello_world

@sumahadevan
Copy link
Contributor

@antocuni @JeffersGlass I was following your discussion regarding the py-script tags closely, and have now verified that the correction above in file runtime.ts (that is, there should be no 'await' before 'script.evaluate') is indeed correct, and fixes BOTH the above issue AND issue #678 (the fact that is fixes #678 may be a little surprising):

  1. I checked that making this one-line correction actually FIXES the issue The WebGL example never finishes loading #678 (The WebGL example never finished loading), despite the fact that the Python code there involves an infinite loop. Actually, @antocuni has also mentioned this very line of code (await) there as a likely reason for the issue. As also mentioned above, this was working earlier (for example it works in the version on the official site pyscript.net/examples).

The reason also became clearly after reading up a little bit on Mozilla Developer Network (MDN) - the await WILL BLOCK all following code in the function is it used in, but WILL NOT BLOCK the calling function one level above. So putting an await here means that all following scripts in the same loop are blocked (which we dont want), while having the await at a much lower level (for example: await runtime.run) means that the following scripts in the loop, AND higher-level calling functions, can execute. In particular, all the messages on screen are displayed AND cleared without getting blocked (the infinite loop in the WebGL Python code is reeally no different from the infinite loops on a Canvas element in HTML games, and whatever happens code executes before this loop actually starts will complete successfully).

  1. I have also made some simple examples (based on that of @antocuni above), both for tags and regular Javascript <script> tags to compare the behavior of the two during async execution. The good news is that for the tags, with basically the same code as above, the console.log output is exactly as desired above, namely:

A 0
B 0
A 1
B 1
A 2
B 2

This is because the line "await asyncio.sleep(0.1)" functions EXACTLY like a blocking await used in a Javascript loop.

  1. I have successfully modeled the same code in Javascript for both a non-blocking await (the await is one level below the loop), and a blocking await (the await is directly IN the loop). I am attaching both the Javascript files for reference (after extracting them from the zips, they can simply be double-clicked and the console.log verified). The code used in them is based on the following MDN link:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function

async_await_nonblocking_javascript_twice.zip
async_await_blockingloop_javascript_twice.zip

@sumahadevan
Copy link
Contributor

I have made a set of Pyscript examples to compare the behavior of Python's asyncio's async/await with that of Javascript's async/await. The behavior of Python's version of async/await is generally similar to that of Javascript's - namely, where you put the await determines whether it will block the loop or not. There is however one key difference in Python/Pyscript's behavior, as shown in example 4.

Firstly, to see the behavior of these Pyscript examples correctly, the most important thing is that the correction pointed out by @JeffersGlass (remove the await before script.evaluate() in runtime.ts) HAS to be made (which I have done locally).

Secondly, the examples are basically just variations of the example given by @antocuni above, but within HTML files that can just be placed in the same location as other local Pyscript examples (that is, in pyscript/pyscriptjs/examples) and the web pages viewed either on localhost (through npm run dev), or just by double-clicking on them (possible on both Chrome and Firefox for these simple pages). There are four examples:

  1. async_await_toplevel_blockingloop_pyscript_twice : This file is basically just the original example from @antocuni of two blocking asyncio await loops at the top level. It produces the expected output indicated by him, namely:
    A 0
    B 0
    A 1
    B 1
    A 2
    B 2

  2. async_await_non_blocking_pyscript_twice : This file is a modification of the above example so that the blocking asyncio call is in functions (asyncCall1 and asyncCall2) one level below the loop. In this case, the top-level code does NOT block and the output is sequential:
    A 0
    A 1
    A 2
    B 0
    B 1
    B 2

Pyodide does tell us that - RuntimeWarning: coroutine 'asyncCall1' was never awaited - and similarly for asyncCall2.

  1. async_await_invoked_blockingloop_pyscript_twice - This is a variation of the blocking loop 1) above with the loop code moved to called functions (asyncCallLoop1 and asyncCallLoop2) for better comparision with 2). The output is the same as 1), namely each of the called loops (asyncCallLoop1 and asyncCallLoop2) block and produce output in alternation. Interestingly, the above warning from Pyodide is not given in this case (although here also we do not await the async coroutines at top level!).

  2. async_await_mixed_blockingloop_pyscript_twice - This is a VERY surprising case of mixing two blocking await loops with code as in 1) and 3). The first loop that outputs 'A' is in in a called routine asyncCallLoop1 (as in 3)), while the second loop that outputs 'B' is a top-level loop. The following is the output:
    B 0
    A 0
    B 1
    A 1
    B 2
    A 2

Surprisingly, B0 is output before A0 (and consequently B2 before A2, and B3 before A3)! What this tells us is that in Python's asyncio, if we dont await an async coroutine, it doesn't even wait for the coroutine to reach the asyncio.await call inside it, it just goes ahead with the code (second loop) that follows the call to the coroutine (first loop). For comparison, Javascript will only hand back control like this when actually executing the await.

I am attaching the above four Pyscript asyncio examples (HTML files) as a single zip, and thought they might be helpful to illustrate Pyscript's behavior in the different cases (and did learn a lot about Python's asyncio in this process, not having used it before).

async_await_pyscript_twice_examples.zip

The outputs in the various cases can then be seen in the console log.

@JeffersGlass
Copy link
Member

@sumahadevan This is great work! And really quite interesting as a set of use cases and test cases.

I do wonder if part of the behavior we're seeing in that last example (async_await_mixed_blockingloop_pyscript_twice) is perhaps something that's unique to the Pyodide environment, and not to Python more generally. Maybe? Since the Pyodide runtime has a custom webloop constantly running (which asyncio uses), and I can't off the top of my head think of the equivalent in a pure Python environment. But I think you're deeper in the weeds on this than I am, if you have thoughts?

That said, it does seem that this behavior is at least consistent between PyScript (with the modification you mention) and Pyodide, as the following code (which runs Python in Pyodide directly) gives the same output as your example 4:

//Tested with Pyodide 0.21.2
async function main() {
    var pyodide = await loadPyodide();

    pyodide.runPythonAsync(`
        import js
        import asyncio

        async def asyncCallLoop1():
            for i in range(3):
                js.console.log('A', i)
                await asyncio.sleep(2)

        asyncCallLoop1()
        `);

    pyodide.runPythonAsync(`
        import js
        import asyncio

        for i in range(3):
            js.console.log('B', i)
            await asyncio.sleep(2)
        `);
    
    };
main();

Output:

B 0
A 0
B 1
A 1
B 2
A 2

@sumahadevan
Copy link
Contributor

@JeffersGlass I finally got round to checking the corresponding async/await behavior of asyncio in Python itself, and I need to make some important corrections to what I had said earlier regarding the four Pyscript examples.

  1. Firstly, it is good that you were able to replicate the same log in Pyodide, and we would expect this as Pyscript uses Pyodide internally (and I also set up a HTML file with your Pyodide code and verified the same).

  2. Secondly, and MOST IMPORTANT, @antocuni is 100% correct in expecting ONLY ONE CORRECT BEHAVIOR from ALL the examples, which is the one he has indicated and was produced by the first of my examples above:
    A 0
    B 0
    A 1
    B 1
    A 2
    B 2

  3. The incorrect behavior in my examples 2) and 4), and in your example as well, is because Pyodide DOES NOT ensure that the caller of an async function does an await (I found this out by looking at the behavior in Python itself). In many cases Pyodide does not even warn that the async function has not been awaited. The moment I systematically put the missing await at the top level (in all the calls to async functions), all four examples produced the same result (as shown in 2) just above. This was ALSO true for your Pyodide example, once I added the missing top-level await.

  4. When running (essentially) the same code in Python, I found that Python will not only ALWAYS warn if a call to an async function is not awaited, but it will also NOT RUN that particular async function (so if async code for the A's is not awaited, no A's will be output!). Once you put the awaits in right up to the top level, both the cases (interleaved as above, and sequential) can be generated as follows in Python.

  5. To produce the equivalent output of TWO separate py-script tags in Python asyncio, we have to use 'gather' (which semantically corresponds to two independent coroutines that can run parallelly). The code for this is as below:

`

#!/usr/bin/env python3

async_await_non_blockingloop_python_twice.py

import asyncio

async def asyncCallLoop1():
for i in range(3):
print("A", i)
await asyncio.sleep(2)

async def asyncScript1():
print("PYTHON - FIRST ASYNC WITH INVOKED LOOP BLOCKING AWAIT AT SAME LEVEL AS LOOP")

await asyncCallLoop1()

async def asyncScript2():
print("PYTHON - SECOND ASYNC WITH TOP-LEVEL LOOP BLOCKING AWAIT AT SAME LEVEL AS LOOP")

for i in range(3):
print("B", i)
await asyncio.sleep(2)

async def main():

await asyncScript1()

await asyncScript2()

await asyncio.gather(asyncScript1(), asyncScript2())

asyncio.run(main())

`

It doesn't matter how the code is structured, as long as we use 'gather' this will produce interleaved output exactly as in 2) above.

  1. To produce sequential output (all A's followed by all B's), simply comment out the 'gather' call in 5) above and uncomment the separate sequential calls to the first and second async coroutines.

In sum, Pyodide (and hence Pyscript) behaves differently and is more lax is flagging missing awaits in asyncio async/await code, as compared to standard Python. This could be considered as a possible issue (in the semantics of Pyodide's implementation of the Python interpreter).

@sumahadevan
Copy link
Contributor

The lines in bold in the Python program above were actually Python comments - though I guess you would have figured that out.

@sumahadevan
Copy link
Contributor

Also, the Python indentation has been lost (though the code is straightforward).

@JeffersGlass
Copy link
Member

JeffersGlass commented Sep 26, 2022

Since PyScript 2022.09.1 has reached release candidate 1, I think we should open a small PR that makes the small change in runtime.ts that we've been discussing.

loader?.log('Initializing scripts...');
if (mode_ == 'play') {
    for (const script of scriptsQueue_) {
+        script.evaluate();
-        await script.evaluate();
    }
    scriptsQueue.set([]);
}

Clearly, we have some more work to do here to come to a fully correct solution, but without this change top-level-await scripts in PyScript 2022.09.1 do not work at all, which would be a significant regression from 2022.06.1. I think we should make the change for 2022.06.1 and continue working on improvements from there.

Thoughts?

@sumahadevan
Copy link
Contributor

I would agree that making this change would at least partially fix the problem with the top-level await scripts, and I just saw that a 2022.09 release is out for testing, so getting this into the release would be good.

@JeffersGlass would you be doing the PR (or do you want me to do it)?

@antocuni your view on making this change? After this change, the two async py-script tags do correctly produce their output in alternation as was initially expected by you.

@tedpatrick
Copy link
Contributor

tedpatrick commented Sep 27, 2022

I am testing an approach using Promise.all() like so:

const scriptsPromises = []
for (const script of scriptsQueue_) {
     scriptsPromises.push( script.evaluate() );
}
await Promise.all(scriptsPromises);
scriptsQueue.set([]);

Locally, all tests pass and all examples work including the case provided by @antocuni and @sumahadevan

Branch: https://github.com/pyscript/pyscript/tree/promise-all-to-enable-root-async

Please test the changes locally and post feedback. Thanks.

@JeffersGlass
Copy link
Member

JeffersGlass commented Sep 27, 2022

@tedpatrick One hangup there is that async Python scripts that never terminate don't allow PyScript to finish initializing nor the loader to close:

<py-script>
  import asyncio
  from itertools import count
  for i in count():
      print(f"Count: {i}")
      await asyncio.sleep(1)
</py-script>

Removing the await before does allow the loader to complete, though haven't tested that idea thoroughly at all...

My thinking is: script.evaluate() returns a promise, we collect those promises into an array and await them all as a single promise... but do we really need to? I don't think we do anything with the values the promises of script.evaluate() resolve to... So there's not much point awaiting their resolution, in my way of thinking, either singly or in aggregate.

@tedpatrick
Copy link
Contributor

I tested out this solution using void. This properly handles the Promise returned from script.evaluate().

for (const script of scriptsQueue_) {
     void script.evaluate();
}

@sumahadevan
Copy link
Contributor

@tedpatrick will check this locally on my machine - the link to your branch seems broken (though I can make the same changes myself).

I wanted to add a small wrinkle to the above discussion. In base,ts, the routine "async evaluate()" returns a void promise (but a PROMISE nonetheless). The issue is that in runtime.ts, this is called DIRECTLY from the loop and awaited there. Let us suppose there are good reasons to AWAIT the async routine evaluate() in order to do something after that. How should we do this? The correct solution is to define an async routine, say "HandleScriptEvaluate(script)", that internally calls the async routine evaluate() and awaits its completion (in order to do something (say log that it is done). This intermediate routine is called from the loop in runtime,ts, which doesnt await anything at all. Doing this is beneficial due to the way await works in Javascript (affects the awaiting routine but not those calling it).

My main point is that we can still usefully await the promise returned by "async evaluate", but do it one level lower than the loop in a separate routine (thus having our cake and eating it too). Of course, I still have to check that this does work as expected.

One more thing that I will check is whether your solution resolves #678 - in that case there is only a single Python script that loops infinitely, so awaiting it causes later Javascript code that clears messages on the web page to never get reached/executed (if one doesnt await at all then of course the Javascript code executes).

@tedpatrick
Copy link
Contributor

I believe we have 2 conflicting issues.

  1. Script evaluation order is off
  2. When scripts contain top-level async code, they never return and do not hide the loader.

I believe we need to await all calls to script.evaluate() but I believe we need to do this within a Promise.all like so:

 const scriptPromises = [];
for (const script of scriptsQueue_) {
    scriptPromises.push( script.evaluate() );
}
await Promise.all(scriptPromises);
scriptsQueue.set([]);

This results in very accurate loading behavior on all but 2 examples: Endless loop and root async await. These 2 examples are related 100% to the loader closing but there is nothing wrong with them logically. This example was provided by @JeffersGlass

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="utf-8" />
    <meta name="viewport" content="width=device-width,initial-scale=1" />
    <title>Async Await BLOCKING LOOP Pyscript Twice</title>
    <link rel="stylesheet" href="https://pyscript.net/latest/pyscript.css" />
    <script defer src="https://pyscript.net/latest/pyscript.js"></script>
</head>
<body>
    <style>
       py-loader {
        display:none
       }
    </style>
    <py-script>
        import asyncio
        from itertools import count
        for i in count():
            print(f"Count: {i}")
            await asyncio.sleep(1)
    </py-script>
</body>
</html>

The other example is the webgl/raycaster. Both work perfectly with py-loader set to display:none

The py-loader can be easily hidden as follows using css.

<style>py-loader{ display:none; }</style>

This is not ideal either but downstream we can make the loader event aware so endless loops or root await that never return can be handled with an api.

# futures
emit( "py-loader.close" ) # this global will emit an event to close the py-loader anywhere within an application.

I want to support the endless looping use case but we are not yet ready with a programmatic loader for these cases. Ideally, we can get this into the 2022.10.1 release, and better support these use cases.

A PR for Review: #796

@JeffersGlass
Copy link
Member

My main point is that we can still usefully await the promise returned by "async evaluate", but do it one level lower than the loop in a separate routine (thus having our cake and eating it too). Of course, I still have to check that this does work as expected.

@sumahadevan I would love to see a proof-of-concept of this, as I don't think I fully understand. But I'd love to play with it.

I would also point out, as you continue your investigations (thank you again for your diligent and detailed work!) that asyncio in Pyodide doesn't necessarily work exactly as it does on desktop, because Pyodide uses its own event loop ("Webloop") in place of an OS-provided one that a desktop would use.

So it's probably worth distinguishing behaviors of "Python" vs. "Python inside Pyodide" for maximum clarity, as they aren't necessarily the same.

@tedpatrick
Copy link
Contributor

Some whys...

Why do we await here? await script.evaluate();
A: Because we need to know when script execution is done.

Why do we need to know it is done? Some scripts are never done.
A: Because that is how the lifecycle works and <py-loader> is dependent on it.

Why is <py-loader> dependent on it?
A: This provides consensus to when to automatically close the loader

At the root, the loader is the problem. We are waiting to remove the loader at a more appropriate time when scripts have completed execution vs when scripts have started. Although the loader being tied to the script(s) completion is a closer approximation of loader accuracy, it causes problems for scripts that never end.

In this light, we should void script.evaluate(); and rework <py-loader> in a later release. A user should be able to deeply customize the <py-loader> or set it to manual and emit an event to close it.

Also key is @JeffersGlass point that the downstream logic never is executed if we await and lifecycle is pretty important to get right.

Let's ship void script.evaluate() in 2022.09.1. This solves the majority of this issue and allows us to enhance <py-loader> in time. This will result in a <py-loader> closing early as we are tied to script evaluation starting rather than when script evaluation completes.

@tedpatrick
Copy link
Contributor

PR is updated to void script.evaluate(); #796

@sumahadevan
Copy link
Contributor

@tedpatrick your explanation of why we need to await in the code (for processing all scripts) is very comprehensive, and it also clarifies the design of the code (and also highlights the important point that the loader issues need to be resolved separately and not by dropping the await). This definitely looks like the best possible solution (leaving aside the loader issues for a separate resolution).

@JeffersGlass I will do a small POC of the code restructuring I was suggesting - as emphasized above, this code will STILL AWAIT, but I was thinking that a wrapper function around script.evaluate() might be helpful (will try this minor change out, but only once the above PR has been merged - the main purpose of the wrapper would be to output messages about the status of the script being evaluated).

And yes, in any further investigation of Pyodide/Python asyncio scripts, I will definitely keep in mind the differing behavior of the Pyodide asyncio vs Python asyncio (to further confuse me, they both behave quite differently from Javascript's async/await which is what I have been used to)!

@antocuni
Copy link
Contributor Author

antocuni commented Sep 28, 2022

I want to support the endless looping use case but we are not yet ready with a programmatic loader for these cases. Ideally, we can get this into the 2022.10.1 release, and better support these use cases.

I'm not 100% sure of this one. Do we really want to support "implicit endless loops"? They look very weird to my python eyes.
One possible alternative is the following:

  • we forbid top-level await
  • we provide a function run_soon (better name needed)

In this way, the endless loop pattern becomes something like this:

<py-script>
import itertools, asyncio
async def my_loop():
    for i in itertools.count(0):
        print(i)
        await asyncio.sleep(1)

run_soon(my_loop)
</py-script>

Advantages: the top-level scripts are always synchronous, their execution order is easy to predict and we have a clear separation between "preparation logic" (done at top level) and "event-driven logic" (which is the endless part).
Bonus point: it become easier to move your code from <py-script> to .py files.

Why do we need to know it is done? Some scripts are never done. A: Because that is how the lifecycle works and <py-loader> is dependent on it.

Why is <py-loader> dependent on it? A: This provides consensus to when to automatically close the loader

At the root, the loader is the problem.

Assuming that we want to allow endless top-level loop (see above), I agree.

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

Successfully merging a pull request may close this issue.

4 participants