feat(build): Add support for Python scripts via pythonExtension
#1686
Conversation
🦋 Changeset detectedLatest commit: 7eddd20 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 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 |
WalkthroughThis pull request introduces a new Python extension integrated into the build process. It adds support for executing Python scripts with enhanced error handling and environment configuration. Updates include modifications to the build configuration and README files, as well as establishing a new Python package with its own changelog, license, documentation, runtime functions, and TypeScript settings. Changes
Sequence Diagram(s)sequenceDiagram
participant B as Build Process
participant PE as PythonExtension
participant FS as File System
participant Env as Environment
B->>PE: onBuildComplete(context, manifest)
alt Build target is "dev"
PE-->>Env: Set PYTHON_BIN_PATH (if provided)
else Other target
PE->>FS: Validate & read requirements file
PE->>Env: Add layer for Python install & virtual env setup
PE->>Env: Add layer for dependency installation
end
sequenceDiagram
participant U as User
participant RI as runInline
participant RS as runScript
participant R as run
U->>RI: Call runInline(python code)
RI->>RI: Create temporary file
RI->>RS: Invoke runScript(temp file)
RS->>R: Execute Python command with arguments
R-->>RS: Return output/error
RS-->>RI: Propagate result
RI-->>U: Return final output and cleanup temporary file
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 1
🧹 Nitpick comments (5)
packages/build/src/extensions/python.ts (5)
8-21: Add doc comments for other properties
While pythonBinaryPath has helpful documentation, consider briefly documenting how "requirements" and "requirementsFile" are expected to be used for added clarity.
30-41: Filter out blank lines or comment lines
When reading requirements from the file, consider filtering out empty or comment lines to avoid unexpected installation errors if the file has whitespace or commented lines.You could do something like this:
this.options.requirements = fs .readFileSync(this.options.requirementsFile, "utf-8") - .split("\n"); + .split("\n") + .map(line => line.trim()) + .filter(line => line && !line.startsWith("#"));
43-50: Consider logging in development mode
Adding a small log statement clarifying that Python installation steps are skipped in development mode can help with visibility and troubleshooting.
52-86: Optimize Docker layering and fix spelling
• To speed up builds, you might separate the Python install from the pip install steps into different layers so that changes to your requirements file don’t invalidate the entire layer.
• There is a minor spelling issue on line 75 where “dependenciess” is spelled with an extra “s.”Example to fix the spelling:
- # Install dependenciess + # Install dependencies
116-116: Consider consistent exports
Having both a named “run” export and a default export referring to the same entity can be confusing. Consider removing the default export or clearly documenting how each should be used to avoid duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/build/src/extensions/python.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / 🧪 Unit Tests
🔇 Additional comments (1)
packages/build/src/extensions/python.ts (1)
23-25: Looks good
Returning an instance of PythonExtension is straightforward and clear.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/build/src/extensions/python.ts (1)
97-100:⚠️ Potential issueShell injection vulnerability in command execution.
The use of
shell: truewith user-provided arguments poses a security risk.This was previously flagged in an earlier review. Please implement the suggested fix to use execa's argument array instead of shell execution.
🧹 Nitpick comments (3)
packages/build/src/extensions/python.ts (3)
8-21: Consider adding validation for requirements and pythonBinaryPath.The
PythonOptionstype could benefit from additional validation:
- For
requirements: Validate package names and version specifiers format- For
pythonBinaryPath: Add OS-specific path validationConsider adding validation in the constructor:
private validateRequirements(reqs: string[]) { const packagePattern = /^[a-zA-Z0-9\-_.]+(==|>=|<=|!=|~=|>|<)?[0-9a-zA-Z\-.]*$/; reqs.forEach(req => { assert(packagePattern.test(req.trim()), `Invalid requirement format: ${req}`); }); } private validatePythonPath(path: string) { if (process.platform === 'win32') { assert(path.endsWith('.exe'), 'Windows Python path must end with .exe'); } assert(fs.existsSync(path), `Python binary not found at ${path}`); }
62-77: Optimize Docker layer setup for better caching.The current Docker layer setup could be optimized to better leverage Docker's layer caching:
- Separate Python installation and requirements installation into distinct layers
- Use multi-stage builds to reduce final image size
- Consider using
--no-install-recommendsfor pip installationsConsider this optimized Dockerfile structure:
- # Install Python - RUN apt-get update && apt-get install -y --no-install-recommends \ - python3 python3-pip python3-venv && \ - apt-get clean && rm -rf /var/lib/apt/lists/* - - # Set up Python environment - RUN python3 -m venv /opt/venv - ENV PATH="/opt/venv/bin:$PATH" - - ARG REQUIREMENTS_CONTENT - RUN echo "$REQUIREMENTS_CONTENT" > requirements.txt - - # Install dependenciess - RUN pip install --no-cache-dir -r requirements.txt + # Install Python in a separate layer + RUN apt-get update \ + && apt-get install -y --no-install-recommends \ + python3 \ + python3-pip \ + python3-venv \ + && apt-get clean \ + && rm -rf /var/lib/apt/lists/* + + # Set up venv in a separate layer + RUN python3 -m venv /opt/venv \ + && /opt/venv/bin/pip install --no-cache-dir --upgrade pip + + ENV PATH="/opt/venv/bin:$PATH" + + # Install dependencies in a separate layer + ARG REQUIREMENTS_CONTENT + RUN if [ ! -z "$REQUIREMENTS_CONTENT" ]; then \ + echo "$REQUIREMENTS_CONTENT" > requirements.txt \ + && pip install --no-cache-dir -r requirements.txt \ + && rm requirements.txt; \ + fi
102-108: Enhance error handling for Python-specific errors.The current error handling doesn't distinguish between different types of Python errors (syntax errors, import errors, etc.).
Consider parsing Python's error output:
try { assert(!result.failed, `Command failed: ${result.stderr}`); assert(result.exitCode === 0, `Non-zero exit code: ${result.exitCode}`); } catch (e) { // Parse Python error type from stderr const errorMatch = result.stderr.match(/^([A-Za-z]+Error):/m); if (errorMatch) { logger.error(`Python ${errorMatch[1]}: ${e.message}`, result); } else { logger.error(e.message, result); } throw e; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/build/src/extensions/python.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: units / 🧪 Unit Tests
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/build/src/extensions/python.ts (2)
23-27: 🛠️ Refactor suggestionImprove requirements parsing robustness.
The current implementation doesn't handle advanced requirements.txt features.
Consider this improved parsing that handles more edge cases:
-const splitAndCleanComments = (str: string) => - str - .split("\n") - .map((line) => line.trim()) - .filter((line) => line && !line.startsWith("#")); +const splitAndCleanComments = (str: string) => + str + .split("\n") + .map((line) => line.trim()) + .filter((line) => line && !line.startsWith("#")) + .map((line) => { + // Handle line continuations + if (line.endsWith("\\")) { + return line.slice(0, -1).trim(); + } + // Extract package name before any version specifiers or options + const [name] = line.split(/[=<>~]/); + return name.trim(); + }) + .filter((line) => !line.startsWith("-r")); // Skip recursive requirements
113-116:⚠️ Potential issueFix shell injection vulnerability.
Using shell=true with user-provided arguments is unsafe.
Apply this fix:
- const result = await $({ - shell: true, - ...options, - })(pythonBin, ...scriptArgs); + const result = await $({ + ...options, + })(pythonBin, ...scriptArgs);
🧹 Nitpick comments (1)
packages/build/src/extensions/python.ts (1)
129-132: Improve input validation for inline scripts.The current validation only checks if the script content exists.
Consider adding more validation:
-export const runInline = (scriptContent: string, options: Parameters<typeof $>[1] = {}) => { - assert(scriptContent, "Script content is required"); - return run([""], { input: scriptContent, ...options }); +export const runInline = (scriptContent: string, options: Parameters<typeof $>[1] = {}) => { + assert(scriptContent, "Script content is required"); + assert( + typeof scriptContent === "string" && scriptContent.trim().length > 0, + "Script content must be a non-empty string" + ); + // Validate script syntax before execution + try { + return run(["-c", "import ast; ast.parse(input())"], { + input: scriptContent, + ...options, + }); + } catch (e) { + throw new Error(`Invalid Python syntax: ${e.message}`); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/build/src/extensions/python.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / 🧪 Unit Tests
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/build/src/extensions/python.ts (2)
31-35: 🛠️ Refactor suggestionEnhance requirements file parsing.
The current implementation doesn't handle advanced requirements.txt features.
Consider this improved parsing:
const splitAndCleanComments = (str: string) => str .split('\n') .map(line => line.trim()) .filter(line => line && !line.startsWith('#')) .map(line => { // Handle line continuations if (line.endsWith('\\')) { return line.slice(0, -1).trim(); } // Extract package name before any version specifiers or options return line.split(/[=<>~\s]/)[0]; }) .filter(Boolean);
102-108:⚠️ Potential issueEnhance security of requirements installation.
The current approach of echoing requirements to a file and installing them could be improved.
Consider these security enhancements:
ARG REQUIREMENTS_CONTENT -RUN echo "$REQUIREMENTS_CONTENT" > requirements.txt - -# Install dependencies -RUN pip install --no-cache-dir -r requirements.txt +# Create requirements file with proper permissions +RUN echo "$REQUIREMENTS_CONTENT" > requirements.txt && \ + chmod 644 requirements.txt && \ + # Verify package integrity during installation + pip install --no-cache-dir --require-hashes -r requirements.txt && \ + # Clean up + rm requirements.txt
🧹 Nitpick comments (2)
packages/build/src/extensions/python.ts (2)
9-29: Add validation for pythonBinaryPath format.Consider adding platform-specific path format validation for the
pythonBinaryPathoption to prevent runtime issues.function validatePythonPath(path: string): boolean { if (process.platform === 'win32') { return /^[a-zA-Z]:\\.*\\python\.exe$/i.test(path); } return /^(\/[\w-]+)+\/python[23]?$/.test(path); }
153-157: Add input validation for inline scripts.Validate the script content to ensure it's not empty or malformed.
export const runInline = (scriptContent: string, options: Parameters<typeof $>[1] = {}) => { assert(scriptContent, "Script content is required"); + assert( + scriptContent.trim().length > 0, + "Script content cannot be empty" + ); + assert( + !scriptContent.includes("import os; os.system"), + "Direct system commands are not allowed" + ); return run([""], { input: scriptContent, ...options }); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/build/src/extensions/python.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: units / 🧪 Unit Tests
🔇 Additional comments (1)
packages/build/src/extensions/python.ts (1)
117-140:⚠️ Potential issueRemove shell execution for better security.
Using
shell: truecan expose the application to command injection vulnerabilities.- const result = await execa({ - shell: true, - verbose: (verboseLine, verboseObject) => logger.debug(verboseLine, verboseObject), - ...options, - })(pythonBin, scriptArgs); + const result = await execa(pythonBin, scriptArgs, { + verbose: (verboseLine, verboseObject) => logger.debug(verboseLine, verboseObject), + ...options, + });Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/build/src/extensions/python.ts (2)
11-31: Add validation for pythonBinaryPath and scripts options.The type definition is well-documented, but could benefit from additional runtime validation:
pythonBinaryPathshould be validated to ensure it points to a valid Python executablescriptsglob patterns should be validated for syntaxConsider adding these validations in the constructor:
if (this.options.pythonBinaryPath) { assert( fs.existsSync(this.options.pythonBinaryPath), `Python binary not found: ${this.options.pythonBinaryPath}` ); assert( fs.statSync(this.options.pythonBinaryPath).mode & fs.constants.X_OK, `Python binary is not executable: ${this.options.pythonBinaryPath}` ); } if (this.options.scripts) { assert( Array.isArray(this.options.scripts) && this.options.scripts.every(s => typeof s === 'string'), 'scripts must be an array of strings' ); // Optional: Add glob pattern validation }
131-137: Enhance error handling with detailed error types.The current error handling could be more informative by using custom error types and preserving the error chain.
Consider this improvement:
class PythonExecutionError extends Error { constructor( message: string, public readonly result: ExecaReturnValue, public readonly cause?: Error ) { super(message); this.name = 'PythonExecutionError'; } } try { assert(!result.failed, `Command failed: ${result.stderr}`); assert(result.exitCode === 0, `Non-zero exit code: ${result.exitCode}`); } catch (e) { logger.error(e.message, result); throw new PythonExecutionError( `Python execution failed: ${e.message}`, result, e ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/build/src/extensions/python.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
🔇 Additional comments (2)
packages/build/src/extensions/python.ts (2)
35-43: Improve requirements file parsing.The current implementation could be enhanced to handle more complex requirements file formats.
66-72: undefined
It has now been fixed! Yupi! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/little-trains-begin.md (1)
5-5: Clear Update Description Provided.
The message clearly outlines the new Python extension’s purpose—enhancing script execution and error handling. For extra clarity, consider linking to more detailed documentation or including a bit more context (e.g., mentioning that this addresses earlier deployment concerns) if it adds value in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.changeset/little-trains-begin.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: typecheck / typecheck
🔇 Additional comments (1)
.changeset/little-trains-begin.md (1)
1-3: Validate Changeset Header Format.
The changeset header correctly declares a minor update for "@trigger.dev/build" using the expected YAML delimiter syntax. This aligns well with our versioning guidelines.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/build/src/extensions/python.ts (2)
24-30: Enhance documentation for scripts option security implications.The documentation for the
scriptsoption should include security considerations and best practices.Add the following to the documentation:
/** * An array of glob patterns that specify which Python scripts are allowed to be executed. * * @remarks * These scripts will be copied to the container during the build process. + * + * @security + * - Ensure glob patterns are specific to avoid including unintended scripts + * - Avoid patterns that could match user-uploaded content + * - Consider using allowlists instead of broad patterns */
131-137: Enhance error handling with detailed error information.The current error handling could provide more context about the failure.
Apply this diff to improve error handling:
try { - assert(!result.failed, `Command failed: ${result.stderr}`); - assert(result.exitCode === 0, `Non-zero exit code: ${result.exitCode}`); + assert(!result.failed, `Python command failed: ${result.stderr}\nCommand: ${result.command}`); + assert(result.exitCode === 0, `Python command exited with non-zero code ${result.exitCode}\nStdout: ${result.stdout}\nStderr: ${result.stderr}`); } catch (e) { - logger.error(e.message, result); + logger.error("Python command execution failed", { + error: e.message, + command: result.command, + stdout: result.stdout, + stderr: result.stderr, + exitCode: result.exitCode + }); throw e; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/build/package.json(1 hunks)packages/build/src/extensions/python.ts(1 hunks)
🔇 Additional comments (2)
packages/build/package.json (1)
69-70: Dependency Update: Adding New Python Extension Support Dependencies.The additions of "@trigger.dev/sdk": "workspace:3.3.13" and "execa": "^9.5.2" in the dependencies block appropriately support the new pythonExtension functionality. Both dependencies align with the PR objectives and are correctly specified.
packages/build/src/extensions/python.ts (1)
90-95: Consider environment variable conflict resolution strategy.The
override: trueflag for environment variables could lead to conflicts if multiple extensions set the same variables.Consider implementing a conflict resolution strategy:
- Document the behavior when conflicts occur
- Add a warning when conflicting environment variables are detected
Run this script to check for potential environment variable conflicts:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for other extensions that might set PYTHON_BIN_PATH rg -l "PYTHON_BIN_PATH" --type tsLength of output: 73
Environment Variable Conflict Check Outcome
Our search found that PYTHON_BIN_PATH is only defined in packages/build/src/extensions/python.ts, with no other instances in the repository. This suggests there is currently no conflict with environment variable assignment. However, consider the following for future-proofing:
- Document that the PYTHON_BIN_PATH is uniquely used by the Python extension.
- Consider adding warning logs or notices if a conflict is detected when multiple extensions are used.
- Review the override behavior in case other extensions are introduced that might reuse similar environment variable names.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| assert(scriptPath, "Script path is required"); | ||
| assert(fs.existsSync(scriptPath), `Script does not exist: ${scriptPath}`); |
There was a problem hiding this comment.
TODO
- in dev, we should check whether
scriptPathmatches the patterns defined inthis.options.scriptsotherwise throw an error even if file exists locally (fail-fast approach to avoid errors only when deploying) - To do so, we better define a
doesMatchPatternfunction (insideadditionalFiles.ts?) similar to findStaticAssetFiles
trigger.dev/packages/build/src/extensions/core/additionalFiles.ts
Lines 37 to 41 in bc7d445
Questions
- How do I check if I am in dev when I have no access to
context: BuildContext?process.env.NODE_DEV === 'development'? - Should
doesMatchPatternbe defined locally or exported fromadditionalFiles.ts? - Do I store the patterns in
this.options.scriptsin an env var as well, in order to have accessible fromrunScript?
|
I've published some preview packages You will need to install these versions: Then run the CLI like this. Dev: Deploy: |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/build/package.json (1)
71-78:⚠️ Potential issueUpdate pnpm-lock.yaml for new dependencies.
The pipeline is failing because the lock file hasn't been updated after adding new dependencies. Run
pnpm installto update the lock file.
🧹 Nitpick comments (1)
packages/build/package.json (1)
73-73: Consider pinning the tinyexec version.The caret (^) in the version constraint for
tinyexecallows minor updates which could introduce breaking changes. Since this is a critical dependency for executing Python commands, consider pinning the version to ensure stability.- "tinyexec": "^0.3.2", + "tinyexec": "0.3.2",Also applies to: 75-75
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
packages/build/package.json(4 hunks)
🧰 Additional context used
🪛 GitHub Actions: 🤖 PR Checks
packages/build/package.json
[error] 1-1: Cannot install with 'frozen-lockfile' because pnpm-lock.yaml is not up to date with packages/build/package.json.
🔇 Additional comments (1)
packages/build/package.json (1)
31-31: LGTM! Python extension configuration is properly structured.The Python extension is correctly configured across all necessary sections, following the same pattern as other extensions in the package.
Also applies to: 55-57, 159-169
|
@zvictor this looks like an amazing start! I haven't had the chance to pull it down and try it out yet, but I think we need a different approach to where this will live. I'm thinking the best thing would be to create a |
|
So the only changes to the final user are in the imports, correct?
Looking in isolation it seems more elegant, but I think this decision needs to be lead by the standard you want to set. Right now, all builtin extensions are placed in Looking forward, I see the code of this PR being duplicated to add support to all sorts of languages. Will they all have their own packages (e.g. |
|
Oh, and regardless of the changes, please try to move the code around ( |
|
The reason I want it in its own package is because it includes both a build extension and runtime features, which is different than our current set of build extensions. And yes, having a separate package for other language support makes sense to me. |
|
Yes, that makes sense! Can you please prioritize the questions here? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/build/src/extensions/python.ts (4)
9-29: Consider adding validation options to PythonOptions.The type definition could benefit from additional options to control Python version constraints and dependency validation.
export type PythonOptions = { requirements?: string[]; requirementsFile?: string; + // Specify minimum Python version required + minPythonVersion?: string; + // Validate dependencies against known vulnerabilities + validateDependencies?: boolean; /** * [Dev-only] The path to the python binary.
44-59: Enhance error messages with more context.The error messages could be more descriptive to help users understand and fix configuration issues.
assert( !(this.options.requirements && this.options.requirementsFile), - "Cannot specify both requirements and requirementsFile" + "Configuration Error: Cannot specify both 'requirements' and 'requirementsFile'. Choose one method to specify Python dependencies." ); if (this.options.requirementsFile) { assert( fs.existsSync(this.options.requirementsFile), - `Requirements file not found: ${this.options.requirementsFile}` + `Configuration Error: Requirements file not found at '${this.options.requirementsFile}'. Ensure the path is correct and the file exists.` );
106-112: Improve pip install error handling.Consider adding error handling and retries for pip install failures, which can occur due to network issues.
ARG REQUIREMENTS_CONTENT RUN echo "$REQUIREMENTS_CONTENT" > requirements.txt # Install dependencies - RUN pip install --no-cache-dir -r requirements.txt + RUN pip install --no-cache-dir -r requirements.txt || \ + (echo "First attempt failed, retrying..." && \ + pip install --no-cache-dir --retries 3 -r requirements.txt)
159-170: Enhance error handling in runInline.The cleanup in the finally block could fail silently. Consider adding error handling for the cleanup operation.
try { return await runScript(tmpFile, [], options); } finally { - await fs.promises.unlink(tmpFile); + try { + await fs.promises.unlink(tmpFile); + } catch (error) { + logger.warn(`Failed to clean up temporary file ${tmpFile}:`, error); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/build/src/extensions/python.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/build/src/extensions/python.ts (3)
Learnt from: zvictor
PR: triggerdotdev/trigger.dev#1686
File: packages/build/src/extensions/python.ts:0-0
Timestamp: 2025-02-10T11:19:37.014Z
Learning: In the Python extension for Trigger.dev, do not enforce `.py` file extensions for Python scripts to maintain flexibility for developers.
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.
Learnt from: zvictor
PR: triggerdotdev/trigger.dev#1686
File: packages/build/src/extensions/python.ts:85-87
Timestamp: 2025-02-10T10:54:17.345Z
Learning: In Python-related Dockerfiles for trigger.dev, avoid adding explicit Python version pinning as the base image already provides conservative version management. Additional pinning would unnecessarily slow down builds.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/python/src/index.ts (2)
6-31: Consider adding a more detailed error message for installation or command-not-found scenarios.
Currently, a non-zero exit code triggers a generic error message. Ifpythonis missing from the system or an environment issue arises, differentiating those causes can improve debugging.
44-61: Use a more robust approach for temporary file creation.
Generating a unique file name withDate.now()works in most cases but can lead to race conditions if many calls happen in the same millisecond. Consider usingfs.mkdtempor a random suffix for improved collision resistance.packages/python/src/extension.ts (1)
59-116: Layered build strategy is clear, though consider different package systems.
Installing Python viaapt-getand configuring a venv is an excellent approach for many Debian-based images. If future usage extends to Alpine or other distros, you may need to customize the build steps.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
.changeset/little-trains-begin.md(1 hunks)packages/build/README.md(1 hunks)packages/python/CHANGELOG.md(1 hunks)packages/python/LICENSE(1 hunks)packages/python/README.md(1 hunks)packages/python/package.json(1 hunks)packages/python/src/extension.ts(1 hunks)packages/python/src/index.ts(1 hunks)packages/python/tsconfig.json(1 hunks)packages/python/tsconfig.src.json(1 hunks)references/v3-catalog/trigger.config.ts(2 hunks)
✅ Files skipped from review due to trivial changes (7)
- packages/python/README.md
- packages/python/tsconfig.src.json
- packages/python/CHANGELOG.md
- packages/python/LICENSE
- packages/python/tsconfig.json
- packages/build/README.md
- packages/python/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/little-trains-begin.md
- references/v3-catalog/trigger.config.ts
🔇 Additional comments (8)
packages/python/src/index.ts (3)
1-5: Imports look solid.
All the imported modules appear consistent with the code’s functionality.
33-42: Good parameter validation.
Asserting that the script path exists is helpful. For extended safety, you may optionally verify that the path refers to a file rather than a directory or check for a.pyextension to ensure clarity for new contributors.
63-63: Default export is consistent.
Exporting all three functions under a single default object is a convenient pattern. Great job.packages/python/src/extension.ts (5)
1-5: Dependencies imported correctly.
Everything here aligns well with the build extension requirements.
7-27: Doc comments are clear and descriptive.
The properties and their intended usage are well-documented, simplifying the learning curve for new contributors.
29-34: Handy utility for cleaning up requirements.
Stripping empty lines and commented lines simplifies the content before passing it to pip.
35-37: Extension registration seems straightforward.
Creating a dedicated extension instance helps keep the code maintainable.
39-57: Robust constructor validation.
Enforcing thatrequirementsandrequirementsFilecannot be set simultaneously prevents ambiguous configurations.
|
@ericallam We have an independent package now, as you suggested! I have built the package and linked it in my local project (i.e. @matt-aitken feel free to release a |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/python/README.md (2)
5-17: Consider Simplifying Wording in the Overview SectionThe overview explains key capabilities clearly. However, the phrase “Provides a variety of functions for executing Python code” can be streamlined for clarity. For example, consider replacing it with “Provides helper functions for executing Python code.”
Suggested diff:
- - **Helper Functions:** Provides a variety of functions for executing Python code: + - **Helper Functions:** Provides helper functions for executing Python code:🧰 Tools
🪛 LanguageTool
[style] ~13-~13: The phrase “a variety of” may be wordy. To make your writing clearer, consider replacing it.
Context: ...ncies. - Helper Functions: Provides a variety of functions for executing Python code: ...(A_VARIETY_OF)
23-39: TypeScript Configuration Example ClarityThe provided configuration snippet is comprehensive and helpful. Consider adding a clarifying comment about the
pathandrootDirvariables—ensuring users know that they need to be defined or imported appropriately in their configuration context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
packages/python/README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
packages/python/README.md
[style] ~13-~13: The phrase “a variety of” may be wordy. To make your writing clearer, consider replacing it.
Context: ...ncies. - Helper Functions: Provides a variety of functions for executing Python code: ...
(A_VARIETY_OF)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: typecheck / typecheck
🔇 Additional comments (9)
packages/python/README.md (9)
1-3: Clear Title and IntroductionThe title and introductory description effectively convey the purpose of the Python extension.
19-22: Clear Usage Instruction IntroductionThe instructions for adding the extension to the configuration file are presented clearly, making it easy for users to follow.
41-42: Optional Requirements File StepThe step to optionally create a
requirements.txtfile is concise and clear.
43-44: Clear Execution Step DescriptionThe instruction to execute Python scripts using the provided functions is succinct and easy to understand.
45-59: Clear Python Script Execution ExampleThe example demonstrating how to run a Python script via
python.runScriptis well structured. It might be beneficial to mention the behavior in error scenarios or what output to expect on failures, but as-is, it clearly communicates the intended usage.
60-77: Verify Inline Python Code ExampleThe inline Python code example is useful and demonstrates dynamic code execution. However, note that the snippet uses JavaScript template literal interpolation (e.g.,
${+new Date() / 1000}) inside the Python code block. Please confirm that this behavior is intended and clearly documented, as it might be confusing for users expecting a purely static Python snippet.
79-92: Lower-Level Commands Usage ExampleThe lower-level commands example clearly shows how to use
python.runto execute commands (e.g., retrieving the Python version). The inclusion of an expected output comment further enhances clarity.
94-98: Update Limitations Section if ApplicableThe limitations section outlines current constraints well. However, please verify that the statement regarding scripts not being automatically copied to staging/production containers remains accurate. If the recent fixes (as mentioned in the discussion) have resolved this issue, consider updating this section to avoid potential confusion.
100-103: Additional Information Section ClarityThe final section succinctly points users to further documentation, which is a nice touch for extended learning.
|
@zvictor this looks great! Going to properly dig in starting tomorrow (🤞) and will try and get this merged shortly |
|
@zvictor This looks like a great start! I'm going to get this into main and then create a preview package for people to try. I also have some ideas I want to get into the build process for making it easier for extensions to add additional files into builds, and then will add this functionality into this extension before doing an official release and changelog entry. |
|
The python package is now available as a preview package here: |
Description:
This PR introduces the
pythonExtensionbuild extension, enabling limited support for running Python scripts within Trigger.dev tasks.Discussion.
Key features:
pip.requirements.txtfile.trigger.config.tsfile using therequirementsoption./opt/venv) inside the containers to isolate Python dependencies.runfunction for executing Python scripts with proper environment setup, arunInlinefor quick python coding from node, andrunScriptfor proper *.py scripts execution.pythonBinaryPathcan be set to use any python installation.Usage:
pythonExtensionto yourtrigger.config.tsfile:(optional) Create a
requirements.txtfile in your project root with your desired Python dependencies.Use the
run,runInline, orrunScriptfunctions to execute Python scripts within your tasks:3.a. With
runScript:Beware: it does not take care of copying your python scripts to the staging/production containers.fixed!3.b. Using
runInline:3.c. For lower-level access,
rungives you direct access to thepythonbin:Limitations:
Summary by CodeRabbit
Summary by CodeRabbit