Use ESM import in module type webworker #2220

leopsidom · 2022-02-26T08:42:15Z

Description

The PR is to avoid importScripts error when loading pyodide in a module type worker. Currently loadScripts uses importScripts to import dependency which throws an error when the worker is of type module.

Checklists

Add a CHANGELOG entry
Add / update tests
Add new / update outdated documentation

hoodmane · 2022-02-26T14:27:47Z

Thanks @leopsidom. Maybe it would be better instead to test typeof loadScript !== "undefined" in the conditional.

leopsidom · 2022-02-26T19:34:50Z

Thanks @leopsidom. Maybe it would be better instead to test typeof loadScript !== "undefined" in the conditional.

Do you mean test type globalThis.importScripts !== "undefined" ? I think the issue is that importScripts is still defined in both classic and module type web worker. It just throws an error when the web worker is of type module at run time.

leopsidom · 2022-02-26T20:00:31Z

Added a check to narrow down the exception type to TypeError and rethrow other exceptions.

hoodmane · 2022-02-27T17:47:50Z

the issue is that importScripts is still defined in both classic and module type web worker. It just throws an error when the web worker is of type module at run time.

That is unfortunate... Is there any other direct means one can use to distinguish between a module worker and a classic worker? I looked through the whatwg worker spec and I think the answer is no. Hopefully this is the only reason that importScripts would raise a TypeError though.

hoodmane

Ideally we should add a test for importing Pyodide as a module, both in a module worker and on the main thread, or else it might get broken again.

leopsidom · 2022-02-27T18:57:36Z

Ideally we should add a test for importing Pyodide as a module, both in a module worker and on the main thread, or else it might get broken again.

Yeah, that makes sense. I'll take a look at adding tests for module import, though the tests won't work for module type worker in firefox yet as it's still catching up with the support at the moment.

hoodmane · 2022-02-27T20:07:00Z

I'll take a look at adding tests for module import

If you get stuck, let me know. You will probably want the selenium_webworker_standalone fixture, but it might be possible to use it with module scope instead of function scope which would decrease time to execute tests. Though it's good to just get something working for now and we can futz with that sort of thing later if we feel like it.

the tests won't work for module type worker in firefox yet

Sure. You can xfail it in firefox.

leopsidom · 2022-02-27T20:46:24Z

Sure, just read a little about module and function scope, I'm not completely sure that I understands the difference.

For the tests, I'm thinking we could add a type parameter to the SeleniumWrapper class, which can be further passed to get_driver and run_webworker to import pyodide in the classic and module manner. Then we can create a separate selenium_module_webworker_standalone and selenium_module_standalone as counter part of selenium_webworker_standalone and selenium_standalone. With which we can write minimum enough tests to just make sure pyodide is loaded correctly in the module syntax. This won't reuse existing tests, so potentially not enough coverage but avoid redundant tests with assumption that after imported, the script should behave identically in both classic and module type.

hoodmane · 2022-02-27T21:09:03Z

Sounds like a good plan to me.

There's a tradeoff we don't want to run the entire test suite in every combination of browser and webworker configuration since it would take too long. Just testing that we can successfully load Pyodide would catch the problem you reported.

hoodmane · 2022-02-27T21:12:50Z

The issue with fixture scope is essentially how often we reload the browser tab. Refreshing the browser tab provides better isolation between tests but at the cost of taking much longer to run them.

leopsidom · 2022-02-27T21:29:02Z

Cool. Got it. I think as long as we don't get issues on tests interacting with each other, we can definitely change the scope so it saves time on refreshing the context.

hoodmane · 2022-02-27T23:11:46Z

Makefile

