-
Notifications
You must be signed in to change notification settings - Fork 634
Minimal python package support #1881
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
base: main
Are you sure you want to change the base?
Changes from 27 commits
4f8990d
3b8ea8f
96153da
d651034
958f232
98ad2f7
e01a96e
2fd4db5
248a149
2ca7e73
58e6874
1cf3db3
ceda163
7244ec5
493cf55
4fa5fce
bf8d3a9
6b365dd
7a2a85f
ecbd776
8016a35
766e58c
40cbc98
f3289dc
6fde2e0
b711067
4c56501
7ade11e
ca0ce79
1a68186
bfcdec1
58b0743
a088712
691e4cb
4740d96
dcdd6fd
d6fd735
ee05c75
75fdf12
79293af
ae50567
5d45c40
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,7 @@ import { bundleWithEsbuild, bundlerOnlyOptionsWarning } from "./bundler"; | |
| import { hasNodejsCompat, parseWranglerConfig } from "./config"; | ||
| import { hasDependencies, installDependencies } from "./installer"; | ||
| import { transformAndResolve } from "./transformer"; | ||
| import type { CreateWorkerOptions, CreateWorkerResult } from "./types"; | ||
| import type { CreateWorkerOptions, CreateWorkerResult, Modules } from "./types"; | ||
| import { | ||
| DEFAULT_ENTRY_POINTS, | ||
| detectEntryPoint, | ||
|
|
@@ -117,7 +117,7 @@ export async function createWorker( | |
| const wranglerConfig = parseWranglerConfig(fileSystem); | ||
| const nodejsCompat = hasNodejsCompat(wranglerConfig); | ||
|
|
||
| // Auto-install dependencies if package.json has dependencies | ||
| // Auto-install dependencies if package.json or pyproject.toml has dependencies | ||
| const installWarnings: string[] = []; | ||
| if (hasDependencies(fileSystem)) { | ||
| const installResult = await installDependencies( | ||
|
|
@@ -143,6 +143,30 @@ export async function createWorker( | |
| ); | ||
| } | ||
|
|
||
| if (entryPoint.endsWith(".py")) { | ||
| const modules: Modules = {}; | ||
| for (const path of fileSystem.list()) { | ||
| if (path === "pyproject.toml") { | ||
| continue; | ||
| } | ||
| const content = fileSystem.read(path); | ||
| if (content !== null) { | ||
| modules[path] = content; | ||
| } | ||
| } | ||
|
Comment on lines
+148
to
+156
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔍 Python modules output includes all filesystem files, potentially including node_modules The Python worker path at Was this helpful? React with 👍 or 👎 to provide feedback. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are just files specified by the user in the createWorker call, right? So this AI review comment seems wrong.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
|
||
| return { | ||
| mainModule: entryPoint, | ||
| modules, | ||
| wranglerConfig: { | ||
| compatibilityDate: wranglerConfig?.compatibilityDate ?? "2026-01-01", | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How is this default compat date selected? How does the JS worker handle this?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, following whatever the JS worker is doing would be a good idea. |
||
| compatibilityFlags: wranglerConfig?.compatibilityFlags ?? [ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We will always need to add
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As in to the user-defined compatibility flags, in the event the user passed flags that didn't include There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, that should no longer be needed for Python dynamic workers. You can run python dynamic workers without that flag.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dom96 Should I remove it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep |
||
| "python_workers" | ||
| ] | ||
| } | ||
| }; | ||
|
abstractedfox marked this conversation as resolved.
|
||
| } | ||
|
|
||
| if (bundle) { | ||
| // Try bundling with esbuild-wasm | ||
| const result = await bundleWithEsbuild({ | ||
|
|
@@ -171,7 +195,6 @@ export async function createWorker( | |
| if (installWarnings.length > 0) { | ||
| result.warnings = [...(result.warnings ?? []), ...installWarnings]; | ||
| } | ||
|
|
||
| return result; | ||
| } else { | ||
| // No bundling - transform files and resolve dependencies. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? No one is likely to be specifying pyproject.toml in the
createWorkercallThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.