Refactor loadPackage #1084

dalcde · 2021-01-09T04:51:36Z

From the API point of view, this removes the messageCallback and errorCallback arguments in loadPackage, runPythonAsync etc. These are used for logging. Previously, any message will be written to the console, and then {message,error}Callback is called with the message as an argument. In most of our use cases, these callbacks do nothing.

If we were to reintroduce this feature, I think we should introduce a logger interface, and users can supply custom loggers. This lets them choose to not output to stdout/stderr as well.

Under the hood, the original motivation of this patch was to get rid of using packageCounter to determine whether all packages have been loaded. This is prone to error if we change the way we load packages (which I attempted in a different branch until I encountered an emscripten bug).

One way or another, I ended up rewriting a lot of the code into something with a clearer control flow imo.

From the API point of view, this removes the messageCallback and errorCallback arguments in loadPackage, runPythonAsync etc. These are used for logging. Previously, any message will be written to the console, and then {message,error}Callback is called with the message as an argument. In most of our use cases, these callbacks do nothing. If we were to reintroduce this feature, I think we should introduce a logger interface, and users can supply custom loggers. This lets them choose to not output to stdout/stderr as well. Under the hood, the original motivation of this patch was to get rid of using packageCounter to determine whether all packages have been loaded. This is prone to error if we change the way we load packages (which I attempted in a different branch until I encountered an emscripten bug). One way or another, I ended up rewriting a lot of the code into something with a clearer control flow imo.

hoodmane

