Minimal python package support#1881
Conversation
This is the beginning of work on this feature, it's a little comment-heavy to that end and has some patterns that may not be permanent (these have generally been noted as such). As of this commit, there are tests that start to poke at this, namely the one that brings in fastapi, which currently fails due to an extension it doesn't like.
Proof of concept for package support in dynamic Python workers. As of this commit, it can retrieve an sdist with a flat layout from pypi and install it into the virtual filesystem. It doesn't do dependency resolution, so the current tests fail when attempting to import FastAPI's deps.
Used sdists before, now it uses wheels. Pre cleanup commit
|
This test only needs to confirm that retrieval of a hard list of pure python packages, without dependency resolution, works.
This seems to be specific to JS packages
This dependendency caused issues when working in a container, but its removal was purely for my sake This reverts commit 4f8990d.
Also makes PyprojectToml interface less stringent
These got caught by the checks done by `pnpm run check`
agents
@cloudflare/ai-chat
@cloudflare/codemode
create-think
hono-agents
@cloudflare/shell
@cloudflare/think
@cloudflare/voice
@cloudflare/worker-bundler
commit: |
| for (const path of fileSystem.list()) { | ||
| if (path !== "pyproject.toml") { | ||
| const content = fileSystem.read(path); | ||
| if (content !== null) { | ||
| modules[path] = content; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🔍 Python modules output includes all filesystem files, potentially including node_modules
The Python worker path at packages/worker-bundler/src/index.ts:148-155 iterates over all files in the filesystem (except pyproject.toml) and includes them in the output modules. If a project has both package.json with npm dependencies and a Python entry point, the installed node_modules/ tree would be included in the modules output. This could result in a very large modules object being passed to the Worker Loader. Consider filtering out node_modules/ and python_modules/ metadata directories.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
These are just files specified by the user in the createWorker call, right? So this AI review comment seems wrong.
There was a problem hiding this comment.
At that point it should be those files + all the packages that were just added, so my assumption is anything that's in there would've been put there deliberately
dom96
left a comment
There was a problem hiding this comment.
Overall approach looks good! Left a few comments with suggestions.
CC @hoodmane @ryanking13 @threepointone for reviews too
| for (const path of fileSystem.list()) { | ||
| if (path !== "pyproject.toml") { | ||
| const content = fileSystem.read(path); | ||
| if (content !== null) { | ||
| modules[path] = content; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
These are just files specified by the user in the createWorker call, right? So this AI review comment seems wrong.
These are already set implicitly by createWorker
There was a problem hiding this comment.
I left a few comments, but it looks like a good starting point.
Not all of my reviews need to be addressed in this PR. Some of my comments are for FYI, as you'll eventually handle them. If you consider some of my comments as out-of-scope of this PR and would want to hand it in the follow up PRs (which is a good practice), then please add a comment about it.
| mainModule: entryPoint, | ||
| modules, | ||
| wranglerConfig: { | ||
| compatibilityDate: wranglerConfig?.compatibilityDate ?? "2026-01-01", |
There was a problem hiding this comment.
How is this default compat date selected? How does the JS worker handle this?
There was a problem hiding this comment.
I recall just choosing it as an arbitrary date that I thought would work, I can put a newer one. The JS worker as in the worker that creates the dynamic worker?
There was a problem hiding this comment.
Yeah, following whatever the JS worker is doing would be a good idea.
| modules, | ||
| wranglerConfig: { | ||
| compatibilityDate: wranglerConfig?.compatibilityDate ?? "2026-01-01", | ||
| compatibilityFlags: wranglerConfig?.compatibilityFlags ?? [ |
There was a problem hiding this comment.
We will always need to add python_workers flag to make the worker work. So maybe we need to inject one if this flag does not exist?
There was a problem hiding this comment.
As in to the user-defined compatibility flags, in the event the user passed flags that didn't include python_workers? I can add that
There was a problem hiding this comment.
Actually, that should no longer be needed for Python dynamic workers. You can run python dynamic workers without that flag.
| */ | ||
|
|
||
| import * as semver from "semver"; | ||
| import { unzipSync } from "fflate"; |
There was a problem hiding this comment.
We do have CompressionStream support in CF worker. fflate claims that the performance of CompressionSTream is bad in the browser, but that might not be true.
you don't need to use CompressionStream in this PR as it is an implementation detail that we can change anytime and the performance is not our concern at this point, but it would be nice to add a comment in the code why you decided to use this package and things that need to be checked (performance compared to the CompressionStream) later.
There was a problem hiding this comment.
I actually only did it this way because I dryly assumed I'd need a lib for it, I'd much rather save a dependency so I'm glad you told me that
| const response = await fetchWithTimeout( | ||
| wheelUrl, | ||
| {}, | ||
| DEFAULT_TIMEOUT_MS * 2 |
There was a problem hiding this comment.
Just curious, why not double the DEFAULT_TIMEOUT_MS itself?
There was a problem hiding this comment.
Mostly because it's not clear to me whether that would have undesirable effects elsewhere, there are other places where this function is called with the default duration. I ended up just staying with what the JS one was doing
| // Skip wheel metadata and data directories | ||
| if (path.includes(".dist-info/") || path.includes(".data/")) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Why? These are important files used in runtime as well.
|
|
||
| async function fetchPythonPackageMetadata(name: string, registry: string) { | ||
| // Fetch package metadata from PyPI JSON API | ||
| const metadataResponse = await fetchWithTimeout(`${registry}/${name}/json`); |
There was a problem hiding this comment.
This is a legacy API endpoint that only PyPI uses. There is a PEP 691 which is a standard now, so I would recommend to use that instead.
There was a problem hiding this comment.
Is it easy to move to the new endpoints? Like is it just a URL change? If not we can do this in a follow up (so a TODO comment is fine here IMO)
There was a problem hiding this comment.
I did end up starting this one on a separate branch, will add a todo
There was a problem hiding this comment.
A little bit of URL change + json handling. You can start with this legacy api and switch to the PEP 691 one in a follow up PR (note that the Pyodide index (index.pyodide.org) does not support this endpoint)
Remove check for when there are no dependencies, since there will always be at least one
abstractedfox
left a comment
There was a problem hiding this comment.
Will have some more commits in for changes requested by @ryanking13, though I may want to save some of them for a future PR if that's alright
| mainModule: entryPoint, | ||
| modules, | ||
| wranglerConfig: { | ||
| compatibilityDate: wranglerConfig?.compatibilityDate ?? "2026-01-01", |
There was a problem hiding this comment.
I recall just choosing it as an arbitrary date that I thought would work, I can put a newer one. The JS worker as in the worker that creates the dynamic worker?
| modules, | ||
| wranglerConfig: { | ||
| compatibilityDate: wranglerConfig?.compatibilityDate ?? "2026-01-01", | ||
| compatibilityFlags: wranglerConfig?.compatibilityFlags ?? [ |
There was a problem hiding this comment.
As in to the user-defined compatibility flags, in the event the user passed flags that didn't include python_workers? I can add that
| */ | ||
|
|
||
| import * as semver from "semver"; | ||
| import { unzipSync } from "fflate"; |
There was a problem hiding this comment.
I actually only did it this way because I dryly assumed I'd need a lib for it, I'd much rather save a dependency so I'm glad you told me that
| installedPackages, | ||
| inProgress, | ||
| registry | ||
| PYPI_JSON_API // hardcoding this for now to keep the implementation light |
There was a problem hiding this comment.
ahh thanks, that actually answers a question that was on my mind this morning. I may want to do that in an upcoming pr if that's alright with you
| } | ||
|
|
||
| // Skip if the package already exists in the filesystem | ||
| if (fileSystem.read(`python_modules/${name}/__init__.py`) !== null) { |
There was a problem hiding this comment.
ah, thanks for pointing that out, I realized distribution packages could have differently named packages inside them while working in another branch but I didn't change this there either (though I didn't know packages could not have __init__.py). I more or less did what the JS implementation was doing here, a snap assumption was that it was to prevent races in some way but the comment for the JS one says it's done in case installDependencies is called on a 'pre-warmed filesystem', though I don't know how often this actually happens
| const response = await fetchWithTimeout( | ||
| wheelUrl, | ||
| {}, | ||
| DEFAULT_TIMEOUT_MS * 2 |
There was a problem hiding this comment.
Mostly because it's not clear to me whether that would have undesirable effects elsewhere, there are other places where this function is called with the default duration. I ended up just staying with what the JS one was doing
| `No wheel distribution found for ${name}@${version} on PyPI` | ||
| ); | ||
| } | ||
| const wheelUrl = wheel.url; |
There was a problem hiding this comment.
Ah thank you, I was thinking earlier about what I'd need to do about platform specific wheels
dom96
left a comment
There was a problem hiding this comment.
A few more comments and things that I think should be fixed before merge, but overall looks good to me so approving.
| modules, | ||
| wranglerConfig: { | ||
| compatibilityDate: wranglerConfig?.compatibilityDate ?? "2026-01-01", | ||
| compatibilityFlags: wranglerConfig?.compatibilityFlags ?? [ |
| if (path === "pyproject.toml") { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Is this necessary? No one is likely to be specifying pyproject.toml in the createWorker call
There was a problem hiding this comment.
@dom96 This is the pyproject.toml that contains all the deps for the worker, so as far as I understand we do need to have it coming into createWorker
There was a problem hiding this comment.
Ahh, my mistake, for some reason I thought pyproject.toml wasn't in the "files" dict, but one level above (same for packages.json, but I just checked and it's also in the "files" dict).
All good to leave this then.
| } | ||
|
|
||
| // Skip if the package already exists in the filesystem | ||
| if (fileSystem.read(`python_modules/${name}/__init__.py`) !== null) { |
There was a problem hiding this comment.
With @ryanking13's comment below, perhaps the best thing to do here is to just delete this and rely on the installedPackages check instead. You can add a TODO to consider doing this in the future because the JS is doing it.
| } | ||
|
|
||
| // Skip if the package already exists in the filesystem | ||
| if (fileSystem.read(`python_modules/${name}/__init__.py`) !== null) { |
There was a problem hiding this comment.
| `No wheel distribution found for ${name}@${version} on PyPI` | ||
| ); | ||
| } | ||
| const wheelUrl = wheel.url; |
There was a problem hiding this comment.
I think this kind of thing can be pushed to a TODO comment and done in separate PRs. Let's iterate via follow up PRs rather than ensuring everything is 100% in this PR, especially when it comes to Python packaging details.
|
|
||
| async function fetchPythonPackageMetadata(name: string, registry: string) { | ||
| // Fetch package metadata from PyPI JSON API | ||
| const metadataResponse = await fetchWithTimeout(`${registry}/${name}/json`); |
There was a problem hiding this comment.
Is it easy to move to the new endpoints? Like is it just a URL change? If not we can do this in a follow up (so a TODO comment is fine here IMO)
This is implicit on Python dynamic workers now
…the result object
…oml and package.json in one worker
abstractedfox
left a comment
There was a problem hiding this comment.
One last question in the comments but I think this is otherwise just about there!
| if (path === "pyproject.toml") { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
@dom96 This is the pyproject.toml that contains all the deps for the worker, so as far as I understand we do need to have it coming into createWorker
|
|
||
| async function fetchPythonPackageMetadata(name: string, registry: string) { | ||
| // Fetch package metadata from PyPI JSON API | ||
| const metadataResponse = await fetchWithTimeout(`${registry}/${name}/json`); |
There was a problem hiding this comment.
I did end up starting this one on a separate branch, will add a todo
|
cc: @threepointone The python workers team doesn't seem to be possible to approve and merge this. Can you help with this? |
|
So if this PR got merged, dynamic worker can be a full featured python code runner, no need to use docker based sandbox right? I don't like the docker sandbox because it's imcompatitable with vitest-pool-workers which make writing tests very hard (need to start real server to connect to docker). |
|
@xcaptain This is still experimental so the answer is yes and no. I would say you should not rely on this yet (or you can volunteer to help us debugging things :) ).
Also, yes and no. It is true that you can use dynamic workers with Python, but it you need to run a workload that is computation / memory heavy, then it is not a good fit for worker. So it depends on your workload |
This implements the beginning of package support for dynamic python workers. At present, it allows defining a flat list of pure Python packages as dependencies, which it installs in the dynamic worker without dependency resolution