-
-
Notifications
You must be signed in to change notification settings - Fork 957
bug/python non top level requirements #2470
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
bug/python non top level requirements #2470
Conversation
@julienvanbeveren
julienvanbeveren
commented
Sep 3, 2025
- 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
- Added .changeset/eleven-months-brake.md declaring a patch release for @trigger.dev/python with a note about a bug fix related to specifying a requirements file.
- Updated packages/python/src/extension.ts: in the python-dependencies layer, changed the Docker COPY destination for the requirements file from the build context root (.) to the same path as the source file (${this.options.requirementsFile}), leaving the pip install command unchanged.
Estimated code review effort
🎯 2 (Simple) | ⏱️ ~10 minutes
✨ Finishing Touches
- 📝 Generate Docstrings
🧪 Generate unit tests
- Create PR with unit tests
- Post copyable unit tests in a comment
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.
🪧 Tips
Chat
There are 3 ways to chat with CodeRabbit:
- Review comments: Directly reply to a review comment made by CodeRabbit. Example:
I pushed a fix in commit <commit_id>, please review it.Open a follow-up GitHub issue for this discussion.
- Files and specific lines of code (under the "Files changed" tab): Tag
@coderabbitaiin a new review comment at the desired location with your query. - PR comments: Tag
@coderabbitaiin a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
Support
Need help? Create a ticket on our support page for assistance with any issues or questions.
CodeRabbit Commands (Invoked using PR/Issue comments)
Type @coderabbitai help to get the list of available commands.
Other keywords and placeholders
- Add
@coderabbitai ignoreor@coderabbit ignoreanywhere in the PR description to prevent this PR from being reviewed. - Add
@coderabbitai summaryto generate the high-level summary at a specific location in the PR description. - Add
@coderabbitaianywhere in the PR title to generate the title automatically.
CodeRabbit Configuration File (.coderabbit.yaml)
- You can programmatically configure CodeRabbit by adding a
.coderabbit.yamlfile to the root of your repository. - Please see the configuration documentation for more information.
- If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation:
# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json
Status, Documentation and Community
- Visit our Status Page to check the current availability of CodeRabbit.
- Visit our Documentation for detailed information on how to use CodeRabbit.
- Join our Discord Community to get help, request features, and share feedback.
- Follow us on X/Twitter for updates and announcements.
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.
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月10日T10:56:31.402Z
Learnt from: zvictor
PR: triggerdotdev/trigger.dev#1686
File: packages/build/src/extensions/python.ts:110-116
Timestamp: 2025年02月10日T10: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
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.
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.
🤖 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.
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.
🛠️ 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>").
ericallam
commented
Sep 3, 2025
@julienvanbeveren Thanks for the contribution! Were you able to test this change?
julienvanbeveren
commented
Sep 5, 2025
@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.