Repository navigation
Clean up Python pool-related codes and remove emscriptenSetup from Pyodide bundle - #8360
ryanking13 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
💬 Approved with comments — The refactor is clean. One WARNING about unrelated lockfile changes that downgrade hermetic_launcher; no blocking issues.
Findings: 1 warning (1 is an inline comment on the diff)
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
🔧 Fix with your agent
Paste the following into your coding agent to investigate and fix these issues.
Fix the following code review issues found in pull request #8360.
Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order
## WARNING (1)
### [Code Quality] Unrelated MODULE.bazel.lock changes downgrade hermetic_launcher (0.0.17 to 0.0.16) and bump aspect_tools_telemetry, with no MODULE.bazel change behind them.
- **File:** MODULE.bazel.lock:145
📋 Code Quality: ⚠️ 1 warning
The refactor is coherent; the lockfile carries unrelated dependency changes, including a downgrade.
Recommendation: 📝 Minor Comments
- [WARNING] Unrelated MODULE.bazel.lock changes downgrade hermetic_launcher (0.0.17 to 0.0.16) and bump aspect_tools_telemetry, with no MODULE.bazel change behind them. (
MODULE.bazel.lock:145)
View full review
MODULE.bazel.lock: the lock replaceshermetic_launcher0.0.17 with 0.0.16 andaspect_tools_telemetry0.5.1 with 0.5.2. This pull request changes noMODULE.bazelor dependency declarations, so these entries look like drift from regenerating the lock locally. Revert the file or regenerate it against the target branch.src/pyodide/internal/util.ts:9-20andsrc/pyodide/internal/python.ts:52: the comments and TODOs that explain thesetInternalErrorReporterindirection still refer to pool bundling, which no longer exists. Consider removing the indirection, or at least updating the comments.
✅ All other checks passed
🔒 Security: ✅ LGTM
No security issues found.
📖 AGENTS.md: ✅ LGTM
No issues found.
⚡ Performance: ✅ LGTM
No performance issues found.
📝 Documentation: ✅ LGTM
The src/pyodide/AGENTS.md updates match the new layout.
Reviewed commit: e2bfdea1
| "https://bcr.bazel.build/modules/googletest/1.18.0/source.json": "cbf8951f03dcff3750c0265b431bb6245e91cf327a2c7284e896acefd1b87b68", | ||
| "https://bcr.bazel.build/modules/hermetic_launcher/0.0.17/MODULE.bazel": "afa67883ae04001c93578f80c39e5ec9231ae670978bc380778baf66e1936ad5", | ||
| "https://bcr.bazel.build/modules/hermetic_launcher/0.0.17/source.json": "46ee01ccb275e46f59a3c070f91be52eb113405efe577a99365dd69f168713ef", | ||
| "https://bcr.bazel.build/modules/hermetic_launcher/0.0.16/MODULE.bazel": "ac8a135156c67b45fff31ca11db0faced1d61890654b96d2c0ea6e8a532a3d9c", |
There was a problem hiding this comment.
Unrelated MODULE.bazel.lock changes downgrade hermetic_launcher (0.0.17 to 0.0.16) and bump aspect_tools_telemetry, with no MODULE.bazel change behind them.
This pull request changes no MODULE.bazel or dependency declarations, yet the lock now resolves hermetic_launcher 0.0.16 instead of 0.0.17. It also moves aspect_tools_telemetry to 0.5.2. Anyone who builds this branch gets a different version of a transitive build dependency of aspect_rules_js for reasons unrelated to the Pyodide cleanup. This looks like lockfile drift from a local regeneration. Revert the lock to the base content, or regenerate it against the target branch's MODULE.bazel.
| // Suppress warning about CommonJS 'module' variable in pyodide.asm.js (ES module context) | ||
| // The pyodide.asm.js file contains embedded libraries (like MiniLZ4) that use CommonJS | ||
| // style module.exports checks for compatibility. These are safe to ignore as they won't | ||
| // execute in our ES module context. |
There was a problem hiding this comment.
| // Suppress warning about CommonJS 'module' variable in pyodide.asm.js (ES module context) | |
| // The pyodide.asm.js file contains embedded libraries (like MiniLZ4) that use CommonJS | |
| // style module.exports checks for compatibility. These are safe to ignore as they won't | |
| // execute in our ES module context. | |
| // Suppress warning about CommonJS 'module' variable in pyodide.asm.js |
On top of #8229.
This PR removes Python pool related codes
/internal/pool, which we didn't implement in the end. This gives us a following improvement:Previously,
emscriptenSetup.jswas bundled into the Pyodide bundle, and any changes there required Pyodide bundle update. Now they are linked statically into the workerd binary, so we don't need to update the bundle except for when we (1) update Pyodide version, or (2) when we add a new asm.js patch.