feat: add pyproject.toml and uv support for Python tasks#3181
feat: add pyproject.toml and uv support for Python tasks#3181deepshekhardas wants to merge 2 commits intotriggerdotdev:mainfrom
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 Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR adds test coverage for environment variable resolution and ZodNamespace functionality, and extends Python extension configuration. The changes include a new test validating that .env.local overrides .env variables during resolution, a new test verifying ZodNamespace message emission and callback schema support, and Python extension modifications introducing pyproject.toml-based configuration alongside optional uv package manager integration including new validation and conditional build steps. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚩 test-dotenv.ts is an ad-hoc script, not a vitest test
This file is committed as a standalone executable script at the package root rather than as a proper vitest test file. The repo's CLAUDE.md states 'We use vitest exclusively' and AGENTS.md says 'Test files live beside the files under test and use descriptive describe and it blocks.' This file uses console.log assertions and process.exit instead of vitest's describe/it/expect. Consider converting this to a proper vitest test in a location next to packages/cli-v3/src/utilities/dotEnv.ts, or removing it if it was only intended as a temporary debugging aid.
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.
🟡 Missing mutual exclusion validation for pyprojectFile with requirements/requirementsFile
The constructor at packages/python/src/extension.ts:46-48 validates that requirements and requirementsFile can't both be specified, but no equivalent assertion is added for pyprojectFile. If a user provides both pyprojectFile and requirements (or requirementsFile), one is silently ignored due to the if/else if chain in onBuildComplete (packages/python/src/extension.ts:114-198). For example, { pyprojectFile: "pyproject.toml", requirements: ["numpy"] } silently drops the requirements with no warning, which is inconsistent with the existing validation pattern that explicitly rejects conflicting options.
| if (this.options.pyprojectFile) { | |
| assert( | |
| fs.existsSync(this.options.pyprojectFile), | |
| `pyproject.toml not found: ${this.options.pyprojectFile}` | |
| ); | |
| } | |
| if (this.options.pyprojectFile) { | |
| assert( | |
| !(this.options.requirements || this.options.requirementsFile), | |
| "Cannot specify pyprojectFile together with requirements or requirementsFile" | |
| ); | |
| assert( | |
| fs.existsSync(this.options.pyprojectFile), | |
| `pyproject.toml not found: ${this.options.pyprojectFile}` | |
| ); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| console.log("✅ SUCCESS: .env.local overrides .env"); | ||
| } else { | ||
| console.log("❌ FAILURE: .env.local did NOT override .env. Got:", vars.TEST_VAR); | ||
| process.exit(1); |
There was a problem hiding this comment.
🟡 Temp directory not cleaned up when test fails
process.exit(1) on line 19 terminates the process before the cleanup call fs.rmSync(tempDir, ...) on line 22 executes, leaving the temp-env-test directory behind on disk whenever the assertion fails. The cleanup should happen before process.exit.
| process.exit(1); | |
| fs.rmSync(tempDir, { recursive: true, force: true }); | |
| process.exit(1); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| 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 ."} |
There was a problem hiding this comment.
🚩 pip install . with only pyproject.toml may fail at Docker build time
The pyprojectFile branch at packages/python/src/extension.ts:149-174 copies only the pyproject.toml into the container and runs pip install . (or uv pip install .). The pip install . command attempts to build and install the package in the current directory, which typically requires more than just a pyproject.toml — it needs the actual source code and a valid build system definition. If the user's pyproject.toml defines a package (with [build-system], name, etc.), the build step will fail because the source files aren't present. This would only work if the user also uses the scripts option to copy their source files, which is not documented or enforced. Worth verifying the intended usage pattern with the author — this could be a design-level issue rather than a simple code bug.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| // 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); |
There was a problem hiding this comment.
🚩 zodNamespace test uses as any casts and may not test real behavior
The test at packages/core/test/v3/zodNamespace.test.ts:34 uses as any to bypass type checking when calling ns.sender.send("SERVER_MSG", { bar: 1 } as any), and the second test at line 58 casts serverMessages as any. The comments acknowledge this: 'Cast for now until we update the types' and 'Currently this might throw or require passing { message: { bar: 1 } }'. These tests may not actually verify correct behavior — they may be aspirational tests for a future API shape rather than tests of current behavior. If so, they could be misleading or flaky.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Added support for parsing pyproject.toml and using the uv package manager for faster Python dependency installation.