@@ -104,6 +105,8 @@ src/js/pyproxy.gen.ts : src/core/pyproxy.* src/core/*.h
 build/test.html: src/templates/test.html
 	cp $< $@

+build/module_test.html: src/templates/module_test.html


At some point we probably should try to combine some of these templates build rules (no action needed in this PR).

src/templates/module_test.html

src/tests/test_webworker_module.py

…orker

leopsidom · 2022-03-01T07:21:37Z

The test_draw_math_text in test-packages-firefox seems to fail pretty consistently in other PR: like this from the latest commit. Even for the passed test, there is a retry after the initial time out failure.

leopsidom · 2022-03-01T07:26:03Z

Similar for this workflow from this commit.

hoodmane · 2022-03-01T07:26:14Z

Yeah the test failure has nothing to do with this PR.

hoodmane · 2022-03-01T07:38:41Z

In particular, that means the test failure won't prevent us from merging this.

leopsidom · 2022-03-01T07:45:22Z

In particular, that means the test failure won't prevent us from merging this.

Yeah it seems to be failing inherently due to an unknown issue. So I guess we shouldn't let it block us from merging new changes. Wonder if it's a selenium issue or firefox issue. But considering it's even xfail chrome for recursion limit, it looks like it's trying to push the limit of the browser capability.

hoodmane · 2022-03-01T07:51:23Z

xfailing chrome for the recursion limit must be out of date, we fixed that cf #2019

hoodmane · 2022-03-01T07:53:43Z

But yeah, perhaps just turning that benchmark off would be a good start.

ryanking13 · 2022-03-01T09:36:37Z

The test_draw_math_text in test-packages-firefox seems to fail pretty consistently in other PR: like this from the latest commit. Even for the passed test, there is a retry after the initial time out failure.

I am looking into it! sorry for the confusion :) (#2226)

src/tests/test_webworker.py

src/tests/test_python_module.py

…fail to skip test in fixture

hoodmane · 2022-03-02T07:16:26Z

src/templates/module_test.html

+      window.logs = [];
+      console.log = function (message) {
+        window.logs.push(message);
+      };
+      console.warn = function (message) {
+        window.logs.push(message);
+      };
+      console.info = function (message) {
+        window.logs.push(message);
+      };
+      console.error = function (message) {
+        window.logs.push(message);
+      };


Perhaps we should move this Javascript setup out of test.html and module_test.html into tools/testsetup.js? This would reduce the duplication.

Though maybe the issue is we specifically don't want it in node? I can't remember. I feel like I tried to move this to testsetup.js before and it didn't work. If we did move it into tools/testsetup.js all window would have to be replaced with self.

Oh sure, I think we could move the logic in a separate js file. I suspect node should not use the test.html file, though need to double check. I can try it at some point.

hoodmane · 2022-03-02T08:31:35Z

Thanks @leopsidom!

ryanking13 · 2022-03-02T23:46:55Z

src/templates/module_test.html

+    </script>
+    <script type="module">
+      import { loadPyodide } from "./pyodide.mjs";
+      window.loadPyodide = loadPyodidea;


I guess this must be a typo?

huh I wonder why that wouldn't break the tests

Hmm, that's interesting. Let me double check ~

Okay I think the tests pass because we already attach loadPyodide to globalThis here, while this typoed line errors out due to undefined loadPyodidea, so the line is not doing anything except throwing an error.

Thanks for the investigation. In that case we don't need to bind loadPyodide to the global in the test.html file anymore, so we would better remove those.

Oh I think I understand what you mean, when we call loadPyodide multiple times, we are overwriting the same Module instance.

Yeah exactly.

I personally don't have much use case to load multiple pyodide instances yet. But I think it could definitely be something useful. Maybe we could wrap modules and loadPyodide in a class Pyodide and expose that class. Then every time we instantiate a new instance of Pyodide, we create a new standalone module that does not interfere with other instances.

Yeah maybe that would work.

Catch importScripts error in module type webworker

adc508f

leopsidom mentioned this pull request Feb 26, 2022

Support importing pyodide as ESM module #2217

Closed

root added 2 commits February 26, 2022 09:03

add change log

c9a7113

remove accidentally committed files

92d608a

root added 2 commits February 26, 2022 19:23

remove unnecessary return

3d9ce75

add comments

2ac2c6b

root added 3 commits February 26, 2022 19:48

Narrow down the exception to TypeError

fa9e593

add ending semicolon

05d6f8d

rethrow error if it's not typeerror

5532a70

hoodmane approved these changes Feb 27, 2022

View reviewed changes

root added 2 commits February 27, 2022 22:40

add tests for importing pyodide.mjs

fa05432

fix syntax error

adfcb1a

hoodmane reviewed Feb 27, 2022

View reviewed changes

src/templates/module_test.html Outdated Show resolved Hide resolved

hoodmane reviewed Feb 27, 2022

View reviewed changes

src/tests/test_webworker_module.py Outdated Show resolved Hide resolved

root added 5 commits February 27, 2022 23:36

use e.message instead of e.toString for error message

00fe65e

fix worker test syntax error

9230698

revert first arg to cb to toString to not break tests

07215f3

parameterize webworker test to test both classic and module type webw…

efd0c52

…orker

try a different parameterize

7193247

root added 10 commits February 28, 2022 02:36

pass worker type as param to fixture

ef460ff

fall back to classic if test doesn't pass a parameter on script type

4cdcc3e

create script_type fixture to pass in script type

7f86edf

Pass request to script type; xfail firefox module webworker tests

da70a20

fix module webworker base url

5bfc928

fix lint

e08dd47

fix lint 2

025d7a3

fix lint 3

05f8882

fix lint 4

eb9dc32

fix lint 5

2d86efe

hoodmane reviewed Mar 2, 2022

View reviewed changes

src/tests/test_webworker.py Outdated Show resolved Hide resolved

hoodmane reviewed Mar 2, 2022

View reviewed changes

src/tests/test_python_module.py Outdated Show resolved Hide resolved

root added 5 commits March 2, 2022 03:12

Change module test selenium fixture to module scope; move webworker x…

0d076f1

…fail to skip test in fixture

fix incorrect variable

b992fe9

fix lint

42e30e8

revert to selenium_standalone for test_init

e2a3118

fix lint

1337343

hoodmane reviewed Mar 2, 2022

View reviewed changes

hoodmane merged commit 8798e1f into pyodide:main Mar 2, 2022

ryanking13 reviewed Mar 2, 2022

View reviewed changes

hoodmane mentioned this pull request Mar 5, 2022

Unable to retry loading pyodide, get error "Pyodide is already loading" #2255

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ESM import in module type webworker #2220

Use ESM import in module type webworker #2220

leopsidom commented Feb 26, 2022 •

edited

Loading

hoodmane commented Feb 26, 2022

leopsidom commented Feb 26, 2022

leopsidom commented Feb 26, 2022

hoodmane commented Feb 27, 2022

hoodmane left a comment

leopsidom commented Feb 27, 2022

hoodmane commented Feb 27, 2022 •

edited

Loading

leopsidom commented Feb 27, 2022

hoodmane commented Feb 27, 2022

hoodmane commented Feb 27, 2022

leopsidom commented Feb 27, 2022

hoodmane Feb 27, 2022

leopsidom commented Mar 1, 2022 •

edited

Loading

leopsidom commented Mar 1, 2022

hoodmane commented Mar 1, 2022

hoodmane commented Mar 1, 2022 •

edited

Loading

leopsidom commented Mar 1, 2022 •

edited

Loading

hoodmane commented Mar 1, 2022

hoodmane commented Mar 1, 2022

ryanking13 commented Mar 1, 2022

hoodmane Mar 2, 2022

hoodmane Mar 2, 2022 •

edited

Loading

leopsidom Mar 2, 2022

hoodmane commented Mar 2, 2022

ryanking13 Mar 2, 2022

hoodmane Mar 3, 2022

leopsidom Mar 3, 2022

leopsidom Mar 3, 2022 •

edited

Loading

ryanking13 Mar 3, 2022

leopsidom Mar 3, 2022

hoodmane Mar 3, 2022

leopsidom Mar 5, 2022

hoodmane Mar 5, 2022

hoodmane Mar 5, 2022

Use ESM import in module type webworker #2220

Use ESM import in module type webworker #2220

Conversation

leopsidom commented Feb 26, 2022 • edited Loading

Description

Checklists

hoodmane commented Feb 26, 2022

leopsidom commented Feb 26, 2022

leopsidom commented Feb 26, 2022

hoodmane commented Feb 27, 2022

hoodmane left a comment

Choose a reason for hiding this comment

leopsidom commented Feb 27, 2022

hoodmane commented Feb 27, 2022 • edited Loading

leopsidom commented Feb 27, 2022

hoodmane commented Feb 27, 2022

hoodmane commented Feb 27, 2022

leopsidom commented Feb 27, 2022

Choose a reason for hiding this comment

leopsidom commented Mar 1, 2022 • edited Loading

leopsidom commented Mar 1, 2022

hoodmane commented Mar 1, 2022

hoodmane commented Mar 1, 2022 • edited Loading

leopsidom commented Mar 1, 2022 • edited Loading

hoodmane commented Mar 1, 2022

hoodmane commented Mar 1, 2022

ryanking13 commented Mar 1, 2022

Choose a reason for hiding this comment

hoodmane Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoodmane commented Mar 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leopsidom Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leopsidom commented Feb 26, 2022 •

edited

Loading

hoodmane commented Feb 27, 2022 •

edited

Loading

leopsidom commented Mar 1, 2022 •

edited

Loading

hoodmane commented Mar 1, 2022 •

edited

Loading

leopsidom commented Mar 1, 2022 •

edited

Loading

hoodmane Mar 2, 2022 •

edited

Loading

leopsidom Mar 3, 2022 •

edited

Loading