feat: add pyproject.toml and uv support for Python tasks - #4468
feat: add pyproject.toml and uv support for Python tasks#4468deepshekhardas wants to merge 2 commits into
Conversation
|
|
Hi @deepshekhardas, thanks for your interest in contributing! This project requires that pull request authors are vouched, and you are not in the list of vouched users. This PR will be closed automatically. See https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md for more details. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughThe Python extension adds ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Warning |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| pyprojectFile?: string; | ||
| useUv?: boolean; |
There was a problem hiding this comment.
🟡 Python package release notes will be missing because no changeset was added
New Python build options are added to the published package (pyprojectFile/useUv at packages/python/src/extension.ts:10-11) without the required changeset entry, so the change ships with no version bump or changelog.
Impact: The feature can be released without a version bump or release-note entry for users.
Repository rule: changesets are mandatory for package changes
CONTRIBUTING.md ("Adding changesets") and CLAUDE.md both state that any change to a package under packages/* requires a changeset. git diff --name-only against the merge base shows only packages/cli-v3/test-dotenv.ts, packages/core/test/v3/zodNamespace.test.ts, and packages/python/src/extension.ts, with no file added under .changeset/.
Prompt for agents
The PR modifies the published package @trigger.dev/python but does not include a changeset. Per CONTRIBUTING.md and CLAUDE.md, run `pnpm run changeset:add` and select @trigger.dev/python (patch or minor as appropriate) so the new pyprojectFile/useUv options are versioned and appear in the changelog.
Was this helpful? React with 👍 or 👎 to provide feedback.
| import { describe, it, expect, vi } from "vitest"; | ||
| import { ZodNamespace } from "../../src/v3/zodNamespace.js"; | ||
| import { z } from "zod"; | ||
| import { Server } from "socket.io"; | ||
| import { createServer } from "node:http"; | ||
|
|
||
| describe("ZodNamespace", () => { | ||
| it("should allow sending messages with the ZodSocketMessageCatalogSchema structure", async () => { | ||
| const io = new Server(createServer()); | ||
|
|
||
| const clientMessages = { | ||
| CLIENT_MSG: { | ||
| message: z.object({ foo: z.string() }) | ||
| } | ||
| }; | ||
|
|
||
| const serverMessages = { | ||
| SERVER_MSG: { | ||
| message: z.object({ bar: z.number() }) | ||
| } | ||
| }; | ||
|
|
||
| const ns = new ZodNamespace({ | ||
| io, | ||
| name: "test", | ||
| clientMessages, | ||
| serverMessages, | ||
| }); | ||
|
|
||
| const emitSpy = vi.spyOn(ns.namespace, "emit"); | ||
|
|
||
| // This should not throw and should emit the correct payload | ||
| // Currently this might throw or require passing { message: { bar: 1 } } | ||
| await ns.sender.send("SERVER_MSG", { bar: 1 } as any); | ||
|
|
||
| expect(emitSpy).toHaveBeenCalledWith("SERVER_MSG", { | ||
| payload: { bar: 1 }, | ||
| version: "v1" | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🟡 Unrelated leftover scratch files are included in the pull request
Two files unrelated to Python build support are committed — a throwaway env-loading script (packages/cli-v3/test-dotenv.ts) and a socket namespace test (packages/core/test/v3/zodNamespace.test.ts) — so the change no longer addresses a single issue and adds a test that fails.
Impact: Reviewers get unrelated code, and the newly added test fails, turning the build red.
Why the added test fails and why the files violate repo rules
CONTRIBUTING.md: "We only accept PRs that address a single issue. Please do not submit PRs containing multiple unrelated fixes or features."
packages/core runs vitest over test/** (packages/core/package.json:167), so packages/core/test/v3/zodNamespace.test.ts executes in CI. It will fail:
ZodNamespacepasses the socket catalog ({ SERVER_MSG: { message: zodSchema } }) straight intoZodMessageSender(packages/core/src/v3/zodNamespace.ts:111-115), andsend()callsschema.safeParse(payload)on that plain object (packages/core/src/v3/zodMessageHandler.ts:273), which is not a function.- The assertion also expects
emit("SERVER_MSG", { payload, version }), but the implementation emitsemit(message.type, message.payload)(packages/core/src/v3/zodNamespace.ts:117). - Both socket.io
Serverinstances are created and never closed, leaving open handles.
Both new files also use 4-space indentation, which does not match the Prettier configuration enforced by pnpm run format (AGENTS.md "Coding style").
Prompt for agents
Remove the unrelated files packages/cli-v3/test-dotenv.ts and packages/core/test/v3/zodNamespace.test.ts from this PR. The core test additionally fails against the current implementation (ZodNamespace passes the socket catalog directly to ZodMessageSender, whose send() calls safeParse on the catalog entry, and emits (type, payload) rather than an object), and it leaks socket.io servers. If the ZodNamespace behaviour genuinely needs fixing, do it in a separate PR with tests that match repo conventions and Prettier formatting.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (this.options.pyprojectFile) { | ||
| assert( | ||
| fs.existsSync(this.options.pyprojectFile), | ||
| `pyproject.toml not found: ${this.options.pyprojectFile}` | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟡 Python dependency options can be combined and silently ignored
A project file can be supplied alongside inline or file-based requirements with no validation (pyprojectFile check at packages/python/src/extension.ts:61-66), so one of the dependency sources is silently dropped at deploy time.
Impact: Users who configure both see some of their Python dependencies never installed in the deployed image, with no warning.
Branch precedence in onBuildComplete
The constructor only asserts that requirements and requirementsFile are not both set. onBuildComplete picks exactly one branch: requirementsFile (packages/python/src/extension.ts:114), else pyprojectFile (:149), else requirements (:175). So pyprojectFile + requirementsFile installs only the requirements file, and pyprojectFile + requirements installs only the pyproject, with no log or error. The existing code warns for the requirements/requirementsFile overlap; the new option has no equivalent guard.
| if (this.options.pyprojectFile) { | |
| assert( | |
| fs.existsSync(this.options.pyprojectFile), | |
| `pyproject.toml not found: ${this.options.pyprojectFile}` | |
| ); | |
| } | |
| if (this.options.pyprojectFile) { | |
| assert( | |
| !this.options.requirementsFile, | |
| "Cannot specify both pyprojectFile and requirementsFile" | |
| ); | |
| assert( | |
| !this.options.requirements, | |
| "Cannot specify both pyprojectFile and requirements" | |
| ); | |
| assert( | |
| fs.existsSync(this.options.pyprojectFile), | |
| `pyproject.toml not found: ${this.options.pyprojectFile}` | |
| ); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| instructions: splitAndCleanComments(` | ||
| # Copy the pyproject file | ||
| COPY ${this.options.pyprojectFile} . | ||
| # Install dependencies | ||
| ${this.options.useUv ? "RUN uv pip install ." : "RUN pip install ."} | ||
| `), |
There was a problem hiding this comment.
🟡 Deploys using a Python project file can fail because only that file is copied into the image
Only the project definition file is copied before the project is installed (COPY/pip install . at packages/python/src/extension.ts:166-168), so the build backend cannot find the package sources or README it references.
Impact: Deployments configured with a Python project file can fail during image build instead of installing dependencies.
Mechanism
The layer copies just pyproject.toml into the working directory and then runs pip install . / uv pip install ., which builds the project as a distribution. Common backends (hatchling, poetry-core) error out when the declared package directory or readme file is absent (e.g. hatchling's "Unable to determine which files to ship"). If the intent is only to install the declared dependencies, a dependency-only install (e.g. uv pip install -r pyproject.toml, or uv sync/uv pip compile with a lock file) is safer, otherwise the referenced project sources must also be copied into the image.
Prompt for agents
The pyproject branch of PythonExtension.onBuildComplete copies only the pyproject.toml file into the image and then runs `pip install .` / `uv pip install .`. Building the project as a distribution requires the package sources (and any readme referenced in pyproject.toml), which are not copied, so most real projects will fail the image build. Decide whether the goal is installing only the declared dependencies (then use a dependency-only install path such as `uv pip install -r pyproject.toml`, or generate a requirements file), or installing the project itself (then also add the relevant source directories to the build context/COPY instructions). Consider also honouring uv.lock when present.
Was this helpful? React with 👍 or 👎 to provide feedback.
| RUN python3 -m venv /opt/venv | ||
| ENV PATH="/opt/venv/bin:$PATH" | ||
|
|
||
| ${this.options.useUv ? "RUN pip install uv" : ""} |
There was a problem hiding this comment.
🔍 uv target environment is not explicit
RUN pip install uv installs uv into /opt/venv and later layers call uv pip install ... with /opt/venv/bin on PATH but without VIRTUAL_ENV set and without --system. uv resolves its target environment from VIRTUAL_ENV, a .venv in the working directory, or the environment uv itself is installed into. The last case is what this relies on; if it does not apply in the builder image, every uv install step fails with "No virtual environment found". Setting ENV VIRTUAL_ENV=/opt/venv (or passing --python /opt/venv/bin/python) would make this deterministic and is worth verifying with a real deploy.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Adds pyproject.toml and uv support for Python tasks.