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.

Sign up for GitHub

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

Jump to bottom

Implements support for external package loading in validator. #3140

Merged
merged 1 commit into from
Nov 27, 2024
Merged

Implements support for external package loading in validator. #3140

merged 1 commit into from
Nov 27, 2024

Conversation

dom96
Copy link
Collaborator

@dom96 dom96 commented Nov 19, 2024 •
edited
Loading

Moves package loading to prepareWasmLinearMemory so that preloadDynamicLibs can use the loaded packages instead of the embedded ones.

Also fixes a stack-use-after-scope error in getPyodidePackage

Test Plan

$ bazel test @workerd//src/cloudflare/internal/test/... --config=asan

@dom96 dom96 force-pushed the dominik/EW-8835 branch 7 times, most recently from ce98a0a to 3a04883 Compare November 25, 2024 12:10
@dom96 dom96 changed the title Don't use embedded package bundle when external package loading is enabled Implements support for external package loading in validator. Nov 25, 2024
@dom96 dom96 marked this pull request as ready for review November 25, 2024 14:32
@dom96 dom96 requested review from a team as code owners November 25, 2024 14:32
@dom96 dom96 force-pushed the dominik/EW-8835 branch 2 times, most recently from 955a8e3 to 5b3d644 Compare November 26, 2024 11:00
dom96
dom96 commented Nov 26, 2024
View reviewed changes
src/pyodide/python-entrypoint-helper.ts Outdated
Comment on lines 170 to 171
// TODO: fix this.
await getPyodide();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I remove this call then some of our workerd tests fail under asan.

https://paste.cfdata.org/GdKtTZYLiz6LktnWP5gP

Hood mentioned this may be related to the isolate pool changes. @danlapid any ideas what might be going on here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Had to remove it again. No way around it. I'll probably disable these tests under asan for now but we should fix this asap, this asan issue is likely lurking there right now and just isn't being triggered.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Pool code hasn't been merged yet so I don't see how it might be related

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alright, I'll merge this in for now and ask the team if they have any ideas about how to debug this.

hoodmane
hoodmane reviewed Nov 26, 2024
View reviewed changes
src/pyodide/internal/setupPackages.ts
@@ -44,6 +45,7 @@ class SitePackagesDir {
path: '',
name: '',
parts: [],
reader: null,
Copy link
Contributor

Choose a reason for hiding this comment

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

What about setting reader: EmbeddedPackagesTarReader and removing the node.reader ?? EmbeddedPackagesTarReader logic in snapshot.ts? Would that work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably, but doesn't matter much. I plan to get rid of EmbeddedPackagesTarReader in a follow up PR.

hoodmane
hoodmane approved these changes Nov 26, 2024
View reviewed changes
Copy link
Contributor

@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.

I made a few minor questions and suggestions.

@dom96 dom96 force-pushed the dominik/EW-8835 branch 3 times, most recently from f9a3602 to 601d573 Compare November 26, 2024 16:49
fhanau
fhanau reviewed Nov 26, 2024
View reviewed changes
.bazelrc Outdated Show resolved Hide resolved
@dom96 dom96 merged commit 564c8a2 into main Nov 27, 2024
14 checks passed
@dom96 dom96 deleted the dominik/EW-8835 branch November 27, 2024 11:51
fhanau added a commit that referenced this pull request Nov 27, 2024
@fhanau
[build] Add missing no-asan tag
e3d2050 
This is needed following #3140, #3166 adds a new python test which also needs
the tag.
@fhanau fhanau mentioned this pull request Nov 27, 2024
Merged
fhanau added a commit that referenced this pull request Nov 27, 2024
@fhanau
[build] Add missing no-asan tag
2cabf27 
This is needed following #3140, #3166 adds a new python test which also needs
the tag.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danlapid danlapid danlapid left review comments

@fhanau fhanau fhanau left review comments

@hoodmane hoodmane hoodmane approved these changes

@ketanhwr ketanhwr Awaiting requested review from ketanhwr ketanhwr is a code owner automatically assigned from cloudflare/workers-runtime-1

@jp4a50 jp4a50 Awaiting requested review from jp4a50 jp4a50 is a code owner automatically assigned from cloudflare/workers-runtime-1

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dom96 @hoodmane @fhanau @danlapid