Thanks for taking a stab at this. I still have lots of issues with this code but this is a clear and manageable improvement. I would have trouble leaving enough stuff alone to maintain a manageable diff (cf #998).
Maybe change toLoad from an object to a Map? I think that'd be better.

I think the windowErrorHandler is really bad: what sorts of errors hit it and why? Can't we handle such errors in a less brute force way?

docs/js-api/pyodide_loadPackagesFromImports.md

src/pyodide.js

Co-authored-by: Hood Chatham <roberthoodchatham@gmail.com>

src/pyodide.js

Co-authored-by: Hood Chatham <roberthoodchatham@gmail.com>

dalcde · 2021-01-09T06:17:56Z

Just for the record, the very original motivation was to get rid of our custom preloadWasm code and use the one that comes with emscripten, but turns out that's not compatible with -s LZ4

hoodmane · 2021-01-09T06:18:07Z

See window.onerror:

When a JavaScript runtime error (including syntax errors and exceptions thrown within handlers) occurs, an error event using interface ErrorEvent is fired at window and window.onerror() is invoked (as well as handlers attached by window.addEventListener (not only capturing)).
When a resource (such as an <img> or <script>) fails to load, an error event using interface Event is fired at the element that initiated the load, and the onerror() handler on the element is invoked. These error events do not bubble up to window, but can be handled with a window.addEventListener configured with useCapture set to true.

I think we're trying to catch the second type of errors? So the correct way to do it is to attach an onError element to the script tag? What else is going to generate uncaught errors?

dalcde · 2021-01-09T06:19:12Z

I think I am concerned mostly about the first kind of error within the loaded script. The second kind is captured by the loadScript function already.

Co-authored-by: Hood Chatham <roberthoodchatham@gmail.com>

hoodmane · 2021-01-09T06:23:01Z

It seems sad to be unable to catch the error any closer to the source, but if we add my noisy and explicit console.error message, I think it's okay. Can you make a test that hits that execution path and cross reference against the test in the code though? I think it's natural to wonder why the error can't be isolated any better.

Incidentally, it's sort of remarkable that there isn't a less error prone and hacky way to load a script. Why doesn't the loadScript api exist on the main thread? Because it's blocking? Why is it blocking?! Browsers are so weird >_<

dalcde · 2021-01-09T06:28:16Z

Well, we can fetch the source code and eval it ....

hoodmane · 2021-01-09T06:31:01Z

fetch the source code and eval

Do we lose anything by doing that? Maybe weaker CORS restrictions?

hoodmane · 2021-01-09T06:33:07Z

Maybe it's forbidden by "Content Security Policy" or something?

hoodmane · 2021-01-09T06:52:34Z

Okay so if I understand correctly for the global window handler:

this is necessary and can happen
the other ways to do this have their own drawbacks and it's unclear if they are any better, and
you know how to cause the code to trigger but it's hard to test

In that case I guess the most reasonable thing is just to put a comment explaining the situation and saying that it is technically necessary but unfortunate, and also to make it emit a noisy and explicit error message into the console if this happens.

The main difference in end result in we no longer produce a hard error when errors occur during the dependency resolution stage; we simply ignore the offending packages. We still error if something goes wrong during package loading, as the system may be left in a broken state. The error messages produced are, however, slightly different. Now if a package is loaded from a custom url then the default channels, we no longer print it as an error; we assume the user is trying to override a dependency. Since we don't act on these errors anyway, this doesn't affect the API. Smaller changes include changing to a recursive DFS algorithm and turning toLoad into a Map

rth · 2021-01-09T10:50:17Z

Previously, any message will be written to the console, and then {message,error}Callback is called with the message as an argument. In most of our use cases, these callbacks do nothing.

In our uses cases yes, but it would allow users to customize where errors/messages are shown.

If we were to reintroduce this feature, I think we should introduce a logger interface, and users can supply custom loggers.

In my understanding this feature is critical for downstream applications (e.g. Basthon, Starboard Notebook). Granted our error handling there can certainly be improved, but if we remove it we can't really make a release until a new API is includes. I think it would be better to,

have a separate PR with as much of refactoring from this PR as possible
open an issue and discuss how we want our error handling API to look. A logger might indeed be nice, but I also don't see it as fundamentally different from error callbacks. For instance Add pyodide-js library (alternative to pyodide.js and webworker.js) #792 still has callbacks.
make a PR that replaces the logging API if necessary. Just to avoid a situation where we decide after removing them that callback where actually fine.

Also for all changes that break backward compatibility and change the Public API, it would be better to have the changed method/arguments names in the title, as opposed to just "refactoring". Here this have been, API Remove messageCallback and errorCallback arguments from loadPackage

dalcde · 2021-01-09T11:54:54Z

In my understanding this feature is critical for downstream applications (e.g. Basthon, Starboard Notebook). Granted our error handling there can certainly be improved, but if we remove it we can't really make a release until a new API is includes. I think it would be better to, - have a separate PR with as much of refactoring from this PR as possible

I'll re-introduce the callbacks into this PR

- open an issue and discuss how we want our error handling API to look. A logger might indeed be nice, but I also don't see it as fundamentally different from error callbacks. For instance #792 still has callbacks.

In my mind a logger interface is an object with ".log" and ".error" functions, so basically combining the two into one. Perhaps I could just do that and pass in console by default. But I'm thinking of using it more pervasively, e.g. by default stdout/stderr should go there.

Also for all changes that break backward compatibility and change the Public API, it would be better to have the changed method/arguments names in the title, as opposed to just "refactoring". Here this have been, `API Remove messageCallback and errorCallback arguments from loadPackage`

Yeah this PR diverted from its original mission by *quite* a bit.

dalcde · 2021-01-09T11:55:43Z

But I think I'll change the behaviour so that the default callbacks are console.log and console.error, and supplying a custom callback would cause it to stop logging to console.

rth · 2021-01-09T12:01:09Z

In my mind a logger interface is an object with ".log" and ".error"
functions, so basically combining the two into one.

Yeah, that sound good. But let's still open an issue to discuss it before implementing it? I would like to also get feedback from downstream package maintainers.

hoodmane · 2021-01-09T13:36:40Z

Yeah we should discuss with downstream whether they actually need an error callback. I think the right thing to do is to be able to request a logging level ("error", "warn", "info", "debug"). If they want to execute code when there is an error, what about try + catch?

rth

Thanks! Should be good to merge unless @hoodmane has more comments.

hoodmane

Looks really nice!

src/pyodide.js

hoodmane · 2021-01-12T03:24:55Z

Okay this looks good to me, I think it should be accepted.

rth · 2021-01-12T08:36:49Z

Thanks @dalcde and @hoodmane !

dalcde added 2 commits January 9, 2021 12:50

Slightly improve error message

d0a9916

hoodmane reviewed Jan 9, 2021

View reviewed changes

dalcde and others added 2 commits January 9, 2021 14:12

Update src/pyodide.js

3a87714

Co-authored-by: Hood Chatham <roberthoodchatham@gmail.com>

Update src/pyodide.js

4c2a862

Co-authored-by: Hood Chatham <roberthoodchatham@gmail.com>

hoodmane reviewed Jan 9, 2021

View reviewed changes

src/pyodide.js Outdated Show resolved Hide resolved

Update src/pyodide.js

e21d708

Co-authored-by: Hood Chatham <roberthoodchatham@gmail.com>

dalcde and others added 2 commits January 9, 2021 14:19

Update src/pyodide.js

adc8983

Co-authored-by: Hood Chatham <roberthoodchatham@gmail.com>

Format

1e6f604

dalcde added 2 commits January 9, 2021 16:10

Format

565878b

rth mentioned this pull request Jan 9, 2021

Add pyodide-js library (alternative to pyodide.js and webworker.js) #792

Closed

Restore callbacks

eb7fff4

dalcde added 3 commits January 9, 2021 22:02

Simplify wait rundependency

2f94fe1

Write better code

120582c

Merge branch 'master' into load

2dfb520

rth approved these changes Jan 11, 2021

View reviewed changes

hoodmane approved these changes Jan 11, 2021

View reviewed changes

src/pyodide.js Outdated Show resolved Hide resolved

src/pyodide.js Outdated Show resolved Hide resolved

dalcde added 4 commits January 12, 2021 08:36

Changes from review

1536beb

Merge branch 'master' into load

3e46486

Fix webworker bug

c8b95f2

Merge branch 'master' into load

d243bf5

rth merged commit a48a1ff into pyodide:master Jan 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor loadPackage #1084

Refactor loadPackage #1084

dalcde commented Jan 9, 2021

hoodmane left a comment •

edited

Loading

dalcde commented Jan 9, 2021

hoodmane commented Jan 9, 2021

dalcde commented Jan 9, 2021

hoodmane commented Jan 9, 2021 •

edited

Loading

dalcde commented Jan 9, 2021

hoodmane commented Jan 9, 2021

hoodmane commented Jan 9, 2021

hoodmane commented Jan 9, 2021 •

edited

Loading

rth commented Jan 9, 2021

dalcde commented Jan 9, 2021 via email

dalcde commented Jan 9, 2021

rth commented Jan 9, 2021

hoodmane commented Jan 9, 2021

rth left a comment

hoodmane left a comment

hoodmane commented Jan 12, 2021

rth commented Jan 12, 2021

Refactor loadPackage #1084

Refactor loadPackage #1084

Conversation

dalcde commented Jan 9, 2021

hoodmane left a comment • edited Loading

Choose a reason for hiding this comment

dalcde commented Jan 9, 2021

hoodmane commented Jan 9, 2021

dalcde commented Jan 9, 2021

hoodmane commented Jan 9, 2021 • edited Loading

dalcde commented Jan 9, 2021

hoodmane commented Jan 9, 2021

hoodmane commented Jan 9, 2021

hoodmane commented Jan 9, 2021 • edited Loading

rth commented Jan 9, 2021

dalcde commented Jan 9, 2021 via email

dalcde commented Jan 9, 2021

rth commented Jan 9, 2021

hoodmane commented Jan 9, 2021

rth left a comment

Choose a reason for hiding this comment

hoodmane left a comment

Choose a reason for hiding this comment

hoodmane commented Jan 12, 2021

rth commented Jan 12, 2021

hoodmane left a comment •

edited

Loading

hoodmane commented Jan 9, 2021 •

edited

Loading

hoodmane commented Jan 9, 2021 •

edited

Loading