Skip to content
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 47 additions & 2 deletions packages/python/src/extension.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

🚩 Pre-existing: requirementsFile warning always fires spuriously

In onBuildComplete at lines 114-119, there's a warning logged when both requirementsFile and requirements are set. However, the constructor at lines 51-58 always sets this.options.requirements from the file contents when requirementsFile is provided. The assert at line 46-48 prevents both from being user-specified simultaneously. This means the warning at line 115-118 will fire every time requirementsFile is used, even though the requirements array was set by the constructor itself, not by the user. The warning message ('requirements will be ignored') is misleading in this context. This is pre-existing and not introduced by this PR.

(Refers to lines 114-119)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Contributor

Choose a reason for hiding this comment

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

🚩 Missing changeset for public package modification

The CLAUDE.md and CONTRIBUTING.md both state that modifications to any public package (packages/*) require a changeset via pnpm run changeset:add. This PR modifies packages/python/src/extension.ts (adding new pyprojectFile and useUv options) but no changeset file was added to .changeset/. This is a release management concern — without a changeset, this feature won't appear in the changelog and the package version won't be bumped on release.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ import { BuildContext, BuildExtension } from "@trigger.dev/core/v3/build";
export type PythonOptions = {
requirements?: string[];
requirementsFile?: string;
pyprojectFile?: string;
useUv?: boolean;

/**
* [Dev-only] The path to the python binary.
*
Expand Down Expand Up @@ -54,6 +57,14 @@ class PythonExtension implements BuildExtension {
fs.readFileSync(this.options.requirementsFile, "utf-8")
);
}

if (this.options.pyprojectFile) {
assert(
fs.existsSync(this.options.pyprojectFile),
`pyproject.toml not found: ${this.options.pyprojectFile}`
);
}
Comment on lines +61 to +66
Copy link
Contributor

Choose a reason for hiding this comment

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

🔴 Missing mutual exclusion validation for pyprojectFile against requirements/requirementsFile

The constructor validates that requirements and requirementsFile cannot both be specified (line 46-48), but the new pyprojectFile option has no such validation. A user can pass { pyprojectFile: "pyproject.toml", requirements: ["numpy"] } or { pyprojectFile: "pyproject.toml", requirementsFile: "requirements.txt" } without error. In onBuildComplete, the if/else if/else if chain at lines 114-199 silently picks one option and ignores the other — requirementsFile wins over pyprojectFile, which wins over requirements. The user gets no error or warning that their configuration is conflicting and one dependency source is being dropped.

Prompt for agents
In packages/python/src/extension.ts, in the constructor (around lines 45-66), add assertions that prevent pyprojectFile from being used together with requirements or requirementsFile. Before the existing assert on line 46, or after it, add:

assert(
  !(this.options.pyprojectFile && this.options.requirements),
  "Cannot specify both pyprojectFile and requirements"
);
assert(
  !(this.options.pyprojectFile && this.options.requirementsFile),
  "Cannot specify both pyprojectFile and requirementsFile"
);

This matches the existing validation pattern at lines 46-48 and ensures users get a clear error when they provide conflicting dependency sources.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.


}

async onBuildComplete(context: BuildContext, manifest: BuildManifest) {
Expand Down Expand Up @@ -88,6 +99,8 @@ class PythonExtension implements BuildExtension {
# Set up Python environment
RUN python3 -m venv /opt/venv
ENV PATH="/opt/venv/bin:$PATH"

${this.options.useUv ? "RUN pip install uv" : ""}
`),
},
deploy: {
Expand Down Expand Up @@ -123,7 +136,36 @@ class PythonExtension implements BuildExtension {
# Copy the requirements file
COPY ${this.options.requirementsFile} .
# Install dependencies
RUN pip install --no-cache-dir -r ${this.options.requirementsFile}
${this.options.useUv
? `RUN uv pip install --no-cache -r ${this.options.requirementsFile}`
: `RUN pip install --no-cache-dir -r ${this.options.requirementsFile}`
}
`),
},
deploy: {
override: true,
},
});
} else if (this.options.pyprojectFile) {
// Copy pyproject file to the container
await addAdditionalFilesToBuild(
"pythonExtension",
{
files: [this.options.pyprojectFile],
},
context,
manifest
);

// Add a layer to the build that installs the dependencies
context.addLayer({
id: "python-dependencies",
image: {
instructions: splitAndCleanComments(`
# Copy the pyproject file
COPY ${this.options.pyprojectFile} .
# Install dependencies
${this.options.useUv ? "RUN uv pip install ." : "RUN pip install ."}
`),
},
deploy: {
Comment on lines +160 to 171
Copy link
Contributor

Choose a reason for hiding this comment

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

🚩 pip install . with only pyproject.toml may fail at build time

When pyprojectFile is used (lines 149-174), only the pyproject.toml file is copied into the container via addAdditionalFilesToBuild, and then pip install . (or uv pip install .) is executed. The pip install . command attempts to build and install the current directory as a Python package, which typically requires the full source tree (not just pyproject.toml). If the pyproject.toml only declares dependencies without a buildable package, or if the source files aren't present, the build will fail. Users would need to also use the scripts option to copy their Python source files for this to work. This isn't strictly a code bug since the Docker build would fail with a clear error, but it may warrant documentation or a different pip invocation (e.g., pip install with dependency extraction from pyproject.toml).

(Refers to lines 160-174)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Expand All @@ -144,7 +186,10 @@ class PythonExtension implements BuildExtension {
RUN echo "$REQUIREMENTS_CONTENT" > requirements.txt

# Install dependencies
RUN pip install --no-cache-dir -r requirements.txt
${this.options.useUv
? "RUN uv pip install --no-cache -r requirements.txt"
: "RUN pip install --no-cache-dir -r requirements.txt"
}
`),
},
deploy: {
Expand Down