Loading and void script.evaluate() #796

tedpatrick · 2022-09-27T22:12:26Z

This PR is intended to fix #751

I believe we have 2 conflicting issues.

Script evaluation order is off
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 - WARNING this is a proposed API to emit events can support py-loader closing programmatically.
emit( "py-loader.close" ) # this global will emit an event to close the py-loader

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.

for more information, see https://pre-commit.ci

JeffersGlass · 2022-09-28T01:27:14Z

Thank you for adding all of the async examples! I think this will make coordinated testing on this issue much cleaner.

That said: The issue (2) is not specifically that the loader never closes - it's that, as long as we're awaiting script evaluation, nothing which follows "await Promise.all...." in initalize runs until all scripts terminate.

For example, try the following with this PR:

<!-- Endless Loop with py-repl and py-onEvent -->
<body>
  <style>
    py-loader {
     display:none
    }
 </style>
  <py-repl>x = range(5)</py-repl>
  <button id="b" py-onclick="import js; js.console.log("Logged!")></button>
  <py-script>
    import asyncio
    for i in range(10):
      print(f"A{i}")
      await asyncio.sleep(1)
  </py-script>
</body>

The py-repl custom element isn't created until after the async script terminates, because createCustomElements() is only called after all scripts terminate.
Similarly, tags with py-on* events are initialized till that later point, because they are created with a postinitializer (initHandlers in pyscript.ts)

I don't believe we can directly await script.evaluate() (either per-script or via await Promise.all...), or we block execution of initialize() until all scripts terminate. Which seems like poor behavior.

We also haven't fully solved the issue (1) of evaluation order here - await2.html (at least for me) still outputs "B0 A0 B1 A1 B2 A2". I believe the execution-order issue is related to @sumahadevan's research in Issue 751 (point 4). It's possible that this is related to how Pyodide specifically handles asyncio, which is different from asyncio on desktop as it uses a custom event loop.

I would propose an adjustment, based on your note in #751, to something you suggested:

    #runtime.ts
    loader?.log('Initializing scripts...');
    const scriptPromises = [];
    for (const script of scriptsQueue_) {
        scriptPromises.push( script.evaluate() );
    }
-   await Promise.all(scriptPromises);
+   void Promise.all(scriptPromises); 
    scriptsQueue.set([]);

