Skip to content

Replace the python package URL to R2#6410

Merged
ryanking13 merged 1 commit intomainfrom
gyeongjae/python-load-path
Mar 31, 2026
Merged

Replace the python package URL to R2#6410
ryanking13 merged 1 commit intomainfrom
gyeongjae/python-load-path

Conversation

@ryanking13
Copy link
Copy Markdown
Contributor

@ryanking13 ryanking13 commented Mar 25, 2026 •
edited
Loading

Replaces the Python fetch URL from GCS to R2. Note that our prod system uses a different path so this only affect workerd.

This is currently blocked by cloudflare/pyodide-build-scripts#14 (All 0.28.2 tests pass while 0.26.0a2 tests fail), hence opening as a draft. Done

I also removed WORKERD_INDEX_URL which I don't think we use anymore (no package loading in the runtime).

@ryanking13 ryanking13 requested review from dom96 and hoodmane March 25, 2026 09:35
@ryanking13 ryanking13 requested review from a team as code owners March 25, 2026 09:35
@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Mar 25, 2026

UnknownError: "Internal server error"

github run

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Mar 25, 2026

@ryanking13 Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

@ryanking13
Copy link
Copy Markdown
Contributor Author

ryanking13 commented Mar 25, 2026

/bonk try again

@ryanking13 ryanking13 marked this pull request as draft March 25, 2026 09:41
ask-bonk[bot]
ask-bonk bot reviewed Mar 25, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot left a comment

Choose a reason for hiding this comment

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

Review Summary

The PR makes two changes: (1) switches the Python package fetch URL from GCS to R2, and (2) removes the now-unused WORKERD_INDEX_URL/indexURL parameter from the TS layer. Both are reasonable cleanup. A few items to address:

  • [before-merge] py_wd_test.bzl has disk cache intentionally disabled with a TODO(before merge) comment — needs to be reverted before landing.
  • [nit] The hardcoded indexURL = '' could use a clarifying comment explaining why it's empty now.
  • The proxy.js simplification correctly removes hostnames that are no longer reachable after the URL migration.
  • No remaining references to the old GCS URL or pyodide-packages.runtime-playground hostname exist in the codebase.

This review was generated by an AI assistant (ask-bonk) and may contain inaccuracies.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Mar 25, 2026

Review posted successfully on PR #6410. Here's a summary of what I flagged:

  1. py_wd_test.bzl (before-merge TODO): The disk cache is intentionally disabled with a TODO(before merge) marker. Posted a suggested edit to restore the original lines when ready to land.

  2. python.ts (nit): indexURL is now hardcoded to '' with no explanation. Suggested adding a comment clarifying that packages are loaded via ArtifactBundler in workerd mode, not Pyodide's URL-based loader.

  3. proxy.js (clarification): The previous proxy forwarded package-related hostnames to env.INTERNET. After this change, only example.com is handled. Asked for confirmation this is intentional (it should be, since C++ handles package fetching, not the JS proxy).

The URL migration itself from GCS (storage.googleapis.com/cloudflare-edgeworker-python-packages/) to R2 (pyodide-capnp-bin.edgeworker.net/) looks correct — all references to the old URLs have been cleaned up, and the WORKERD_INDEX_URL removal is safe since packages are loaded entirely through the C++ ArtifactBundler layer, not Pyodide's built-in URL fetcher.

github run

@ryanking13 ryanking13 force-pushed the gyeongjae/python-load-path branch from 707e0a4 to e953b6f Compare March 26, 2026 05:52
@ryanking13 ryanking13 marked this pull request as ready for review March 26, 2026 05:52
@ryanking13 ryanking13 changed the title DRAFT Replace the python package URL to R2 Replace the python package URL to R2 Mar 26, 2026
@ryanking13 ryanking13 force-pushed the gyeongjae/python-load-path branch from e953b6f to 68f289f Compare March 30, 2026 05:19
hoodmane
hoodmane reviewed Mar 30, 2026
View reviewed changes
hoodmane
hoodmane approved these changes Mar 30, 2026
View reviewed changes
Copy link
Copy Markdown
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.

LGTM but we should remove proxy.js and clean that up before merging.

@ryanking13 ryanking13 force-pushed the gyeongjae/python-load-path branch from 68f289f to 875b808 Compare March 31, 2026 06:06
@ryanking13 ryanking13 force-pushed the gyeongjae/python-load-path branch from 875b808 to 1ee9654 Compare March 31, 2026 06:39
@ryanking13 ryanking13 merged commit 866c9c7 into main Mar 31, 2026
24 checks passed
@ryanking13 ryanking13 deleted the gyeongjae/python-load-path branch March 31, 2026 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ask-bonk ask-bonk[bot] ask-bonk[bot] left review comments

@dom96 dom96 dom96 approved these changes

@hoodmane hoodmane hoodmane approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ryanking13 @dom96 @hoodmane