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

FileSystem calls over Atomics.wait instead of service worker when available #114

Merged
merged 5 commits into from
Jun 21, 2024

Conversation

martinRenou
Copy link
Member

This has the implication of moving from using comlink to using coincident for the Main <-> Worker communcication

@martinRenou martinRenou marked this pull request as draft June 7, 2024 09:45
Copy link
Contributor

github-actions bot commented Jun 7, 2024

lite-badge 👈 Try it on ReadTheDocs

@martinRenou martinRenou added the enhancement New feature or request label Jun 7, 2024
@martinRenou
Copy link
Member Author

Oh there are no visual regression tests testing the code execution. That makes me uncomfortable :)

@martinRenou
Copy link
Member Author

Well at least we have the readthedocs build for trying

@martinRenou martinRenou marked this pull request as ready for review June 7, 2024 10:49
@martinRenou martinRenou requested review from jtpio and bollwyvl June 7, 2024 10:49
@jtpio jtpio added this to the 0.4.0 milestone Jun 7, 2024
@bollwyvl bollwyvl mentioned this pull request Jun 7, 2024
3 tasks
@martinRenou martinRenou marked this pull request as draft June 7, 2024 14:09
@martinRenou
Copy link
Member Author

Let's get #115 in first

@martinRenou martinRenou force-pushed the coincident_shared_array branch from 3e2cd9d to ba9de02 Compare June 17, 2024 08:21
@martinRenou martinRenou marked this pull request as ready for review June 17, 2024 10:29
@martinRenou martinRenou requested a review from bollwyvl June 17, 2024 10:45
@bollwyvl
Copy link
Contributor

On RTD, this still demonstrates jupyterlite/jupyterlite#1412 .

If that gets fixed over there, we need that to land a PR and ship another beta there before this can be reviewed.

If this is going to get fixed on this side, it has to happen either on a separate PR, or on this PR, before it can be properly reviewed.

@martinRenou
Copy link
Member Author

I can't seem to be able to reproduce what you're seeing on this RTD build

Screenshot from 2024-06-17 15-00-17

@jtpio
Copy link
Member

jtpio commented Jun 17, 2024

I can't seem to be able to reproduce what you're seeing on this RTD build

jupyterlite/jupyterlite#1412 mentions the build on the JupyterLite repo, which likely uses an old release of jupyterlite-pyodide-kernel. In that case using https://jupyterlite--1383.org.readthedocs.build/en/1383/_static/lab/index.html I can also reproduce the error:

image

Could it be because that RTD demo uses the latest JupyterLite, but an older jupyterlite-pyodide-kernel. If so, wouldn't that be fixed with the JupyterLite 0.4.0 and jupyterlite-pyodide-kernel 0.4.0 final releases? (as with this PR)

@bollwyvl
Copy link
Contributor

Welp, i'm getting 100% repro of the above issue on linux/chromium 126.0.6478.61, in and out of an incognito window.

image

We know it won't work in FF private, but also seeing in firefox 127.0:

image

@martinRenou
Copy link
Member Author

I've looked deeper into it, I believe jupyterlite/jupyterlite#1418 should fix it. Can you give it a try? I see it working there

Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

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

Thanks!

@jtpio jtpio merged commit ae20dc7 into jupyterlite:main Jun 21, 2024
10 checks passed
@jtpio
Copy link
Member

jtpio commented Jun 21, 2024

FYI @bollwyvl 0.4.0b0 is out with this change: https://github.com/jupyterlite/pyodide-kernel/releases/tag/v0.4.0b0

If that looks good to you, we can likely move forward with JupyterLite 0.4.0 and release final in the coming days.

@martinRenou martinRenou deleted the coincident_shared_array branch June 21, 2024 12:23
@bollwyvl
Copy link
Contributor

Won't be able to review much this weekend, but will roll it on conda-forge and see what we get...

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

Successfully merging this pull request may close these issues.

3 participants