This allows the both the endless-loop-with-repl-and-button example above to function correctly, as well as the WebGL example (per #678) . All integration tests continue to pass.

It does not solve the out-of-order-execution in await2.html, but as above, I believe this is a separate issue.

tedpatrick · 2022-09-28T11:08:23Z

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 on when to automatically close the loader

At the root, the <py-loader> is the problem. We are waiting to remove the <py-loader> at a more appropriate time when scripts have completed execution vs when scripts have started. Although the <py-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.

marimeireles · 2022-09-28T12:49:20Z

Wow, cool work here!
I'd recommend to have these examples as tests. What do you all think?

sumahadevan · 2022-09-28T13:00:24Z

Wow, cool work here! I'd recommend to have these examples as tests. What do you all think?

Yes. I think they would highlight various edge cases.

tedpatrick · 2022-09-28T13:33:47Z

I would like to get this into the 2022.09.1 release. Let's plan to add more tests in time but as I see it, this looks and feels ready. I have tested EVERY single example we have and things look very good with a slight issue in /examples/async/async2.html due to Pyodide webloop differences with CPython.

tedpatrick · 2022-09-28T13:49:04Z

Ok it looks like the change from:
await script.evaluate() to void script.evaluate()

Breaks this example using Toga. http://localhost:8080/toga/freedom.html

tedpatrick · 2022-09-28T14:18:57Z

It feels like the regression in /examples/await/await2.html and /examples/toga/freedom.html represent differences in Pyodide's webloop and CPython. Ideally breaking these in a release would allow us to more deeply understand things here yet enable a larger class of working apps.

JeffersGlass · 2022-09-28T14:52:29Z

@tedpatrick Makes sense to me! I think these small breaks are worth it to maintain top-level-await not generally blocking initalization, and we can continue to dig deeper into the most correct behavior.

For what it's worth, we've discussed moving the toga eaxmple to the Collective since it's based on a fairly heavily customized version of Toga, and uses some pre-built wheels of that customized version that rely other older behaviors in 2022.06.1. So I'm not entirely surprised that it's broken.

antocuni

+0.5 to merge this PR.
I think that in general we need to take a step back and think about what are the async features we want to offer, the patterns we want to support and the subtle interactions between the JS and Python event loops.
But this PR partially solves some of the problems so it's surely a step forward.

That said, it definitely lacks tests.
I am with @marimeireles here: instead of having example/await*.html, we should have test/integration/test_await.py which contains all the relevant cases we want to test. E.g.:

    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>
        """
        )
        assert self.console.log.lines == [
            'A 0',
            'B 0',
            'A 1',
            'B 1',
            'A 2',
            'B 2'
        ]

pyscriptjs/src/runtime.ts

tedpatrick · 2022-09-28T16:34:37Z

Given the timeframe to getting 2022.09.1 released, I would like to defer adding new async tests given how we test is looking to change after this release.

JeffersGlass · 2022-09-28T17:24:10Z

LGTM! Is it worth adding a comment/tag to the examples of what their expected output should be, or what it is as of this PR? We have the context captured here and in #751 , but wondering if that would be useful direclty within the await*.html examples.

for more information, see https://pre-commit.ci

tedpatrick · 2022-09-28T18:37:16Z

Added a test. Also discovered the benefits of self.wait_for_console("async tadone")

antocuni

LGTM, thank you!

tedpatrick · 2022-09-28T22:39:14Z

Once this builds. I am deploying as snapshot as 2022.09.1.RC2

Ted Patrick and others added 2 commits September 27, 2022 17:08

Loading and Script evaluate with Promise.all

3f880ff

[pre-commit.ci] auto fixes from pre-commit.com hooks

a77bf58

for more information, see https://pre-commit.ci

tedpatrick requested review from antocuni and fpliger September 27, 2022 22:12

tedpatrick mentioned this pull request Sep 27, 2022

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

Closed

Ted Patrick added 2 commits September 28, 2022 05:50

Move to 'void script.evaluate();'

3cefe41

chmod files to 644

714ffc1

tedpatrick changed the title ~~Loading and Script evaluate with Promise.all~~ Loading and void script.evaluate() Sep 28, 2022

tedpatrick requested a review from JeffersGlass September 28, 2022 14:58

antocuni requested changes Sep 28, 2022

View reviewed changes

pyscriptjs/src/runtime.ts Show resolved Hide resolved

Ted Patrick and others added 2 commits September 28, 2022 13:34

Add a test

c983b6d

[pre-commit.ci] auto fixes from pre-commit.com hooks

4051efa

for more information, see https://pre-commit.ci

antocuni approved these changes Sep 28, 2022

View reviewed changes

tedpatrick merged commit 7d5f6c9 into main Sep 28, 2022

tedpatrick deleted the loading-and-script-evaluate branch September 28, 2022 22:38

tedpatrick mentioned this pull request Oct 6, 2022

The WebGL example never finishes loading #678

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loading and void script.evaluate() #796

Loading and void script.evaluate() #796

tedpatrick commented Sep 27, 2022

JeffersGlass commented Sep 28, 2022 •

edited

Loading

tedpatrick commented Sep 28, 2022 •

edited

Loading

marimeireles commented Sep 28, 2022

sumahadevan commented Sep 28, 2022

tedpatrick commented Sep 28, 2022

tedpatrick commented Sep 28, 2022 •

edited

Loading

tedpatrick commented Sep 28, 2022

JeffersGlass commented Sep 28, 2022

antocuni left a comment •

edited

Loading

tedpatrick commented Sep 28, 2022

JeffersGlass commented Sep 28, 2022

tedpatrick commented Sep 28, 2022

antocuni left a comment

tedpatrick commented Sep 28, 2022

Loading and void script.evaluate() #796

Loading and void script.evaluate() #796

Conversation

tedpatrick commented Sep 27, 2022

JeffersGlass commented Sep 28, 2022 • edited Loading

tedpatrick commented Sep 28, 2022 • edited Loading

marimeireles commented Sep 28, 2022

sumahadevan commented Sep 28, 2022

tedpatrick commented Sep 28, 2022

tedpatrick commented Sep 28, 2022 • edited Loading

tedpatrick commented Sep 28, 2022

JeffersGlass commented Sep 28, 2022

antocuni left a comment • edited Loading

Choose a reason for hiding this comment

tedpatrick commented Sep 28, 2022

JeffersGlass commented Sep 28, 2022

tedpatrick commented Sep 28, 2022

antocuni left a comment

Choose a reason for hiding this comment

tedpatrick commented Sep 28, 2022

JeffersGlass commented Sep 28, 2022 •

edited

Loading

tedpatrick commented Sep 28, 2022 •

edited

Loading

tedpatrick commented Sep 28, 2022 •

edited

Loading

antocuni left a comment •

edited

Loading