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

Don't fail at upload time when module modifies sys.path #3409

Closed
wants to merge 1 commit into from

Conversation

hoodmane
Copy link
Contributor

We try to execute all the top level imports but we don't execute changes to the path so we can get crashes.

@hoodmane hoodmane requested review from a team as code owners January 24, 2025 18:54
@danlapid
Copy link
Collaborator

Why not just execute the top level scope?

@hoodmane
Copy link
Contributor Author

Well the idea here is that we share the package snapshots between workers with the same top level imports. If you exec'd the top level scope, you'd have to use a dedicated snapshot just for that one worker.

@danlapid
Copy link
Collaborator

Well the idea here is that we share the package snapshots between workers with the same top level imports. If you exec'd the top level scope, you'd have to use a dedicated snapshot just for that one worker.

I see, so that's why you went with a try and ignore failures approach 👍

@hoodmane
Copy link
Contributor Author

Also, this avoids the need to encrypt snapshots which makes debugging a lot easier.

@jasnell jasnell added the python Issues/PRs relating to Python Workers label Jan 24, 2025
Copy link
Collaborator

@dom96 dom96 left a comment

Choose a reason for hiding this comment

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

A test for the case that this solves would be good

src/pyodide/internal/snapshot.ts Outdated Show resolved Hide resolved
@hoodmane
Copy link
Contributor Author

Yeah we'll need a validator test for this.

We try to execute all the top level imports but we don't execute
changes to the path so we can get crashes.
@hoodmane hoodmane force-pushed the hoodmane/sys-path-modification branch from cd4ee13 to 26ea4a8 Compare January 25, 2025 17:59
@hoodmane
Copy link
Contributor Author

hoodmane commented Jan 25, 2025

I guess we should add a test that exercises the validation code path from workerd so we don't have to rely on edgeworker tests for it.

@hoodmane
Copy link
Contributor Author

Okay I am no longer reproducing any bug so I'll close this.

@hoodmane hoodmane closed this Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Issues/PRs relating to Python Workers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants