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

Fix JsException passing from pyodide environment to host #44

Merged
merged 8 commits into from
Oct 12, 2022

Conversation

koenvo
Copy link
Contributor

@koenvo koenvo commented Oct 11, 2022

When a JsException exception is raised the pyodide environment pickles the exception and send it back to the host. The host tries to unpickle it and fails. This happens because pyodide.JsException does not exist.

This PR solves this issue by interception during unpickling.

note: maybe a more generic way of passing exceptions from pyodide environment to host is better? cloudpickle might do the trick.

Copy link
Member

@hoodmane hoodmane left a comment

Choose a reason for hiding this comment

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

Thanks @koenvo! It looks good to me.

@koenvo
Copy link
Contributor Author

koenvo commented Oct 11, 2022

Thanks @koenvo! It looks good to me.

I noticed some tests were failing. Hope all run correctly now.

tests/test_decorator.py Outdated Show resolved Hide resolved
Co-authored-by: Hood Chatham <roberthoodchatham@gmail.com>
Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

Thanks @koenvo . LGTM as well.

note: maybe a more generic way of passing exceptions from pyodide environment to host is better? cloudpickle might do the trick.

Indeed that's a good question. We could consider switching to cloudpickle if we run into other frequent pickling issues.

@rth
Copy link
Member

rth commented Oct 12, 2022

Could you please just add a changelog.md entry?

@rth
Copy link
Member

rth commented Oct 12, 2022

main / test (macos-latest, 0.21.0, selenium, safari, 1) (pull_request) Failing after

Not sure what's going on with that test, but it's failing quite frequently (unrelated to this PR).

@rth rth merged commit cf625a8 into pyodide:main Oct 12, 2022
@koenvo koenvo deleted the fix-jsexception branch October 12, 2022 14:11
@ryanking13
Copy link
Member

main / test (macos-latest, 0.21.0, selenium, safari, 1) (pull_request) Failing after

Not sure what's going on with that test, but it's failing quite frequently (unrelated to this PR).

It seems like safaridriver is a bit unstable. Probably we can disable a test using selenium_standalone_refresh fixture which fails more frequently.

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

Successfully merging this pull request may close these issues.

4 participants