Make sure we don't import pyodide into memory snapshot. #3499
Conversation
hoodmane
force-pushed
the
hoodmane/dont-snapshot-pyodide
branch
from
ecd7f65 to
761ce3d
Compare
| @@ -0,0 +1,2 @@ | |||
| async def test(): | |||
There was a problem hiding this comment.
How does this test the issue?
There was a problem hiding this comment.
To trigger the bug we need to do three things:
- import
pyodideat top level - ensure that there is some package requirement
- make
test()async
The numpy import here is unnecessary, but if someone removed (name = "numpy", pythonRequirement = "") from the wd-test file it would no longer reproduce the problem.
There was a problem hiding this comment.
Oh shoot ruff removed the imports. I keep having this problem.
There was a problem hiding this comment.
But where is the pyodide import? 😅
There was a problem hiding this comment.
Oh shoot ruff removed the imports. I keep having this problem.
Hahaha that explains it
There’s probably some comments that disable ruff on a file right?
There was a problem hiding this comment.
I put a whole explanation in now. Also #3415 is such a huge improvement to our test suite I am so happy we merged it.
| const moduleSet = new Set(importedModules); | ||
| // Importing pyodide before `finalizeBootstrap()` is called causes mayhem | ||
| // Make sure we don't do that! | ||
| // TODO: in Pyodide 0.27 we can call finalizeBootstrap() before taking the snapshot. Doing so will |
There was a problem hiding this comment.
Perhaps we need a way to check pyodide version when generating the bundles so we could make things like this conditional
|
LGTM, only problem is I don’t understand the test |
Make sure that we don't do that.
hoodmane
force-pushed
the
hoodmane/dont-snapshot-pyodide
branch
from
761ce3d to
cfa585d
Compare
| // Make sure we don't do that! | ||
| // TODO: in Pyodide 0.27 we can call finalizeBootstrap() before taking the snapshot. Doing so will | ||
| // remove the need for this. | ||
| moduleSet.delete('pyodide'); |
There was a problem hiding this comment.
We already have some filtering logic in
workerd/src/workerd/api/pyodide/pyodide.c++
Line 351 in 07cb474
Seems to only be filtering out js directly right now, but best to do the filtering in one place.
| const moduleSet = new Set(importedModules); | ||
| // Importing pyodide before `finalizeBootstrap()` is called causes mayhem | ||
| // Make sure we don't do that! | ||
| // TODO: in Pyodide 0.27 we can call finalizeBootstrap() before taking the snapshot. Doing so will |
There was a problem hiding this comment.
Since this is only necessary for 0.26, should we gate this behind a version check?
There was a problem hiding this comment.
It's necessary for both until we move finalizeBootstrap().
|
There's nothing else that is publicly allowed to import. There are a few implementation detail imports that could cause trouble but I think that'd be a won't-fix since people really shouldn't touch that stuff. |
Importing pyodide before
finalizeBootstrap()is called causes mayhem.