-
Notifications
You must be signed in to change notification settings - Fork 293
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
Mount python worker files after taking memory snapshot #2806
Conversation
93f9e76
to
246c248
Compare
Module.FS.mount(mdFS, {}, '/session/metadata'); | ||
simpleRunPython( | ||
Module, | ||
`import sys; sys.path.append("/session/metadata"); del sys` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be totally off base here but wouldn't we want sys.path.insert(0, ...)
here?
If a user has http.py file with his main.py file we'd want his http.py file to take precedence over the builtin http module when he uses import http
no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit: I can see this was moved so obviously it isn't specific to this PR but just a general question
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, and sorry for being annoying but the move actually does change behaviour where now site-packages are before session/metadata in path in contrast to previous behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point I put that back, and sys.path.insert(0, ...)
would be better. I'll make that change in a separate PR though.
045d770
to
c05f0ea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this!
@@ -424,7 +424,7 @@ struct CompatibilityFlags @0x8f8c1b68151b6cef { | |||
pythonWorkers @43 :Bool | |||
$compatEnableFlag("python_workers") | |||
$pythonSnapshotRelease(pyodide = "0.26.0a2", pyodideRevision = "2024-03-01", | |||
packages = "2024-03-01", backport = 0) | |||
packages = "2024-03-01", backport = 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why you're bumping this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I made a new bundle with the updated code with backport number 1. I want the new bundle to be used, so I have to bump the backport number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, because the .ts file changes here need a new bundle in order to be used in prod, right?
It's going to take a bit of a mind shift to keep that in mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's annoying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah.. this is the thing that I'm concerned is going to end up causing us too much pain. Like even my small PR will need a regenerated bundle, right? #2830
I guess small changes like that can wait. Anyway, we should discuss about how we want to handle this. Started a thread internally for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This ensures that the contents of worker files cannot be accessed prior to taking the snapshot and so won't appear in the linear memory.
c05f0ea
to
08bad5a
Compare
This ensures that the contents of worker files cannot be accessed prior to taking the snapshot and so won't appear in the linear memory.