bug/python non top level requirements #2470
Conversation
julienvanbeveren
commented
Sep 3, 2025
julienvanbeveren
commented
- fix support for requirements file at any depth
- add changelog
🦋 Changeset detectedLatest commit: b3af2aa The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Walkthrough
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/python/src/extension.ts (1)
48-56: Avoid false warning when only requirementsFile is providedYou populate
this.options.requirementsfrom the file in the constructor, which later triggers the “both specified” warning even when the user only setrequirementsFile. Remove the assignment.if (this.options.requirementsFile) { assert( fs.existsSync(this.options.requirementsFile), `Requirements file not found: ${this.options.requirementsFile}` ); - this.options.requirements = splitAndCleanComments( - fs.readFileSync(this.options.requirementsFile, "utf-8") - ); }Optional: if you still need parsed contents later, store them in a private field (e.g.,
this._requirementsFromFile) and do not useoptions.requirementsfor that.Also applies to: 101-107
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.changeset/eleven-months-brake.md(1 hunks)packages/python/src/extension.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
packages/python/src/extension.ts
🧠 Learnings (1)
📚 Learning: 2025-02-10T10:56:31.402Z
Learnt from: zvictor
PR: triggerdotdev/trigger.dev#1686
File: packages/build/src/extensions/python.ts:110-116
Timestamp: 2025-02-10T10:56:31.402Z
Learning: In Docker build contexts for Trigger.dev extensions, avoid over-engineering security measures when handling user-provided configuration (like Python requirements) as the build context is already isolated and the content is user-controlled.
Applied to files:
packages/python/src/extension.ts
| "@trigger.dev/python": patch | ||
| --- | ||
|
|
||
| Fixed the bug where you can only specify the requirements file as top level |
There was a problem hiding this comment.
Polish changelog phrasing (“top‑level”)
Tighten wording and fix the hyphenation.
-Fixed the bug where you can only specify the requirements file as top level
+Fixed a bug that required the requirements file to be top‑level; nested paths are now supported.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Fixed the bug where you can only specify the requirements file as top level | |
| Fixed a bug that required the requirements file to be top-level; nested paths are now supported. |
🤖 Prompt for AI Agents
In .changeset/eleven-months-brake.md around line 5, update the changelog
sentence to tighten wording and use the hyphenated term "top‑level"; replace the
current line ("Fixed the bug where you can only specify the requirements file as
top level") with a clearer phrase such as "Fixed the bug where the requirements
file could only be specified at the top‑level." Ensure the hyphenation is the
non‑breaking hyphen (top‑level) and adjust punctuation to match the file's
style.
| COPY ${this.options.requirementsFile} ${this.options.requirementsFile} | ||
| # Install dependencies | ||
| RUN pip install --no-cache-dir -r ${this.options.requirementsFile} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use JSON-array COPY and quote the pip path; normalize for Windows backslashes
Prevents failures when the requirements path contains spaces or backslashes and makes COPY unambiguous.
- COPY ${this.options.requirementsFile} ${this.options.requirementsFile}
+ COPY ["${this.options.requirementsFile.replace(/\\\\/g, "/")}", "${this.options.requirementsFile.replace(/\\\\/g, "/")}"]
# Install dependencies
- RUN pip install --no-cache-dir -r ${this.options.requirementsFile}
+ RUN pip install --no-cache-dir -r "${this.options.requirementsFile.replace(/\\\\/g, "/")}"If you prefer readability, precompute the normalized path once and reuse it:
// above, before context.addLayer(...)
const reqFile = this.options.requirementsFile!.replace(/\\/g, "/");and then:
- COPY ["${this.options.requirementsFile.replace(/\\\\/g, "/")}", "${this.options.requirementsFile.replace(/\\\\/g, "/")}"]
+ COPY ["${reqFile}", "${reqFile}"]
- RUN pip install --no-cache-dir -r "${this.options.requirementsFile.replace(/\\\\/g, "/")}"
+ RUN pip install --no-cache-dir -r "${reqFile}"🤖 Prompt for AI Agents
In packages/python/src/extension.ts around lines 124 to 126, the Dockerfile COPY
and RUN lines can break when requirementsFile contains spaces or Windows
backslashes; normalize the path (replace backslashes with "/") once into a
variable (e.g. reqFile) and then use the JSON-array form for COPY and quote the
pip path in RUN so the COPY is unambiguous and pip receives a quoted, normalized
path (i.e., use COPY ["<reqFile>","<reqFile>"] and RUN pip install
--no-cache-dir -r "<reqFile>").
|
@julienvanbeveren Thanks for the contribution! Were you able to test this change? |
|
@ericallam No, how do you recommend this to be tested / what is the setup for good testing? I would love to test it and finalize the PR. |
