pythonExtension and python runtime improvements #1734
Conversation
|
WalkthroughThis update introduces significant restructuring across multiple packages. In the build package, it adds a dedicated internal module export and streamlines static asset handling by delegating to a new function. The core package now relies on a centralized AsyncIterableStream implementation instead of defining it locally, with several import path adjustments. Logger interfaces have been enhanced to support additional span options and tracing methods. The Python extension has been overhauled to improve configuration, error handling, and streaming capabilities, while a new Python catalog project brings its own configuration, dependency, and task files. Changes
Sequence Diagram(s)sequenceDiagram
participant B as Build Process
participant O as onBuildComplete
participant A as addAdditionalFilesToBuild
participant FS as File System
B->>O: Invoke onBuildComplete
O->>A: Delegate processing of additional files
A->>FS: Search and copy files
FS-->>A: Return file operation results
A-->>O: Return build results
sequenceDiagram
participant U as User
participant P as python.runScript
participant L as Logger
participant E as Execution Engine
U->>P: Invoke runScript with scriptPath and args
P->>L: Log execution details (script path, attributes)
P->>E: Execute Python script (with streaming capability)
E-->>P: Return output or error
P-->>U: Provide final result (or streamed output)
Suggested reviewers
Poem
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 (
|
ericallam
force-pushed
the
feat/python-extension-improvements
branch
from
b7e3601 to
4d8896e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (11)
references/python-catalog/src/trigger/pythonTasks.ts (2)
5-27: Consider adding error handling for Python script execution.The
convertUrlToMarkdowntask runs Python scripts but doesn't have any error handling for potential failures that might occur during execution. Consider adding try/catch blocks to handle errors gracefully.export const convertUrlToMarkdown = schemaTask({ id: "convert-url-to-markdown", schema: z.object({ url: z.string().url(), }), run: async (payload) => { + try { const result = await python.runScript("./src/python/html2text_url.py", [payload.url]); logger.debug("convert-url-to-markdown", { url: payload.url, output: result.stdout, }); const streamingResult = python.stream.runScript("./src/python/html2text_url.py", [payload.url]); for await (const chunk of streamingResult) { logger.debug("convert-url-to-markdown", { url: payload.url, chunk, }); } + } catch (error) { + logger.error("convert-url-to-markdown failed", { + url: payload.url, + error: error instanceof Error ? error.message : String(error) + }); + throw error; + } }, });
55-55: Check for errors in the Python execution results.The function returns
result.stdoutwithout checking if there were errors inresult.stderr. Consider checking for errors before returning.- return result.stdout; + if (result.stderr) { + logger.warn("python-run-inline produced stderr output", { stderr: result.stderr }); + } + return result.stdout;references/python-catalog/src/python/html2text_url.py (1)
18-22: Add timeout parameter for HTTP requests.It's a good practice to set timeouts for HTTP requests to prevent the script from hanging indefinitely if a server is unresponsive.
# Set up command line argument parsing parser = argparse.ArgumentParser(description='Convert HTML from a URL to plain text.') parser.add_argument('url', help='The URL to fetch HTML from') parser.add_argument('--ignore-links', action='store_true', help='Ignore converting links from HTML') +parser.add_argument('--timeout', type=float, default=30.0, + help='Timeout for HTTP requests in seconds (default: 30)')And update the
fetch_htmlfunction:-def fetch_html(url): +def fetch_html(url, timeout=30): """Fetch HTML content from a URL.""" try: - response = requests.get(url) + response = requests.get(url, timeout=timeout) response.raise_for_status() # Raise an exception for HTTP errors return response.textThen update the call in
main():# Fetch HTML from the URL -html_content = fetch_html(args.url) +html_content = fetch_html(args.url, args.timeout)packages/python/src/utils/tempFiles.ts (1)
1-2: Use consistent import styles.The file mixes different import styles - using named imports from 'node:fs' but destructuring imports from 'node:fs/promises'. Consider using consistent import styles for better readability.
-import { mkdtempSync } from "node:fs"; -import { mkdtemp, writeFile, rm } from "node:fs/promises"; +import * as fs from "node:fs"; +import * as fsp from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path";Then update the function calls:
- const tempDir = await mkdtemp(join(tmpdir(), "app-")); + const tempDir = await fsp.mkdtemp(join(tmpdir(), "app-"));And make similar updates throughout the file.
packages/build/src/internal/additionalFiles.ts (1)
17-31: Consider adding explicit error handling for file pattern resolution.At present, if an error occurs within
findStaticAssetFiles(e.g., invalid file pattern orglobfailure), the error would bubble up, potentially failing the entire build without a meaningful message. Wrapping this in a structured try/catch or explicitly handling the error could give more granular feedback.packages/build/src/extensions/core/additionalFiles.ts (1)
12-12: Consider explicit error handling for build completion.If
addAdditionalFilesToBuildfails internally (e.g., a file I/O error), it will throw an error. Confirm whether letting this entire function fail is the desired fallback, or if partial success (logging the error) is more appropriate.packages/python/src/index.ts (3)
14-15: Encourage setting a consistent environment variable for unbuffered output.To ensure real-time capturing of Python output, consider appending
env: { PYTHONUNBUFFERED: "1" }to the x(...) call. This prevents the Python process from buffering stdout/stderr, which can improve logging accuracy in streaming or trace scenarios.
54-54: Switch to asynchronous file existence checks.
assert(fs.existsSync(scriptPath))is synchronous and may block the event loop. Switching to an async check withfs.promises.stat(and handling potential ENOENT) can improve performance under high concurrency.- assert(fs.existsSync(scriptPath), `Script does not exist: ${scriptPath}`); + const scriptExists = await fs.promises + .stat(scriptPath) + .then(() => true) + .catch(() => false); + assert(scriptExists, `Script does not exist: ${scriptPath}`);
141-166: Assess concurrency for multiple stream calls.When multiple
python.stream.run()calls happen concurrently, they each create separate child processes. Ensure your system can handle multiple processes without resource contention. Consider a queue or concurrency limit if you expect heavy usage.packages/core/src/v3/index.ts (1)
24-24: Consider using explicit exports for clarity.
Whileexport *is convenient, it can obscure the origin and usage of exported symbols. Explicit exports often make the codebase easier to understand and reduce the risk of unintentional export changes.packages/core/src/v3/streams/asyncIterableStream.ts (1)
22-50: Stream cancellation and transform look good.
The approach cleanly leverages anAbortSignalfor graceful teardown, ensuring no infinite reading loop. If you expect additional error cases, consider adding more robust error handling for partial reads.
📜 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 (26)
packages/build/package.json(3 hunks)packages/build/src/extensions/core/additionalFiles.ts(2 hunks)packages/build/src/internal.ts(1 hunks)packages/build/src/internal/additionalFiles.ts(1 hunks)packages/core/src/v3/apiClient/index.ts(1 hunks)packages/core/src/v3/apiClient/runStream.ts(1 hunks)packages/core/src/v3/apiClient/stream.ts(1 hunks)packages/core/src/v3/index.ts(1 hunks)packages/core/src/v3/logger/index.ts(2 hunks)packages/core/src/v3/logger/taskLogger.ts(3 hunks)packages/core/src/v3/runMetadata/index.ts(1 hunks)packages/core/src/v3/runMetadata/manager.ts(1 hunks)packages/core/src/v3/runMetadata/noopManager.ts(1 hunks)packages/core/src/v3/runMetadata/types.ts(1 hunks)packages/core/src/v3/streams/asyncIterableStream.ts(1 hunks)packages/python/README.md(4 hunks)packages/python/package.json(1 hunks)packages/python/src/extension.ts(4 hunks)packages/python/src/index.ts(1 hunks)packages/python/src/utils/tempFiles.ts(1 hunks)references/python-catalog/package.json(1 hunks)references/python-catalog/requirements.txt(1 hunks)references/python-catalog/src/python/html2text_url.py(1 hunks)references/python-catalog/src/trigger/pythonTasks.ts(1 hunks)references/python-catalog/trigger.config.ts(1 hunks)references/python-catalog/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (9)
- packages/build/src/internal.ts
- packages/core/src/v3/runMetadata/index.ts
- references/python-catalog/tsconfig.json
- packages/core/src/v3/runMetadata/types.ts
- packages/core/src/v3/runMetadata/manager.ts
- packages/core/src/v3/apiClient/runStream.ts
- references/python-catalog/requirements.txt
- packages/core/src/v3/runMetadata/noopManager.ts
- references/python-catalog/package.json
⏰ 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: typecheck / typecheck
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (32)
packages/core/src/v3/logger/taskLogger.ts (2)
27-27: TaskLogger interface enhanced with startSpan methodThe addition of the
startSpanmethod to the TaskLogger interface provides a more direct way to create spans compared to the existingtracemethod, which is useful when you need manual control over span lifecycle.
94-96: Implementation of startSpan in OtelTaskLogger looks goodThe implementation correctly delegates to the tracer's startSpan method and properly passes through all parameters.
packages/core/src/v3/logger/index.ts (3)
3-3: Import update for new functionalityThe import has been correctly updated to include SpanOptions from OpenTelemetry.
50-52: Trace method signature updated for consistencyThe trace method signature has been properly updated to include the optional options parameter, maintaining consistency with the TaskLogger interface.
54-56: StartSpan method implementation looks goodThe startSpan method implementation correctly delegates to the underlying TaskLogger and properly passes all parameters.
packages/python/package.json (1)
58-65: Dependency management improvement.Good restructuring of dependencies. Moving
@trigger.dev/buildand@trigger.dev/sdkto peer dependencies helps avoid duplication and ensures version consistency across the project.packages/python/src/utils/tempFiles.ts (1)
13-31: Robust implementation of withTempFile.The implementation of
withTempFileis well done with proper error handling and cleanup. The use of try/finally ensures the temporary directory is always cleaned up, even if an error occurs.references/python-catalog/trigger.config.ts (1)
1-29: Well-structured Trigger.dev configuration for Python integrationThe configuration appears well-organized, with appropriate settings for the Python extension. It correctly specifies the runtime, project ID, machine type, and maximum duration. The Python extension configuration is properly set up with the requirements file path, dev Python binary path, and script patterns.
A few notes:
- The project ID
proj_hbsqkjxevkyuklehrgrpappears to be an actual production ID rather than a placeholder- The retry configuration is well-defined with appropriate exponential backoff settings
packages/python/README.md (9)
17-17: Updated property name improves clarityThe property has been appropriately renamed from
pythonBinaryPathtodevPythonBinaryPath, which better indicates that this is specifically for development environments.
25-25: Import syntax updated to use named exportsThe import statement has been updated to use named exports, which is consistent with how the extension is exported in the source code.
33-35: Property name updated in exampleThe example correctly uses the renamed
devPythonBinaryPathproperty and shows a sensible example value pointing to a virtual environment Python binary.
43-62: Helpful requirements file example addedThe added example for using a requirements.txt file provides valuable guidance for users on how to specify Python dependencies. Including both the requirements file format and the corresponding trigger configuration is especially helpful.
70-70: Import syntax updated to use named exportsThe import statement has been consistently updated to use named exports across all examples.
80-94: Valuable streaming output example addedThe new example for streaming Python script output is a useful addition that demonstrates an important capability of the extension. This helps users understand how to process outputs incrementally instead of waiting for the entire script to complete.
101-101: Import syntax updated to use named exportsThe import statement has been consistently updated across all examples.
109-109: Fixed pandas usage in exampleThe example now correctly uses the
pdalias which was imported earlier, rather than referring directly topandas.
120-120: Import syntax updated to use named exportsThe import statement has been consistently updated across all examples.
packages/build/package.json (3)
26-26: Added internal module export for improved modularizationThe new internal module export defines a clear boundary for accessing internal functionality, which improves code organization and maintainability.
40-42: Added TypeScript types configuration for internal moduleThe typesVersions configuration properly defines the TypeScript declaration file location for the internal module, ensuring correct type resolution.
102-112: Added comprehensive exports configuration for internal moduleThe detailed exports configuration for the internal module follows the project's established pattern, with proper support for both ESM and CommonJS consumers, as well as TypeScript types.
packages/python/src/extension.ts (6)
3-3: Updated import to use the new internal moduleThe import now correctly uses the newly exposed
addAdditionalFilesToBuildfunction from the build package's internal module.
19-19: Renamed property to better reflect its purposeThe property has been renamed from
pythonBinaryPathtodevPythonBinaryPath, which better indicates that this option is specifically for development environments. This change is consistent with the documentation update in the README.
60-67: Improved file management with dedicated functionThe implementation now uses the
addAdditionalFilesToBuildfunction for handling script files, which provides a more standardized approach to file management in the build process. The explicit identifier "pythonExtension" helps with tracking and debugging.
69-72: Updated property referenceThe code now correctly references the renamed
devPythonBinaryPathproperty, maintaining consistency with the type definition change.
101-133: Enhanced requirements file handlingThe implementation now properly handles requirements files by:
- Logging a warning when both options are specified
- Copying the requirements file directly to the container
- Installing dependencies from the copied file
- Using a separate layer for dependency installation
This approach is more robust than the previous implementation and follows container best practices by keeping the layers separate.
133-154: Improved conditional handling of requirementsThe code now conditionally adds a layer for dependency installation only when needed, which optimizes the build process. The structure with explicit conditional blocks improves readability and maintainability.
packages/build/src/internal/additionalFiles.ts (1)
73-81: Verify path manipulation for potential misalignment on Windows platforms.Here, using
posix.sepfor splitting and filtering..might cause path-related inconsistencies on Windows. Consider using Node’s built-in path functions or normalizing paths to ensure correct handling across all platforms.packages/build/src/extensions/core/additionalFiles.ts (1)
2-2: Confirm that the import references the intended file.You reference
"../../internal/additionalFiles.js". Ensure it aligns with the transpiled file extension (TypeScript → JS) at runtime. When using ES modules, mismatched extensions can cause import resolution issues.packages/python/src/index.ts (1)
28-34: Double-check partial error handling vs. throwOnError.You override
throwOnErrorto false but immediately throw ifexitCode !== 0. Confirm that you don’t inadvertently swallow additional context fromtinyexec. If you need to differentiate between error types (e.g., OOM vs. script error), you might want a more nuanced approach than a single throw.packages/core/src/v3/apiClient/index.ts (1)
64-64: Import usage looks good.
The newly introduced import forAsyncIterableStreamis well-placed and consistent with the updated stream-based APIs.packages/core/src/v3/apiClient/stream.ts (1)
12-12: Import change is consistent with the reorganization.
ReferencingAsyncIterableStreamandcreateAsyncIterableStreamfrom the new dedicated location keeps stream functionality centralized.packages/core/src/v3/streams/asyncIterableStream.ts (1)
1-2: Confirm environment support for ReadableStream.
ReadableStreammight require polyfills or a specific runtime version. Ensure compatibility if you plan to run on Node versions that predate fullReadableStreamsupport.
| def fetch_html(url): | ||
| """Fetch HTML content from a URL.""" | ||
| try: | ||
| response = requests.get(url) | ||
| response.raise_for_status() # Raise an exception for HTTP errors | ||
| return response.text | ||
| except requests.exceptions.RequestException as e: | ||
| print(f"Error fetching URL: {e}", file=sys.stderr) | ||
| sys.exit(1) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for library usage scenarios.
The current error handling exits the entire process with sys.exit(1), which is appropriate for command-line usage but not when imported as a module. Consider returning errors instead of exiting.
def fetch_html(url):
"""Fetch HTML content from a URL."""
try:
response = requests.get(url)
response.raise_for_status() # Raise an exception for HTTP errors
return response.text
except requests.exceptions.RequestException as e:
- print(f"Error fetching URL: {e}", file=sys.stderr)
- sys.exit(1)
+ error_msg = f"Error fetching URL: {e}"
+ print(error_msg, file=sys.stderr)
+ if __name__ == "__main__":
+ sys.exit(1)
+ return NoneYou'll need to update the main function to handle this return value:
# Fetch HTML from the URL
html_content = fetch_html(args.url)
+if html_content is None:
+ sys.exit(1)📝 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.
| def fetch_html(url): | |
| """Fetch HTML content from a URL.""" | |
| try: | |
| response = requests.get(url) | |
| response.raise_for_status() # Raise an exception for HTTP errors | |
| return response.text | |
| except requests.exceptions.RequestException as e: | |
| print(f"Error fetching URL: {e}", file=sys.stderr) | |
| sys.exit(1) | |
| import sys | |
| import requests | |
| def fetch_html(url): | |
| """Fetch HTML content from a URL.""" | |
| try: | |
| response = requests.get(url) | |
| response.raise_for_status() # Raise an exception for HTTP errors | |
| return response.text | |
| except requests.exceptions.RequestException as e: | |
| error_msg = f"Error fetching URL: {e}" | |
| print(error_msg, file=sys.stderr) | |
| if __name__ == "__main__": | |
| sys.exit(1) | |
| return None | |
| # Example main function usage | |
| if __name__ == "__main__": | |
| import argparse | |
| parser = argparse.ArgumentParser(description="Fetch HTML content from a URL.") | |
| parser.add_argument("url", help="URL to fetch") | |
| args = parser.parse_args() | |
| # Fetch HTML from the URL | |
| html_content = fetch_html(args.url) | |
| if html_content is None: | |
| sys.exit(1) | |
| # Continue processing html_content... | |
| print(html_content) |
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
docs/config/extensions/pythonExtension.mdx (1)
133-154:⚠️ Potential issueCritical Issue: Variable Name Mismatch in Streaming Output Example
In the streaming output snippet (lines 133–154), there is an inconsistency: the stream result is assigned to a variable namedresulton line 145, but the subsequentfor awaitloop iterates overstreamingResult. Please update the snippet to use a consistent variable name. For example:- const result = python.stream.runScript("./python/my_script.py", ["hello", "world"]); - - // result is an async iterable/readable stream - for await (const chunk of streamingResult) { - console.log(chunk); - } + const streamingResult = python.stream.runScript("./python/my_script.py", ["hello", "world"]); + + // streamingResult is an async iterable/readable stream + for await (const chunk of streamingResult) { + console.log(chunk); + }🧰 Tools
🪛 LanguageTool
[style] ~134-~134: Consider removing “of” to be more concise
Context: ...e Python scripts. ## Streaming output All of thepythonfunctions have a streaming ver...(ALL_OF_THE)
🧹 Nitpick comments (14)
docs/config/config-file.mdx (1)
326-327: Review Repetitive Sentence Structure in emitDecoratorMetadata Section.
The documentation line “See the emitDecoratorMetadata documentation for more information.” follows a repetitive pattern found in other sections. Consider varying the sentence construction for enhanced readability and style consistency.docs/config/extensions/additionalPackages.mdx (1)
22-23: Refine Sentence Punctuation in Extension Description.
The paragraph beginning with “This allows you to include additional packages…” is informative; however, the sentence “We will try to automatically resolve the version of the package but you can specify the version by using the@symbol:” would benefit from a comma before “but” to improve readability. For example:“We will try to automatically resolve the version of the package, but you can specify the version by using the
@symbol:”🧰 Tools
🪛 LanguageTool
[style] ~22-~22: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ckage that includes a CLI tool that you want to invoke in your tasks viaexec. We wil...(REP_WANT_TO_VB)
[uncategorized] ~22-~22: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...cally resolve the version of the package but you can specify the version by using th...(COMMA_COMPOUND_SENTENCE)
docs/config/extensions/overview.mdx (1)
7-9: Improve Grammar in the Build Extensions Introduction.
The opening sentence “Build extension allow you to hook into the build system…” should be revised to “Build extensions allow you to hook into the build system…” for subject-verb agreement. This minor change will improve the clarity of the documentation’s introduction.docs/config/extensions/prismaExtension.mdx (2)
9-15: Grammar & Terminology Improvement
In the introductory bullet points (lines 9–15), the phrases “deploy process” appear (e.g. “Generates the Prisma client during the deploy process” and “Optionally will migrate the database during the deploy process”). For clarity and consistency, consider changing “deploy process” to “deployment process.”🧰 Tools
🪛 LanguageTool
[grammar] ~10-~10: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ... Generates the Prisma client during the deploy process - Optionally will migrate the d...(PREPOSITION_VERB)
[grammar] ~11-~11: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ...ly will migrate the database during the deploy process - Support for TypedSQL and mult...(PREPOSITION_VERB)
142-144: Punctuation Suggestion
In the “Multiple schemas” section (lines 142–144), consider adding a comma for improved readability. For example, change
If you have multiple separate schemas in the same project you can add the extension multiple times:
to
If you have multiple separate schemas in the same project, you can add the extension multiple times:🧰 Tools
🪛 LanguageTool
[uncategorized] ~144-~144: Possible missing comma found.
Context: ...e multiple separate schemas in the same project you can add the extension multiple time...(AI_HYDRA_LEO_MISSING_COMMA)
docs/config/extensions/custom.mdx (4)
7-7: Grammar Correction
On line 7, the sentence begins with “Build extension allow you…” Consider changing it to “Build extensions allow you…” to match the plural subject.
179-187: Clarification on Placeholder Usage
In the “onBuildComplete” code snippet (lines 179–187), thedependenciesfield is used without being defined. Consider either providing a concrete example (e.g.,"my-dependency": "^1.0.0") or clarifying in a comment that it is a placeholder for user-defined dependencies.
195-203: BuildTarget Section – Clarity Improvement
The sentence “Can either bedevordeploy, matching the CLI command name that is being run.” is missing an explicit subject. Rephrase it to:
“The BuildTarget can be eitherdevordeploy, matching the CLI command name that is being run.”🧰 Tools
🪛 LanguageTool
[style] ~196-~196: To form a complete sentence, be sure to include a subject.
Context: ...how to useaddLayer. ## BuildTarget Can either bedevordeploy, matching t...(MISSING_IT_THERE)
380-381: Text Correction in Troubleshooting Section
At line 380, the text currently reads:
“You should also take a look at our built in extensions for inspiration on how to create your own. You can find them in in the source code here.”
Please update it to remove the duplicate “in” and change “built in” to “built-in.” For example:
“You should also take a look at our built-in extensions for inspiration on how to create your own. You can find them here.”🧰 Tools
🪛 LanguageTool
[grammar] ~380-~380: A hyphen is missing in the adjective “built-in”.
Context: ...``` You should also take a look at our built in extensions for inspiration on how to cr...(BUILT_IN_HYPHEN)
[duplication] ~380-~380: Possible typo: you repeated a word.
Context: ...w to create your own. You can find them in in [the source code here](https://github.c...(ENGLISH_WORD_REPEAT_RULE)
docs/config/extensions/pythonExtension.mdx (1)
44-46: Grammar Improvement
In the “Adding python scripts” section (lines 44–46), the phrase “during the deploy process” appears. For improved clarity, consider replacing it with “during the deployment process.”🧰 Tools
🪛 LanguageTool
[grammar] ~46-~46: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ...ripts to the build directory during the deploy process. For example: ```ts import { d...(PREPOSITION_VERB)
docs/config/extensions/syncEnvVars.mdx (3)
29-34: Comprehensive Callback Context Description.The section explaining the callback context (lines 29–34) is very informative, outlining all relevant properties (
environment,projectRef, andenv). A minor stylistic improvement would be to review the punctuation in the property list to ensure optimal clarity.🧰 Tools
🪛 LanguageTool
[uncategorized] ~31-~31: Loose punctuation mark.
Context: ...e following properties: -environment: The environment name that the task is b...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~32-~32: Loose punctuation mark.
Context: ...uction,staging, etc.) -projectRef`: The project ref of the Trigger.dev proj...(UNLIKELY_OPENING_PUNCTUATION)
37-37: Minor Punctuation Improvement.On line 37, consider adding a comma after "In this example" to enhance readability:
"In this example, we're using env vars from Infisical."🧰 Tools
🪛 LanguageTool
[typographical] ~37-~37: It appears that a comma is missing.
Context: ...: Sync env vars from Infisical In this example we're using env vars from [Infisical](h...(DURING_THAT_TIME_COMMA)
39-68: Infisical Integration Example Provides Clear Guidance.The second code snippet (lines 39–68) effectively illustrates integration with Infisical, showcasing proper authentication via the
InfisicalSDKand mapping retrieved secrets to the expected format. It might also be beneficial for production readiness to consider including error handling mechanisms.docs/docs.json (1)
42-42: Validation of "apikeys" Entry
The inclusion of the "apikeys" page in the Fundamentals group should be cross-checked for naming consistency and proper placement within the documentation hierarchy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
docs/config/config-file.mdx(1 hunks)docs/config/extensions/additionalFiles.mdx(1 hunks)docs/config/extensions/additionalPackages.mdx(1 hunks)docs/config/extensions/aptGet.mdx(1 hunks)docs/config/extensions/audioWaveform.mdx(1 hunks)docs/config/extensions/custom.mdx(1 hunks)docs/config/extensions/emitDecoratorMetadata.mdx(1 hunks)docs/config/extensions/esbuildPlugin.mdx(1 hunks)docs/config/extensions/ffmpeg.mdx(1 hunks)docs/config/extensions/overview.mdx(2 hunks)docs/config/extensions/prismaExtension.mdx(1 hunks)docs/config/extensions/puppeteer.mdx(1 hunks)docs/config/extensions/pythonExtension.mdx(1 hunks)docs/config/extensions/syncEnvVars.mdx(1 hunks)docs/docs.json(8 hunks)docs/guides/examples/ffmpeg-video-processing.mdx(1 hunks)docs/guides/examples/libreoffice-pdf-conversion.mdx(1 hunks)docs/guides/examples/pdf-to-image.mdx(1 hunks)docs/guides/examples/puppeteer.mdx(2 hunks)docs/guides/examples/scrape-hacker-news.mdx(2 hunks)docs/guides/examples/sentry-error-tracking.mdx(1 hunks)docs/guides/examples/vercel-sync-env-vars.mdx(1 hunks)docs/guides/frameworks/prisma.mdx(2 hunks)docs/guides/frameworks/supabase-edge-functions-database-webhooks.mdx(1 hunks)docs/troubleshooting.mdx(1 hunks)packages/python/src/utils/tempFiles.ts(1 hunks)
✅ Files skipped from review due to trivial changes (14)
- docs/guides/frameworks/prisma.mdx
- docs/guides/examples/vercel-sync-env-vars.mdx
- docs/guides/examples/sentry-error-tracking.mdx
- docs/guides/examples/pdf-to-image.mdx
- docs/guides/examples/ffmpeg-video-processing.mdx
- docs/guides/examples/scrape-hacker-news.mdx
- docs/config/extensions/emitDecoratorMetadata.mdx
- docs/config/extensions/additionalFiles.mdx
- docs/config/extensions/audioWaveform.mdx
- docs/config/extensions/ffmpeg.mdx
- docs/guides/examples/puppeteer.mdx
- docs/guides/frameworks/supabase-edge-functions-database-webhooks.mdx
- docs/config/extensions/esbuildPlugin.mdx
- docs/guides/examples/libreoffice-pdf-conversion.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/python/src/utils/tempFiles.ts
🧰 Additional context used
🪛 LanguageTool
docs/config/config-file.mdx
[style] ~325-~325: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...rmation. #### emitDecoratorMetadata See the [emitDecoratorMetadata documentatio...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/config/extensions/aptGet.mdx
[duplication] ~7-~7: Possible typo: you repeated a word.
Context: ...system packages into the deployed image using using the aptGet extension: ```ts import {...
(ENGLISH_WORD_REPEAT_RULE)
docs/config/extensions/pythonExtension.mdx
[grammar] ~46-~46: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ...ripts to the build directory during the deploy process. For example: ```ts import { d...
(PREPOSITION_VERB)
[grammar] ~64-~64: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ...ctory to the build directory during the deploy process. You can then execute these scr...
(PREPOSITION_VERB)
[style] ~134-~134: Consider removing “of” to be more concise
Context: ...e Python scripts. ## Streaming output All of the python functions have a streaming ver...
(ALL_OF_THE)
docs/troubleshooting.mdx
[misspelling] ~101-~101: Did you mean “than”?
Context: ...current version, you can't perform more that one "wait" in parallel. Waits include:...
(THAT_THAN)
docs/config/extensions/custom.mdx
[style] ~196-~196: To form a complete sentence, be sure to include a subject.
Context: ...how to use addLayer. ## BuildTarget Can either be dev or deploy, matching t...
(MISSING_IT_THERE)
[uncategorized] ~302-~302: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...e run after packages have been installed and the code copied into the container in t...
(COMMA_COMPOUND_SENTENCE_2)
[style] ~302-~302: Consider a more expressive alternative.
Context: ...n't be available in the final stage. To do that, please use the pkgs property of...
(DO_ACHIEVE)
[grammar] ~380-~380: A hyphen is missing in the adjective “built-in”.
Context: ...``` You should also take a look at our built in extensions for inspiration on how to cr...
(BUILT_IN_HYPHEN)
[duplication] ~380-~380: Possible typo: you repeated a word.
Context: ...w to create your own. You can find them in in [the source code here](https://github.c...
(ENGLISH_WORD_REPEAT_RULE)
docs/config/extensions/syncEnvVars.mdx
[uncategorized] ~31-~31: Loose punctuation mark.
Context: ...e following properties: - environment: The environment name that the task is b...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~32-~32: Loose punctuation mark.
Context: ...uction, staging, etc.) - projectRef`: The project ref of the Trigger.dev proj...
(UNLIKELY_OPENING_PUNCTUATION)
[typographical] ~37-~37: It appears that a comma is missing.
Context: ...: Sync env vars from Infisical In this example we're using env vars from [Infisical](h...
(DURING_THAT_TIME_COMMA)
docs/config/extensions/prismaExtension.mdx
[grammar] ~10-~10: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ... Generates the Prisma client during the deploy process - Optionally will migrate the d...
(PREPOSITION_VERB)
[grammar] ~11-~11: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ...ly will migrate the database during the deploy process - Support for TypedSQL and mult...
(PREPOSITION_VERB)
[uncategorized] ~144-~144: Possible missing comma found.
Context: ...e multiple separate schemas in the same project you can add the extension multiple time...
(AI_HYDRA_LEO_MISSING_COMMA)
docs/config/extensions/additionalPackages.mdx
[style] ~22-~22: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ckage that includes a CLI tool that you want to invoke in your tasks via exec. We wil...
(REP_WANT_TO_VB)
[uncategorized] ~22-~22: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...cally resolve the version of the package but you can specify the version by using th...
(COMMA_COMPOUND_SENTENCE)
🪛 GitHub Actions: 📚 Docs Checks
docs/config/config-file.mdx
[error] 1-1: 1 broken link found: /config/extensions/esbuild-plugins.
⏰ 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: units / 🧪 Unit Tests
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (51)
docs/config/extensions/aptGet.mdx (3)
1-6: Accurate Metadata and Document Header.The metadata block correctly sets the document title, sidebar title, and description for the
aptGetextension documentation.
9-20: Well-Formatted Example Code Block.The TypeScript example demonstrating how to install a package (e.g.,
ffmpeg) is clear, accurate, and serves its purpose in illustrating the usage of theaptGetextension.
22-34: Clear Example for Installing a Specific Version.The second example effectively shows how to specify a package version (e.g.,
ffmpeg=6.0-4). The code snippet is well-structured and consistent with the documentation’s intent.docs/config/config-file.mdx (7)
317-318: Clarify and Validate Extension Reference for additionalFiles.
The updated text now directs users to the dedicated documentation page for theadditionalFilesextension. Please verify that the link/config/extensions/additionalFilesis correct and that the wording remains clear in the overall context.
322-323: Concise Reference for additionalPackages Extension.
The updated sentence now succinctly refers users to the new additional documentation page. Verify that the link/config/extensions/additionalPackagesis accurate and that the style is consistent with the other extension references.
330-331: Accurate Redirection for Prisma Extension.
The updated reference now correctly points users to the/config/extensions/prismaExtensionpage. Ensure that this newly referenced documentation thoroughly covers any changes related to Prisma client configuration.
334-335: Validate the Sync Environment Variables Link.
The updated sentence now directs users to the dedicated sync environment variables documentation. Confirm that the link/config/extensions/syncEnvVarsis correct and that it fits seamlessly within the documentation’s overall style.
338-339: Ensure Puppeteer Documentation Link Accuracy.
This change succinctly refers users to the new Puppeteer documentation at/config/extensions/puppeteer. Please verify the link and ensure that the brief guidance provided is sufficiently informative for users seeking setup instructions.
342-343: Confirm FFmpeg Documentation Reference.
The new documentation reference for the FFmpeg extension appears clear. Double-check that the link/config/extensions/ffmpegis active and that users can find detailed usage instructions there.
350-351: Validate aptGet Documentation Reference.
The updated line directs users to/config/extensions/aptGetfor further details on the aptGet extension. Confirm that the link is accurate and that the concise description aligns with the intended user guidance.docs/config/extensions/puppeteer.mdx (1)
1-31: New Puppeteer Extension Documentation File.
This new file provides clear and concise instructions for integrating the Puppeteer build extension. The inclusion of the<ScrapingWarning />component and the detailed code snippet for updating thetrigger.config.tsfile offer a helpful guide for users. Please ensure that the example link to/guides/examples/puppeteeris active and that the instructions on setting thePUPPETEER_EXECUTABLE_PATHenvironment variable are correct.docs/config/extensions/additionalPackages.mdx (3)
1-8: New Additional Packages Extension Documentation – Header and Introduction.
The header and introductory text clearly explain the purpose of the additionalPackages extension. The provided title, sidebar title, and description match the intended usage. Overall, the introduction sets the context well for the more detailed examples that follow.
9-20: Clear Example for Including Additional Packages.
The first code example demonstrates how to import and use theadditionalPackagesextension effectively. The example is concise and correctly configured for a basic use case.
24-34: Alternate Example for Specifying Package Versions.
The second code example, which shows how to include a specific version (e.g."wrangler@1.19.0"), is well presented. This provides users with a practical reference for when they need to pin package versions.docs/troubleshooting.mdx (1)
97-98: Updated Guidance for Prisma Client Initialization.
The revised sentence now directs users to the/config/extensions/prismaExtensionpage when encountering the@prisma/client did not initialize yet.error. This update improves clarity regarding the configuration steps needed.docs/config/extensions/overview.mdx (5)
3-3: Update Sidebar Title for Consistency.
The updated sidebar title “Overview” succinctly represents the content of this page. This helps differentiate it from more specific extension documentation pages.
11-12: Clarify How Build Extensions Are Applied.
The statement “Build extensions are added to yourtrigger.config.tsfile under thebuild.extensionsproperty:” neatly explains where extensions should be specified. This clarity is beneficial for users configuring their projects.
31-32: Direct Users to Pre-built Extension Imports.
The sentence “If you are using a pre-built extension, you can import it from the@trigger.dev/buildpackage:” clearly introduces the next code snippet example. The guidance is concise and effective.
33-43: Accurate Code Example for Importing a Pre-built Extension.
The code snippet demonstrating the import of theffmpegextension from@trigger.dev/build/extensions/coreis clear and correctly formatted. This example provides a useful reference for users looking to implement pre-built extensions.
64-67: Clear Introduction for Custom Extensions.
The concluding section on custom extensions directs users to a guide on creating their own build extensions. This final note successfully wraps up the overview content.docs/config/extensions/prismaExtension.mdx (4)
16-34: Simple Prisma Setup Example – Looks Good!
The code snippet demonstrating a basic Prisma setup is clear and easy to follow.
35-62: Migrations Example – Approved
The migration example with themigrateoption is well presented and instructive.
64-93: Client Generator Example – Approved
The example showing the use of theclientGeneratoroption is clear and correctly demonstrates how to prevent generation of unwanted client generators.
114-133: TypedSQL Example – Approved
The TypedSQL section and its corresponding code snippet are clear and informative.docs/config/extensions/custom.mdx (3)
1-8: YAML Header & Introduction – Approved
The YAML header and the introductory text set a clear context for custom build extensions.
22-38: Custom Extension Example – Approved
The inline example demonstrating a simple custom build extension is clear and serves as a good starting point.
47-66: Extracted Build Extension Example – Approved
The example showing how to extract the build extension into a separate function is clear and well documented.docs/config/extensions/pythonExtension.mdx (6)
1-8: YAML Header & Introduction – Approved
The header and introduction clearly present the purpose of the Python extension and its integration.
9-14: Installation Instructions – Approved
The instructions for installing the@trigger.dev/pythonpackage are clear and concise.
15-27: Configuration Example – Approved
The code snippet showing how to configure the Python extension intrigger.config.tsis well written and easy to follow.
29-42: Inline Python Script Example – Approved
The example demonstrating inline Python script execution is clear and effectively shows how to capture output.
84-102: Using Requirements Files – Approved
The requirements file usage example is concise and clearly explains how packages inrequirements.txtare handled during the build process.
111-129: Virtual Environments Configuration – Approved
The explanation and example for specifying a virtual environment’s Python binary are clear and appropriate.docs/config/extensions/syncEnvVars.mdx (6)
1-5: Document Header Formatting is Correct.The YAML metadata block (lines 1–5) is well-structured and provides all necessary details (title, sidebarTitle, and description) for the documentation.
7-10: Clear Introduction and Usage Description.The introductory paragraphs concisely explain the purpose of the
syncEnvVarsextension and how its async callback provides environment variables for synchronization.
11-27: SyncEnvVars Basic Usage Example is Well-Structured.The first code snippet (lines 11–27) clearly demonstrates how to integrate the
syncEnvVarsextension within a Trigger.dev configuration. The sample is easy to follow and correctly shows the use of an async callback returning an array of environment variable objects.
70-81: Clear Explanation for syncVercelEnvVars.The documentation (lines 70–81) for the
syncVercelEnvVarsextension is detailed and instructive, providing clear prerequisites and configuration advice for users syncing Vercel environment variables.
83-95: Default syncVercelEnvVars Usage is Correct.The provided code snippet (lines 83–95) shows the standard usage of
syncVercelEnvVars, leveraging environment variables (VERCEL_ACCESS_TOKENandVERCEL_PROJECT_ID). The example is accurate and well-aligned with expected usage patterns.
97-117: Alternative Vercel Env Vars Sync Usage is Clear.The alternative example (lines 97–117) demonstrates how to explicitly pass the token and project ID as arguments. This alternative approach is clear and useful for scenarios where the environment variables may not be pre-set.
docs/docs.json (12)
21-27: Enhanced "Getting started" Navigation Pages Formatting
The added list items ("introduction", "quick-start", "video-walkthrough", "how-it-works", "limits") improve readability and navigation. Please verify that these page identifiers correctly map to the existing documentation files.
34-38: Updated "Tasks" Subgroup in Fundamentals
The revised pages ("tasks/overview", "tasks/schemaTask", "tasks/scheduled") under the "Tasks" subgroup provide a clearer structure for task-related documentation. Ensure that these paths match the actual content locations.
52-59: Reorganization of the "Wait" Subgroup under "Writing tasks"
The updated list for the "Wait" subgroup ("wait", "wait-for", "wait-until", "wait-for-event", "wait-for-request") enhances clarity and grouping. Confirm that this structure accurately reflects the intended documentation flow.
69-70: Addition of "context" Page in Writing Tasks
Adding "context" extends the documentation related to writing tasks. Please ensure that accompanying content is updated and that the new section integrates seamlessly with related pages.
73-100: Restructured "Configuration" Group Navigation
The nested grouping—introducing a "Build extensions" subgroup with further nested "Built-in extensions"—improves categorical clarity. Verify that all referenced paths (such as "config/config-file", "config/extensions/pythonExtension", etc.) are correct and that the new hierarchy is reflected in the docs.
101-106: Simplified "Development" Group Entry
The concise inclusion of only the "cli-dev" page creates a focused navigation entry for development. Ensure that any other development-related resources are referenced appropriately elsewhere.
165-173: Introduction of the "Using the Dashboard" Group
The new group consolidates dashboard-related pages ("run-tests", "troubleshooting-alerts", "replaying", "bulk-actions"), which enhances accessibility. Confirm that pages like "troubleshooting-alerts" are correctly repositioned from previous groups (e.g., "Troubleshooting").
197-203: Refinement of the "Help" Group Navigation
The streamlined list ("community", "help-slack", "help-email") in the "Help" group improves the support interface. Please check that these pages exist and provide the intended assistance.
386-389: Standardized OpenAPI Array in API Section
Specifying both "openapi.yml" and "v3-openapi.yaml" clarifies the available API specification versions. Ensure these files are maintained consistently with the documentation schema.
224-227: Updated "Tasks API" Navigation Entries
The modifications adding "management/tasks/trigger" and "management/tasks/batch-trigger" improve API documentation specificity. Validate these endpoint references for correctness.
271-275: Refined "Introduction" Subgroup within Guides & Examples
Consolidating the guidance to a single "guides/introduction" entry helps improve discoverability for new users. Verify that this serves as an effective entry point to the guides.
564-564: Final JSON Formatting Review
The slight change on the final line appears to be a formatting adjustment. Please confirm that the JSON file remains valid against the defined schema.
ericallam
force-pushed
the
feat/python-extension-improvements
branch
from
8ee1f3a to
dcea320
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
docs/config/extensions/aptGet.mdx (1)
22-34: Consider Adding the Missing Import in the Second Code Example.
For consistency and to ensure the snippet is self-contained, it is recommended to include the import foraptGetin the second example as well. This will help avoid confusion for users copying the snippet directly.-import { defineConfig } from "@trigger.dev/sdk/v3"; +import { defineConfig } from "@trigger.dev/sdk/v3"; +import { aptGet } from "@trigger.dev/build/extensions/core";docs/config/extensions/additionalPackages.mdx (1)
22-23: Descriptive Text: Punctuation and Style Suggestion
In the sentence starting on line 22, consider inserting a comma before "but" (i.e. “…automatically resolve the version of the package, but you can specify…”) to improve readability. Additionally, varying the phrasing slightly may avoid repetition with nearby sentences.🧰 Tools
🪛 LanguageTool
[style] ~22-~22: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ckage that includes a CLI tool that you want to invoke in your tasks viaexec. We wil...(REP_WANT_TO_VB)
[uncategorized] ~22-~22: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...cally resolve the version of the package but you can specify the version by using th...(COMMA_COMPOUND_SENTENCE)
docs/config/config-file.mdx (1)
316-350: Streamlined Extensions Section with External Documentation Links
The updated “Extensions” section now focuses on linking out to detailed documentation for each extension (e.g. additionalFiles, additionalPackages, emitDecoratorMetadata, etc.). This concise approach aids in keeping the config file documentation lean. One suggestion: to further assist users unfamiliar with the extensions, consider adding a brief one-line summary for each extension (or a “further reading” link) so that they understand what functionality each extension provides without needing to navigate away immediately.Additionally, note that the repeated use of “See the [...] documentation for more information.” across these items might benefit from slight rephrasing to vary sentence construction.
🧰 Tools
🪛 LanguageTool
[style] ~325-~325: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...rmation. ####emitDecoratorMetadataSee the [emitDecoratorMetadata documentatio...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/config/extensions/custom.mdx (1)
42-46: Enhance Note Clarity
In the<Note>block, consider rephrasing “Make sure it's version matches that of the installed@trigger.dev/sdkpackage” to “Make sure its version matches the installed@trigger.dev/sdkpackage version” for improved clarity and grammatical correctness.docs/config/extensions/prismaExtension.mdx (1)
10-11: Refine Terminology for Deployment Process
The list items mention phrases like “during the deploy process.” For improved clarity and correctness, consider replacing “deploy process” with “deployment process” throughout this section.-Generates the Prisma client during the deploy process -Optionally will migrate the database during the deploy process +Generates the Prisma client during the deployment process +Optionally migrates the database during the deployment process🧰 Tools
🪛 LanguageTool
[grammar] ~10-~10: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ... Generates the Prisma client during the deploy process - Optionally will migrate the d...(PREPOSITION_VERB)
[grammar] ~11-~11: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ...ly will migrate the database during the deploy process - Support for TypedSQL and mult...(PREPOSITION_VERB)
docs/config/extensions/pythonExtension.mdx (1)
134-134: Nitpick: Remove Superfluous Preposition
The sentence “All of thepythonfunctions have a streaming version…” could be streamlined to “Allpythonfunctions have a streaming version…” for brevity.🧰 Tools
🪛 LanguageTool
[style] ~134-~134: Consider removing “of” to be more concise
Context: ...e Python scripts. ## Streaming output All of thepythonfunctions have a streaming ver...(ALL_OF_THE)
docs/config/extensions/syncEnvVars.mdx (2)
31-33: Fix punctuation in list items.Each bullet point starts with a loose punctuation mark. Consider fixing the formatting to ensure proper rendering.
-\- `environment`: The environment name that the task is being deployed to (e.g. `production`, `staging`, etc.) -\- `projectRef`: The project ref of the Trigger.dev project -\- `env`: The environment variables that are currently set in the Trigger.dev project +- `environment`: The environment name that the task is being deployed to (e.g. `production`, `staging`, etc.) +- `projectRef`: The project ref of the Trigger.dev project +- `env`: The environment variables that are currently set in the Trigger.dev project🧰 Tools
🪛 LanguageTool
[uncategorized] ~31-~31: Loose punctuation mark.
Context: ...e following properties: -environment: The environment name that the task is b...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~32-~32: Loose punctuation mark.
Context: ...uction,staging, etc.) -projectRef`: The project ref of the Trigger.dev proj...(UNLIKELY_OPENING_PUNCTUATION)
37-38: Add a comma after the introductory phrase.A comma should be added after "In this example" for better readability.
-In this example we're using env vars from [Infisical](https://infisical.com). +In this example, we're using env vars from [Infisical](https://infisical.com).🧰 Tools
🪛 LanguageTool
[typographical] ~37-~37: It appears that a comma is missing.
Context: ...: Sync env vars from Infisical In this example we're using env vars from [Infisical](h...(DURING_THAT_TIME_COMMA)
packages/python/README.md (1)
80-94: Fix variable name inconsistency in streaming example.The streaming example uses
streamingResultin the for loop, but the variable is namedresultwhen it's defined.- const result = python.stream.runScript("my_script.py", ["hello", "world"]); + const streamingResult = python.stream.runScript("my_script.py", ["hello", "world"]); // result is an async iterable/readable stream for await (const chunk of streamingResult) {packages/python/src/index.ts (2)
142-166: Consider adding error handling to stream.runUnlike the non-streaming counterparts, the streaming methods don't check for non-zero exit codes or throw errors on failure. This could lead to silent failures where consumers don't know a command failed unless they parse the output themselves.
You might want to add error handling by modifying the transform function to check the process result:
return createAsyncIterableStreamFromAsyncIterable(pythonProcess, { - transform: (chunk, controller) => { + transform: (chunk, controller, process) => { controller.enqueue(chunk); + + // Check for errors if the process has exited + if (process.exitCode !== undefined && process.exitCode !== 0) { + span.setAttribute("exitCode", process.exitCode); + controller.error(new Error( + `${scriptArgs.join(" ")} exited with a non-zero code ${process.exitCode}` + )); + } }, flush: () => { span.end(); }, });
141-230: Consider reducing duplication between streaming and non-streaming implementationsThere's significant code duplication between the streaming and non-streaming methods. Consider extracting common logic like Python binary resolution, argument handling, and span attribute setup into helper functions.
For example, you could create a helper function:
function getPythonBinAndSpanAttributes( scriptPath?: string, scriptArgs: string[] = [], scriptContent?: string ) { const pythonBin = process.env.PYTHON_BIN_PATH || "python"; const attributes: Record<string, any> = { pythonBin, [SemanticInternalAttributes.STYLE_ICON]: "brand-python", }; if (scriptArgs.length > 0) { attributes.args = scriptArgs.join(" "); } if (scriptPath) { attributes.scriptPath = scriptPath; } if (scriptContent) { attributes.contentPreview = scriptContent.substring(0, 100) + (scriptContent.length > 100 ? "..." : ""); } return { pythonBin, attributes }; }This would reduce duplication and make future updates more maintainable.
📜 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 (51)
docs/config/config-file.mdx(1 hunks)docs/config/extensions/additionalFiles.mdx(1 hunks)docs/config/extensions/additionalPackages.mdx(1 hunks)docs/config/extensions/aptGet.mdx(1 hunks)docs/config/extensions/audioWaveform.mdx(1 hunks)docs/config/extensions/custom.mdx(1 hunks)docs/config/extensions/emitDecoratorMetadata.mdx(1 hunks)docs/config/extensions/esbuildPlugin.mdx(1 hunks)docs/config/extensions/ffmpeg.mdx(1 hunks)docs/config/extensions/overview.mdx(2 hunks)docs/config/extensions/prismaExtension.mdx(1 hunks)docs/config/extensions/puppeteer.mdx(1 hunks)docs/config/extensions/pythonExtension.mdx(1 hunks)docs/config/extensions/syncEnvVars.mdx(1 hunks)docs/docs.json(8 hunks)docs/guides/examples/ffmpeg-video-processing.mdx(1 hunks)docs/guides/examples/libreoffice-pdf-conversion.mdx(1 hunks)docs/guides/examples/pdf-to-image.mdx(1 hunks)docs/guides/examples/puppeteer.mdx(2 hunks)docs/guides/examples/scrape-hacker-news.mdx(2 hunks)docs/guides/examples/sentry-error-tracking.mdx(1 hunks)docs/guides/examples/vercel-sync-env-vars.mdx(1 hunks)docs/guides/frameworks/prisma.mdx(2 hunks)docs/guides/frameworks/supabase-edge-functions-database-webhooks.mdx(1 hunks)docs/troubleshooting.mdx(1 hunks)packages/build/package.json(3 hunks)packages/build/src/extensions/core/additionalFiles.ts(2 hunks)packages/build/src/internal.ts(1 hunks)packages/build/src/internal/additionalFiles.ts(1 hunks)packages/core/src/v3/apiClient/index.ts(1 hunks)packages/core/src/v3/apiClient/runStream.ts(1 hunks)packages/core/src/v3/apiClient/stream.ts(1 hunks)packages/core/src/v3/index.ts(1 hunks)packages/core/src/v3/logger/index.ts(2 hunks)packages/core/src/v3/logger/taskLogger.ts(3 hunks)packages/core/src/v3/runMetadata/index.ts(1 hunks)packages/core/src/v3/runMetadata/manager.ts(1 hunks)packages/core/src/v3/runMetadata/noopManager.ts(1 hunks)packages/core/src/v3/runMetadata/types.ts(1 hunks)packages/core/src/v3/streams/asyncIterableStream.ts(1 hunks)packages/python/README.md(4 hunks)packages/python/package.json(1 hunks)packages/python/src/extension.ts(4 hunks)packages/python/src/index.ts(1 hunks)packages/python/src/utils/tempFiles.ts(1 hunks)references/python-catalog/package.json(1 hunks)references/python-catalog/requirements.txt(1 hunks)references/python-catalog/src/python/html2text_url.py(1 hunks)references/python-catalog/src/trigger/pythonTasks.ts(1 hunks)references/python-catalog/trigger.config.ts(1 hunks)references/python-catalog/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- references/python-catalog/package.json
🚧 Files skipped from review as they are similar to previous changes (33)
- docs/guides/examples/vercel-sync-env-vars.mdx
- docs/guides/examples/pdf-to-image.mdx
- docs/guides/examples/puppeteer.mdx
- docs/guides/frameworks/prisma.mdx
- packages/build/src/internal.ts
- packages/core/src/v3/index.ts
- docs/guides/examples/sentry-error-tracking.mdx
- packages/core/src/v3/runMetadata/index.ts
- docs/guides/examples/scrape-hacker-news.mdx
- docs/guides/examples/libreoffice-pdf-conversion.mdx
- docs/guides/frameworks/supabase-edge-functions-database-webhooks.mdx
- docs/config/extensions/additionalFiles.mdx
- packages/core/src/v3/runMetadata/noopManager.ts
- references/python-catalog/tsconfig.json
- docs/config/extensions/puppeteer.mdx
- packages/core/src/v3/runMetadata/types.ts
- references/python-catalog/src/trigger/pythonTasks.ts
- references/python-catalog/requirements.txt
- packages/core/src/v3/runMetadata/manager.ts
- docs/guides/examples/ffmpeg-video-processing.mdx
- packages/core/src/v3/apiClient/runStream.ts
- packages/core/src/v3/logger/index.ts
- docs/config/extensions/esbuildPlugin.mdx
- docs/config/extensions/emitDecoratorMetadata.mdx
- packages/core/src/v3/logger/taskLogger.ts
- docs/config/extensions/audioWaveform.mdx
- packages/core/src/v3/apiClient/index.ts
- references/python-catalog/trigger.config.ts
- references/python-catalog/src/python/html2text_url.py
- packages/python/src/extension.ts
- packages/python/src/utils/tempFiles.ts
- packages/build/src/internal/additionalFiles.ts
- packages/core/src/v3/streams/asyncIterableStream.ts
🧰 Additional context used
🪛 LanguageTool
docs/config/extensions/pythonExtension.mdx
[grammar] ~46-~46: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ...ripts to the build directory during the deploy process. For example: ```ts import { d...
(PREPOSITION_VERB)
[grammar] ~64-~64: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ...ctory to the build directory during the deploy process. You can then execute these scr...
(PREPOSITION_VERB)
[style] ~134-~134: Consider removing “of” to be more concise
Context: ...e Python scripts. ## Streaming output All of the python functions have a streaming ver...
(ALL_OF_THE)
docs/config/extensions/custom.mdx
[style] ~196-~196: To form a complete sentence, be sure to include a subject.
Context: ...how to use addLayer. ## BuildTarget Can either be dev or deploy, matching t...
(MISSING_IT_THERE)
[uncategorized] ~302-~302: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...e run after packages have been installed and the code copied into the container in t...
(COMMA_COMPOUND_SENTENCE_2)
[style] ~302-~302: Consider a more expressive alternative.
Context: ...n't be available in the final stage. To do that, please use the pkgs property of...
(DO_ACHIEVE)
[grammar] ~380-~380: A hyphen is missing in the adjective “built-in”.
Context: ...``` You should also take a look at our built in extensions for inspiration on how to cr...
(BUILT_IN_HYPHEN)
[duplication] ~380-~380: Possible typo: you repeated a word.
Context: ...w to create your own. You can find them in in [the source code here](https://github.c...
(ENGLISH_WORD_REPEAT_RULE)
docs/config/config-file.mdx
[style] ~325-~325: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...rmation. #### emitDecoratorMetadata See the [emitDecoratorMetadata documentatio...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/config/extensions/syncEnvVars.mdx
[uncategorized] ~31-~31: Loose punctuation mark.
Context: ...e following properties: - environment: The environment name that the task is b...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~32-~32: Loose punctuation mark.
Context: ...uction, staging, etc.) - projectRef`: The project ref of the Trigger.dev proj...
(UNLIKELY_OPENING_PUNCTUATION)
[typographical] ~37-~37: It appears that a comma is missing.
Context: ...: Sync env vars from Infisical In this example we're using env vars from [Infisical](h...
(DURING_THAT_TIME_COMMA)
docs/config/extensions/additionalPackages.mdx
[style] ~22-~22: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ckage that includes a CLI tool that you want to invoke in your tasks via exec. We wil...
(REP_WANT_TO_VB)
[uncategorized] ~22-~22: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...cally resolve the version of the package but you can specify the version by using th...
(COMMA_COMPOUND_SENTENCE)
docs/config/extensions/aptGet.mdx
[duplication] ~7-~7: Possible typo: you repeated a word.
Context: ...system packages into the deployed image using using the aptGet extension: ```ts import {...
(ENGLISH_WORD_REPEAT_RULE)
docs/config/extensions/prismaExtension.mdx
[grammar] ~10-~10: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ... Generates the Prisma client during the deploy process - Optionally will migrate the d...
(PREPOSITION_VERB)
[grammar] ~11-~11: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ...ly will migrate the database during the deploy process - Support for TypedSQL and mult...
(PREPOSITION_VERB)
docs/troubleshooting.mdx
[misspelling] ~101-~101: Did you mean “than”?
Context: ...current version, you can't perform more that one "wait" in parallel. Waits include:...
(THAT_THAN)
⏰ 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: typecheck / typecheck
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (44)
packages/core/src/v3/apiClient/stream.ts (1)
12-12: Good refactoring to centralize stream functionality.The code now imports
AsyncIterableStreamandcreateAsyncIterableStreamfrom a dedicated streams module rather than defining them in this file. This change improves code organization by centralizing stream-related functionality in a single location, making it easier to maintain and reuse across the codebase.docs/config/extensions/aptGet.mdx (2)
7-7: Fix Duplicate Word in Description Text.
There is a duplicate occurrence of the word “using” which affects clarity. Please remove the redundant word.-You can install system packages into the deployed image using using the `aptGet` extension: +You can install system packages into the deployed image using the `aptGet` extension:🧰 Tools
🪛 LanguageTool
[duplication] ~7-~7: Possible typo: you repeated a word.
Context: ...system packages into the deployed image using using theaptGetextension: ```ts import {...(ENGLISH_WORD_REPEAT_RULE)
9-20: Code Example for Basic Package Installation Approved.
The first code snippet clearly demonstrates how to import and use theaptGetextension within the configuration. The example is self-contained and easy to follow.docs/config/extensions/additionalPackages.mdx (5)
1-5: New Documentation Frontmatter and Metadata
The frontmatter is well structured with a clear title, sidebarTitle, and description.
7-8: Introduction Text for Usage Example
The introductory sentence clearly guides users on how to import and use theadditionalPackagesextension.
9-20: Code Example for Basic Usage
The code block demonstrating the basic configuration withadditionalPackagesis clear and concise.
24-34: Code Example with Version Specification
The second code block showing how to specify a package version (e.g."wrangler@1.19.0") is clear and provides helpful context for users needing to pin versions.
36-37: Clarification on Extension Behavior in Different Modes
The explanation on line 36 regarding the extension’s behavior indevversusdeploymodes is concise and informative.docs/config/extensions/ffmpeg.mdx (1)
1-42: FFmpeg Extension Documentation Looks Clear and Informative
This new documentation file for the FFmpeg extension is well structured. The frontmatter, code examples, and descriptive text effectively explain how to integrate and optionally pin the FFmpeg version. The note regarding settingFFMPEG_PATHandFFPROBE_PATHand the external dependency forfluent-ffmpegis clear.packages/python/package.json (2)
58-65: Dependency Restructuring is Consistent
The movement of@trigger.dev/buildand@trigger.dev/sdkfrom thedependenciessection todevDependencies—with their inclusion aspeerDependencies—is a sound decision for better modularity and dependency management. This change should help avoid versioning issues for consumers of this package.
47-67: Ensure Downstream Compatibility
Double-check that any consumers or integrations with the Python extension are updated to handle the new dependency classification. This may involve verifying that peer dependency warnings (if any) are addressed in consuming projects.docs/troubleshooting.mdx (3)
96-98: Updated Prisma Client Error Guidance
The URL update in the troubleshooting section for the errorError: @prisma/client did not initialize yet.correctly reflects the new documentation location (/config/extensions/prismaExtension). This change will help users find the appropriate troubleshooting guide.
100-102: Typographical Correction in Parallel Waits Limitation
On line 101, the text currently states “…more that one 'wait' in parallel.” This should be corrected to “more than one 'wait' in parallel.”- can't perform more that one "wait" in parallel. + can't perform more than one "wait" in parallel.This comment echoes previous feedback; please ensure the correction is applied consistently.
🧰 Tools
🪛 LanguageTool
[misspelling] ~101-~101: Did you mean “than”?
Context: ...current version, you can't perform more that one "wait" in parallel. Waits include:...(THAT_THAN)
103-110: Addition oftask.batchTriggerAndWait()in Wait Functions List
Includingtask.batchTriggerAndWait()in the list of unsupported parallel wait functions is a welcome enhancement that ensures users are fully informed of the limitations regarding parallel waits.docs/config/extensions/overview.mdx (3)
3-3: Clarify Sidebar Title Consistency
The new sidebar title"Overview"is clear, but please ensure it aligns with naming conventions used in other extension documentation for consistency.
9-9: Confirm Clarity of Pre-built Extensions Description
The line “You can use pre-built extensions by installing the@trigger.dev/buildpackage into yourdevDependencies…” is clear. Ensure that examples in subsequent documentation consistently reference the same package and usage.
45-60: Ensure Consistent Naming and Correct Links in Extensions Table
The built-in extensions table appears to list various extensions. Notably, the[esbuildPlugin](/config/extensions/esbuildPlugin)entry seems inconsistent with other naming conventions. Please verify and update the link text (and URL) so it properly aligns with the established naming pattern (for example, consider renaming to something like/config/extensions/esbuild-plugins) and resolves any broken links.docs/config/extensions/custom.mdx (1)
22-38: Verify Simplicity of Extension Example
The inline extension example is concise and clear. Just double-check that the example aligns with all recent internal API changes.docs/config/extensions/prismaExtension.mdx (2)
18-34: Validate Code Example for Basic Prisma Setup
The example configuration shows proper usage of theprismaExtension. Ensure that the placeholder<project ref>is clearly documented elsewhere for user guidance.
36-40: Clarify Note on Command Effectiveness
The<Note>clarifying that the extension does not affect thedevcommand is useful. Verify that similar notes appear in related documentation for consistency.docs/config/extensions/pythonExtension.mdx (1)
9-13: Ensure Installation Instructions Are Clear
The installation instructions usingnpm add @trigger.dev/pythonare clear. Confirm that users understand if additional installation steps are needed based on their environment (e.g., in virtual environments).packages/build/package.json (4)
26-26: Verify New Export for Internal Module
The new export entry"./internal": "./src/internal.ts"has been added. Please confirm that consumers relying on internal APIs have updated their import paths accordingly.
40-42: Confirm TypesVersions Mapping for Internal Module
The addition of the"internal"mapping in thetypesVersionssection (mapping todist/commonjs/internal.d.ts) appears correct. Verify that the build output includes this file in the expected location.
102-112: Review Export Structure for Internal Module
The detailed export structure for"./internal"now includes both ESM and CommonJS configurations. This structure looks well organized. Please ensure that downstream consumers using bothimportandrequirewill receive the correct type definitions and default exports.
90-101: Validate Consistency Across Exports
The exports for the main package (lines 90-101) and the new"./internal"entry should be consistently structured. Make sure that similar patterns are followed for other internal modules to maintain predictable behavior.docs/config/extensions/syncEnvVars.mdx (3)
9-27: LGTM! Clear documentation with good examples.The explanation and example for the
syncEnvVarsextension are clear and easy to follow. The code example effectively demonstrates the basic usage pattern.
47-64: LGTM! Comprehensive example for Infisical integration.The Infisical integration example is thorough and demonstrates proper context usage, including how to adapt to different environments.
70-116: LGTM! Well-documented Vercel env vars sync with options.The
syncVercelEnvVarsextension is well-documented with both default usage and parameterized examples. The note about required environment variables is particularly helpful.packages/python/README.md (5)
17-17: Variable name update to be more descriptive.The renamed variable
devPythonBinaryPathmore clearly indicates that this is for development environments only, which is a good clarification.
25-25: Updated import to use named imports.Switching to named imports is more consistent with modern JavaScript/TypeScript practices.
33-34: Good configuration options for Python extension.The options now include the renamed
devPythonBinaryPathand a newscriptsoption that accepts glob patterns, providing more flexibility.
43-62: LGTM! Added concrete examples for requirements.txt and configuration.These additional examples make it clearer how to set up Python dependencies and configure the extension properly.
109-109: LGTM! Fixed pandas reference.Correctly using the
pdalias for pandas instead of an incorrect reference.packages/build/src/extensions/core/additionalFiles.ts (2)
2-2: LGTM! Imported centralized function for handling additional files.Moving the implementation to a shared internal module is good for maintainability and reuse.
12-12: Simplified implementation with delegated functionality.The complex implementation has been nicely abstracted into a single function call, reducing code duplication and making this extension easier to maintain.
docs/docs.json (4)
21-27: LGTM! Improved formatting for better readability.The improved formatting with indentation and line breaks makes the navigation structure more readable and maintainable.
73-99: LGTM! Well-organized Configuration section with logical grouping.The new "Configuration" section with nested "Build extensions" group provides a clearer organization for configuration-related documentation.
165-173: LGTM! New dashboard section improves navigation.Creating a dedicated "Using the Dashboard" section makes it easier for users to find dashboard-related documentation.
386-389: LGTM! Consistent formatting for API configuration.The OpenAPI specification references are now properly formatted with indentation.
packages/python/src/index.ts (5)
1-10: Good import structure with clear dependenciesThe imports are well-organized, bringing in necessary utilities from the Trigger.dev ecosystem while also including Node.js built-ins and the new temp file utilities. Particularly good to see the explicit imports from node: namespace.
12-46: Well-structured Python execution method with thorough error handlingThe
python.runmethod is well-implemented with:
- Proper environment variable fallback for Python binary
- Comprehensive tracing and span attribution
- Clear error handling for non-zero exit codes
- Good attribute context for debugging
The implementation correctly avoids using
throwOnError: trueto handle errors manually, providing better context.
48-93: Good validation and error messaging in runScript methodI like the assertions to validate script existence before execution and the thorough error message that includes both script path and arguments when failures occur.
95-139: Great improvement with withTempFile handlingThe
runInlineimplementation is significantly improved by using thewithTempFileutility for automatic cleanup. This ensures temporary files are properly managed regardless of execution outcome.
205-229: Clean up temp files post-stream usage.While
createTempFileSyncis convenient, the temp file remains after the process completes. If a large number of inline scripts are called, this can lead to clutter. Consider adding logic to remove the temporary file once the streaming is finalized.Replace the current implementation with the
withTempFileapproach used in the non-streaming version:- const pythonScriptPath = createTempFileSync(`script_${Date.now()}.py`, scriptContent); - - const pythonProcess = x(pythonBin, [pythonScriptPath], { - ...options, - throwOnError: false, - }); - - const span = logger.startSpan("python.stream.runInline()", { - attributes: { - pythonBin, - contentPreview: - scriptContent.substring(0, 100) + (scriptContent.length > 100 ? "..." : ""), - [SemanticInternalAttributes.STYLE_ICON]: "brand-python", - }, - }); - - return createAsyncIterableStreamFromAsyncIterable(pythonProcess, { - transform: (chunk, controller) => { - controller.enqueue(chunk); - }, - flush: () => { - span.end(); - }, - }); + const span = logger.startSpan("python.stream.runInline()", { + attributes: { + pythonBin, + contentPreview: + scriptContent.substring(0, 100) + (scriptContent.length > 100 ? "..." : ""), + [SemanticInternalAttributes.STYLE_ICON]: "brand-python", + }, + }); + + let stream: AsyncIterableStream<string>; + withTempFile( + `script_${Date.now()}.py`, + (tempFilePath) => { + span.setAttribute("tempFilePath", tempFilePath); + + const pythonProcess = x(pythonBin, [tempFilePath], { + ...options, + throwOnError: false, + }); + + stream = createAsyncIterableStreamFromAsyncIterable(pythonProcess, { + transform: (chunk, controller) => { + controller.enqueue(chunk); + }, + flush: () => { + span.end(); + }, + }); + + return stream; + }, + scriptContent + ); + + return stream!;Note: This implementation is more complex due to the streaming nature. You may need to adjust the approach to properly handle the temporary file lifecycle with streaming operations.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
ericallam
force-pushed
the
feat/python-extension-improvements
branch
from
bdd1c76 to
98b44bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
packages/core/src/v3/logger/taskLogger.ts (1)
112-114:⚠️ Potential issueNoopTaskLogger's startSpan signature doesn't match interface
The implementation of
startSpanin NoopTaskLogger doesn't match the interface signature. It should accept the same parameters as defined in the interface even if they aren't used.- startSpan(): Span { + startSpan(name: string, options?: SpanOptions): Span { return {} as Span; }packages/core/src/v3/streams/asyncIterableStream.ts (1)
3-20:⚠️ Potential issueFix type mismatch in the async iterator's return value.
Currently, the
next()method is declared to returnPromise<IteratorResult<string>>instead ofPromise<IteratorResult<T>>. This breaks strict type checking and may cause runtime confusion.Apply this fix:
- async next(): Promise<IteratorResult<string>> { + async next(): Promise<IteratorResult<T>> {docs/config/extensions/pythonExtension.mdx (1)
134-153:⚠️ Potential issueFix variable name mismatch in streaming example.
The code example has a variable name mismatch: the async iterable is stored in
resulton line 145, but the loop usesstreamingResulton line 148.// You don't need to await the result const result = python.stream.runScript("./python/my_script.py", ["hello", "world"]); // result is an async iterable/readable stream - for await (const chunk of streamingResult) { + for await (const chunk of result) { console.log(chunk); }🧰 Tools
🪛 LanguageTool
[style] ~134-~134: Consider removing “of” to be more concise
Context: ...e Python scripts. ## Streaming output All of thepythonfunctions have a streaming ver...(ALL_OF_THE)
packages/python/src/index.ts (1)
239-275: 🛠️ Refactor suggestionClean up temp files post-stream usage.
While
createTempFileSyncis convenient, the temp file remains after the process completes. If a large number of inline scripts are called, this can lead to clutter. Consider adding logic to remove the temporary file once the streaming is finalized.Consider using the
withTempFileutility as you do in the non-streamingrunInlinemethod:runInline(scriptContent: string, options: PythonExecOptions = {}): AsyncIterableStream<string> { assert(scriptContent, "Script content is required"); const pythonBin = process.env.PYTHON_BIN_PATH || "python"; - const pythonScriptPath = createTempFileSync(`script_${Date.now()}.py`, scriptContent); - - const pythonProcess = x(pythonBin, [pythonScriptPath], { - ...options, - nodeOptions: { - ...(options.nodeOptions || {}), - env: { - ...process.env, - ...options.env, - }, - }, - throwOnError: false, - }); - - const span = logger.startSpan("python.stream.runInline()", { - attributes: { - pythonBin, - contentPreview: - scriptContent.substring(0, 100) + (scriptContent.length > 100 ? "..." : ""), - [SemanticInternalAttributes.STYLE_ICON]: "brand-python", - }, - }); - - return createAsyncIterableStreamFromAsyncIterable(pythonProcess, { - transform: (chunk, controller) => { - controller.enqueue(chunk); - }, - flush: () => { - span.end(); - }, - }); + + // Create a controller and stream for managing the AsyncIterableStream + const { stream, controller } = createAsyncIterableStream<string>(); + + // Start the span + const span = logger.startSpan("python.stream.runInline()", { + attributes: { + pythonBin, + contentPreview: + scriptContent.substring(0, 100) + (scriptContent.length > 100 ? "..." : ""), + [SemanticInternalAttributes.STYLE_ICON]: "brand-python", + }, + }); + + // Use withTempFile to handle cleanup + withTempFile( + `script_${Date.now()}.py`, + async (tempFilePath) => { + try { + const pythonProcess = x(pythonBin, [tempFilePath], { + ...options, + nodeOptions: { + ...(options.nodeOptions || {}), + env: { + ...process.env, + ...options.env, + }, + }, + throwOnError: false, + }); + + // Forward chunks from process to our stream + for await (const chunk of pythonProcess) { + controller.enqueue(chunk); + } + + controller.close(); + } catch (error) { + controller.error(error); + } finally { + span.end(); + } + }, + scriptContent + ).catch(error => { + controller.error(error); + span.end(); + }); + + return stream; }This approach ensures the temporary file is cleaned up and properly handles stream lifecycle.
🧹 Nitpick comments (10)
packages/core/src/v3/streams/asyncIterableStream.ts (1)
7-7: Consider using a more type-safe approach thananycast.The use of
anybypasses TypeScript's type checking, which could lead to runtime errors. Consider using a more explicit type assertion.- const transformedStream: any = source.pipeThrough(new TransformStream(transformer)); + const transformedStream = source.pipeThrough(new TransformStream(transformer)) as unknown as ReadableStream<T>;docs/config/extensions/additionalPackages.mdx (1)
22-34: Improve sentence structure for better readability.The explanation paragraph could be improved for clarity and grammar.
- This allows you to include additional packages in the build that are not automatically included via imports. This is useful if you want to install a package that includes a CLI tool that you want to invoke in your tasks via `exec`. We will try to automatically resolve the version of the package but you can specify the version by using the `@` symbol: + This allows you to include additional packages in the build that are not automatically included via imports. This is useful if you want to install a package that includes a CLI tool for invoking in your tasks via `exec`. We will try to automatically resolve the version of the package, but you can specify the version by using the `@` symbol:🧰 Tools
🪛 LanguageTool
[style] ~22-~22: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ckage that includes a CLI tool that you want to invoke in your tasks viaexec. We wil...(REP_WANT_TO_VB)
[uncategorized] ~22-~22: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...cally resolve the version of the package but you can specify the version by using th...(COMMA_COMPOUND_SENTENCE)
docs/config/config-file.mdx (1)
318-318: Improve documentation style for reference links.Multiple consecutive sections using identical sentence structure ("See the [...] documentation") creates a repetitive pattern. Consider varying the sentence structure for better readability.
You could vary the formatting like this for a few sections:
- See the [additionalFiles documentation](/config/extensions/additionalFiles) for more information. + For details about including additional files in your build, check the [additionalFiles documentation](/config/extensions/additionalFiles). - See the [additionalPackages documentation](/config/extensions/additionalPackages) for more information. + Need to include external packages? Refer to the [additionalPackages documentation](/config/extensions/additionalPackages).Also applies to: 322-322, 326-326, 330-330, 334-334, 338-338, 342-342, 346-346, 350-350
docs/troubleshooting.mdx (1)
101-102: Typo Correction in "Parallel waits" Section.
Consider changing “more that one” to “more than one” to improve clarity and professionalism in the documentation.🧰 Tools
🪛 LanguageTool
[misspelling] ~101-~101: Did you mean “than”?
Context: ...current version, you can't perform more that one "wait" in parallel. Waits include:...(THAT_THAN)
docs/config/extensions/custom.mdx (1)
380-381: Eliminate Duplicate Word in Source Code Reference.
Please remove the extra “in” so that the text reads “…find them in [the source code here]” which improves clarity.🧰 Tools
🪛 LanguageTool
[grammar] ~380-~380: A hyphen is missing in the adjective “built-in”.
Context: ...``` You should also take a look at our built in extensions for inspiration on how to cr...(BUILT_IN_HYPHEN)
[duplication] ~380-~380: Possible typo: you repeated a word.
Context: ...w to create your own. You can find them in in [the source code here](https://github.c...(ENGLISH_WORD_REPEAT_RULE)
docs/config/extensions/prismaExtension.mdx (1)
9-11: Terminology Improvement Suggestion.
To maintain clarity, consider using “deployment process” instead of “deploy process” (e.g. “Generates the Prisma client during the deployment process”) since “deploy” is a verb.🧰 Tools
🪛 LanguageTool
[grammar] ~10-~10: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ... Generates the Prisma client during the deploy process - Optionally will migrate the d...(PREPOSITION_VERB)
[grammar] ~11-~11: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ...ly will migrate the database during the deploy process - Support for TypedSQL and mult...(PREPOSITION_VERB)
docs/config/extensions/pythonExtension.mdx (2)
44-64: Consider using "deployment process" instead of "deploy process".For better grammar, consider changing "deploy process" to "deployment process" since "deploy" is a verb and "deployment" is the noun form.
-You can automatically add python scripts to your project using the `scripts` option in the `pythonExtension` function. This will copy the specified scripts to the build directory during the deploy process. For example: +You can automatically add python scripts to your project using the `scripts` option in the `pythonExtension` function. This will copy the specified scripts to the build directory during the deployment process. For example:And in line 64:
-This will copy all Python files in the `python` directory to the build directory during the deploy process. You can then execute these scripts using the `python.runScript` function: +This will copy all Python files in the `python` directory to the build directory during the deployment process. You can then execute these scripts using the `python.runScript` function:🧰 Tools
🪛 LanguageTool
[grammar] ~46-~46: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ...ripts to the build directory during the deploy process. For example: ```ts import { d...(PREPOSITION_VERB)
[grammar] ~64-~64: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ...ctory to the build directory during the deploy process. You can then execute these scr...(PREPOSITION_VERB)
134-134: Consider removing "of" to be more concise.For more concise writing, consider revising the phrase.
-All of the `python` functions have a streaming version that allows you to stream the output of the Python script as it runs. For example: +All `python` functions have a streaming version that allows you to stream the output of the Python script as it runs. For example:🧰 Tools
🪛 LanguageTool
[style] ~134-~134: Consider removing “of” to be more concise
Context: ...e Python scripts. ## Streaming output All of thepythonfunctions have a streaming ver...(ALL_OF_THE)
docs/config/extensions/syncEnvVars.mdx (2)
29-34: Fix formatting in the context properties list.The bullet points in the list of context properties have formatting issues. There should be no spaces between the dash and the backtick.
-The callback is passed a context object with the following properties: - -- `environment`: The environment name that the task is being deployed to (e.g. `production`, `staging`, etc.) -- `projectRef`: The project ref of the Trigger.dev project -- `env`: The environment variables that are currently set in the Trigger.dev project +The callback is passed a context object with the following properties: + +- `environment`: The environment name that the task is being deployed to (e.g. `production`, `staging`, etc.) +- `projectRef`: The project ref of the Trigger.dev project +- `env`: The environment variables that are currently set in the Trigger.dev project🧰 Tools
🪛 LanguageTool
[uncategorized] ~31-~31: Loose punctuation mark.
Context: ...e following properties: -environment: The environment name that the task is b...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~32-~32: Loose punctuation mark.
Context: ...uction,staging, etc.) -projectRef`: The project ref of the Trigger.dev proj...(UNLIKELY_OPENING_PUNCTUATION)
35-38: Add a comma in the introduction sentence.For better grammar, add a comma after "In this example".
-In this example we're using env vars from [Infisical](https://infisical.com). +In this example, we're using env vars from [Infisical](https://infisical.com).🧰 Tools
🪛 LanguageTool
[typographical] ~37-~37: It appears that a comma is missing.
Context: ...: Sync env vars from Infisical In this example we're using env vars from [Infisical](h...(DURING_THAT_TIME_COMMA)
📜 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 (51)
docs/config/config-file.mdx(1 hunks)docs/config/extensions/additionalFiles.mdx(1 hunks)docs/config/extensions/additionalPackages.mdx(1 hunks)docs/config/extensions/aptGet.mdx(1 hunks)docs/config/extensions/audioWaveform.mdx(1 hunks)docs/config/extensions/custom.mdx(1 hunks)docs/config/extensions/emitDecoratorMetadata.mdx(1 hunks)docs/config/extensions/esbuildPlugin.mdx(1 hunks)docs/config/extensions/ffmpeg.mdx(1 hunks)docs/config/extensions/overview.mdx(2 hunks)docs/config/extensions/prismaExtension.mdx(1 hunks)docs/config/extensions/puppeteer.mdx(1 hunks)docs/config/extensions/pythonExtension.mdx(1 hunks)docs/config/extensions/syncEnvVars.mdx(1 hunks)docs/docs.json(8 hunks)docs/guides/examples/ffmpeg-video-processing.mdx(1 hunks)docs/guides/examples/libreoffice-pdf-conversion.mdx(1 hunks)docs/guides/examples/pdf-to-image.mdx(1 hunks)docs/guides/examples/puppeteer.mdx(2 hunks)docs/guides/examples/scrape-hacker-news.mdx(2 hunks)docs/guides/examples/sentry-error-tracking.mdx(1 hunks)docs/guides/examples/vercel-sync-env-vars.mdx(1 hunks)docs/guides/frameworks/prisma.mdx(2 hunks)docs/guides/frameworks/supabase-edge-functions-database-webhooks.mdx(1 hunks)docs/troubleshooting.mdx(1 hunks)packages/build/package.json(3 hunks)packages/build/src/extensions/core/additionalFiles.ts(2 hunks)packages/build/src/internal.ts(1 hunks)packages/build/src/internal/additionalFiles.ts(1 hunks)packages/core/src/v3/apiClient/index.ts(1 hunks)packages/core/src/v3/apiClient/runStream.ts(1 hunks)packages/core/src/v3/apiClient/stream.ts(1 hunks)packages/core/src/v3/index.ts(1 hunks)packages/core/src/v3/logger/index.ts(2 hunks)packages/core/src/v3/logger/taskLogger.ts(3 hunks)packages/core/src/v3/runMetadata/index.ts(1 hunks)packages/core/src/v3/runMetadata/manager.ts(1 hunks)packages/core/src/v3/runMetadata/noopManager.ts(1 hunks)packages/core/src/v3/runMetadata/types.ts(1 hunks)packages/core/src/v3/streams/asyncIterableStream.ts(1 hunks)packages/python/README.md(4 hunks)packages/python/package.json(1 hunks)packages/python/src/extension.ts(4 hunks)packages/python/src/index.ts(1 hunks)packages/python/src/utils/tempFiles.ts(1 hunks)references/python-catalog/package.json(1 hunks)references/python-catalog/requirements.txt(1 hunks)references/python-catalog/src/python/html2text_url.py(1 hunks)references/python-catalog/src/trigger/pythonTasks.ts(1 hunks)references/python-catalog/trigger.config.ts(1 hunks)references/python-catalog/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (35)
- docs/guides/frameworks/supabase-edge-functions-database-webhooks.mdx
- docs/guides/examples/pdf-to-image.mdx
- docs/guides/examples/ffmpeg-video-processing.mdx
- docs/guides/examples/libreoffice-pdf-conversion.mdx
- packages/build/src/internal.ts
- docs/guides/examples/vercel-sync-env-vars.mdx
- docs/config/extensions/audioWaveform.mdx
- docs/guides/examples/scrape-hacker-news.mdx
- docs/guides/examples/sentry-error-tracking.mdx
- docs/guides/examples/puppeteer.mdx
- packages/core/src/v3/apiClient/index.ts
- docs/config/extensions/puppeteer.mdx
- packages/core/src/v3/runMetadata/manager.ts
- packages/core/src/v3/runMetadata/noopManager.ts
- docs/config/extensions/esbuildPlugin.mdx
- packages/core/src/v3/runMetadata/index.ts
- packages/core/src/v3/index.ts
- references/python-catalog/requirements.txt
- references/python-catalog/src/python/html2text_url.py
- references/python-catalog/tsconfig.json
- packages/build/src/internal/additionalFiles.ts
- references/python-catalog/src/trigger/pythonTasks.ts
- packages/core/src/v3/apiClient/runStream.ts
- docs/config/extensions/aptGet.mdx
- packages/core/src/v3/apiClient/stream.ts
- docs/guides/frameworks/prisma.mdx
- packages/build/package.json
- references/python-catalog/package.json
- docs/config/extensions/additionalFiles.mdx
- references/python-catalog/trigger.config.ts
- docs/docs.json
- packages/python/src/utils/tempFiles.ts
- docs/config/extensions/emitDecoratorMetadata.mdx
- packages/python/README.md
- packages/core/src/v3/runMetadata/types.ts
🧰 Additional context used
🪛 LanguageTool
docs/config/config-file.mdx
[style] ~325-~325: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...rmation. #### emitDecoratorMetadata See the [emitDecoratorMetadata documentatio...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/config/extensions/pythonExtension.mdx
[grammar] ~46-~46: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ...ripts to the build directory during the deploy process. For example: ```ts import { d...
(PREPOSITION_VERB)
[grammar] ~64-~64: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ...ctory to the build directory during the deploy process. You can then execute these scr...
(PREPOSITION_VERB)
[style] ~134-~134: Consider removing “of” to be more concise
Context: ...e Python scripts. ## Streaming output All of the python functions have a streaming ver...
(ALL_OF_THE)
docs/config/extensions/prismaExtension.mdx
[grammar] ~10-~10: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ... Generates the Prisma client during the deploy process - Optionally will migrate the d...
(PREPOSITION_VERB)
[grammar] ~11-~11: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ...ly will migrate the database during the deploy process - Support for TypedSQL and mult...
(PREPOSITION_VERB)
docs/config/extensions/custom.mdx
[style] ~196-~196: To form a complete sentence, be sure to include a subject.
Context: ...how to use addLayer. ## BuildTarget Can either be dev or deploy, matching t...
(MISSING_IT_THERE)
[uncategorized] ~302-~302: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...e run after packages have been installed and the code copied into the container in t...
(COMMA_COMPOUND_SENTENCE_2)
[style] ~302-~302: Consider a more expressive alternative.
Context: ...n't be available in the final stage. To do that, please use the pkgs property of...
(DO_ACHIEVE)
[grammar] ~380-~380: A hyphen is missing in the adjective “built-in”.
Context: ...``` You should also take a look at our built in extensions for inspiration on how to cr...
(BUILT_IN_HYPHEN)
[duplication] ~380-~380: Possible typo: you repeated a word.
Context: ...w to create your own. You can find them in in [the source code here](https://github.c...
(ENGLISH_WORD_REPEAT_RULE)
docs/config/extensions/syncEnvVars.mdx
[uncategorized] ~31-~31: Loose punctuation mark.
Context: ...e following properties: - environment: The environment name that the task is b...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~32-~32: Loose punctuation mark.
Context: ...uction, staging, etc.) - projectRef`: The project ref of the Trigger.dev proj...
(UNLIKELY_OPENING_PUNCTUATION)
[typographical] ~37-~37: It appears that a comma is missing.
Context: ...: Sync env vars from Infisical In this example we're using env vars from [Infisical](h...
(DURING_THAT_TIME_COMMA)
docs/config/extensions/additionalPackages.mdx
[style] ~22-~22: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ckage that includes a CLI tool that you want to invoke in your tasks via exec. We wil...
(REP_WANT_TO_VB)
[uncategorized] ~22-~22: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...cally resolve the version of the package but you can specify the version by using th...
(COMMA_COMPOUND_SENTENCE)
docs/troubleshooting.mdx
[misspelling] ~101-~101: Did you mean “than”?
Context: ...current version, you can't perform more that one "wait" in parallel. Waits include:...
(THAT_THAN)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (49)
packages/core/src/v3/logger/taskLogger.ts (2)
27-27: Interface enhancement with startSpan method is well-designed.Good addition of the
startSpanmethod to theTaskLoggerinterface. This enhances the logging capabilities by providing a more flexible way to create spans for tracing purposes.
94-96: Well-implemented method delegation in OtelTaskLogger.The implementation correctly delegates to the underlying tracer's
startSpanmethod, passing through both the name and options parameters. Clean and proper implementation that maintains the contract defined in the interface.packages/core/src/v3/logger/index.ts (3)
3-3: Import correctly updated to include SpanOptions.The import statement has been appropriately updated to include the
SpanOptionstype from@opentelemetry/api, which is necessary for the new method signatures.
50-52: Trace method signature properly enhanced with options parameter.The
tracemethod has been correctly updated to include an optionaloptionsparameter of typeSpanOptions. This provides more flexibility when creating spans for tracing operations and properly passes this parameter to the underlying task logger.
54-56: Well-implemented startSpan method delegation.The new
startSpanmethod correctly delegates to the task logger's implementation, maintaining a consistent API pattern throughout the logging system. This addition complements the existing trace method and provides more flexibility for manual span creation.packages/core/src/v3/streams/asyncIterableStream.ts (3)
1-1: Well-structured type definitionThe combined
AsyncIterableStream<T>type offers a clean way to merge functionality of bothAsyncIterable<T>andReadableStream<T>interfaces.
22-50: LGTM - Good implementation of the ReadableStream with abort handling.The
createAsyncIterableReadablefunction correctly handles the abort signal and properly sets up the reading process from the transformed stream.
52-97: Robust implementation with proper error handling.The
createAsyncIterableStreamFromAsyncIterablefunction has excellent error handling and properly handles abort signals. The special case for killable processes is a nice touch for process management.docs/config/extensions/additionalPackages.mdx (2)
1-21: Clear and concise documentation with good examples.The documentation effectively explains the purpose and usage of the
additionalPackagesextension with a helpful code example.
36-36: Clear explanation of behavior in different modes.Good clarification about the extension's behavior in both
devanddeploymodes.docs/config/extensions/ffmpeg.mdx (2)
1-35: Well-structured documentation with clear examples.The documentation effectively explains how to use the
ffmpegextension, including how to specify a particular version if needed.
37-41: Helpful explanation of environment variables and dependencies.Good explanation of environment variables being set automatically and the requirement to add
fluent-ffmpegto the external configuration.docs/config/config-file.mdx (1)
346-346:✅ Verification successful
Verify the esbuild plugins documentation link.
The link to the esbuild plugins documentation has been updated from
/config/extensions/esbuild-pluginsto/config/extensions/esbuildPlugin. Please verify that this new path exists to avoid broken links.
🏁 Script executed:
#!/bin/bash # Check if the esbuildPlugin documentation file exists if [ -f "docs/config/extensions/esbuildPlugin.mdx" ]; then echo "✅ esbuildPlugin.mdx file exists" else echo "❌ esbuildPlugin.mdx file NOT found - this will cause a broken link" # See if there are any similar files that might be the correct target echo "Similar files found:" ls -la docs/config/extensions/ | grep -i esbuild fiLength of output: 126
Confirmed documentation link works.
After verifying, the filedocs/config/extensions/esbuildPlugin.mdxexists, so the updated link/config/extensions/esbuildPluginresolves correctly. No further action is needed.packages/python/package.json (2)
58-60: Ensure Correct Dependency Reclassification.
The update moves"@trigger.dev/build"and"@trigger.dev/sdk"into thedevDependenciessection with the appropriate workspace version. This reclassification aligns with the intended usage of these packages as development-only dependencies.
62-64: Addition of Peer Dependencies.
The new peer dependencies for"@trigger.dev/sdk"and"@trigger.dev/build"(using version ranges likeworkspace:^3.3.16) correctly signal that consumers of this package must also provide these packages.docs/troubleshooting.mdx (1)
97-97: Corrected Prisma URL in Error Guidance.
The link now directs users to/config/extensions/prismaExtensionas per the updated documentation structure for Prisma.docs/config/extensions/overview.mdx (3)
3-9: Streamlined Overview Content.
The updated introductory section—with clear front matter, title, and sidebar title—provides a concise overview of build extensions and sets the right context for users.
31-33: Clear Guidance on Pre-built Extension Imports.
The instructions for importing pre-built extensions (e.g. using@trigger.dev/build) are straightforward and align with the new package structure.
45-60: Verify Built-in Extension Links.
Ensure that all links in the built-in extensions table (such as[prismaExtension],[pythonExtension], and[esbuildPlugin]) point to the correct and updated documentation pages.docs/config/extensions/custom.mdx (2)
1-7: Clear Introduction and Metadata.
The front matter and opening section effectively introduce custom build extensions. The bullet list clearly enumerates the capabilities, making it easy for users to grasp what can be achieved.
40-46: Effective Example for Extracting Extensions.
The example demonstrating how to extract a build extension into a separate function (with a note about adding@trigger.dev/buildto your devDependencies) is well presented and informative.docs/config/extensions/prismaExtension.mdx (6)
1-7: Comprehensive Introduction for Prisma Extension.
The front matter and initial bullet list clearly communicate the features and benefits of using theprismaExtension. The descriptions are concise and to the point.
18-34: Well-Structured Basic Prisma Setup Example.
The code snippet demonstrating a simple Prisma setup is clear and shows how to integrate the extension with your configuration effectively.
41-62: Clear Migrations Configuration Example.
The migration example succinctly demonstrates how to use themigrateoption to run database migrations during the build process.
64-93: Detailed Client Generator Example.
The example covering theclientGeneratoroption (including handling multiple generators in the Prisma schema) is comprehensive and provides valuable guidance for users needing to restrict client generation.
112-133: Clear Guidance on Enabling TypedSQL.
The TypedSQL section includes a straightforward example and a helpful note regarding environment variable injection, making it easy for users to enable and understand this feature.
142-157: Useful Example for Multiple Schemas.
The provided example for handling multiple Prisma schemas in a single project is practical and enhances the documentation by addressing more complex scenarios.docs/config/extensions/pythonExtension.mdx (5)
1-5: Good title and description structure for documentation.The title, sidebar title, and description are clearly defined and provide a good overview of the extension's purpose.
7-27: Clear installation and configuration instructions.The introduction and basic configuration example are well-structured and easy to follow. The code snippets are properly formatted with appropriate syntax highlighting.
86-109: Clear explanation of requirements file usage.The documentation provides a clear explanation of how to use requirements files with good code examples and helpful notes about the difference between production and development modes.
111-132: Good explanation of virtual environments configuration.The documentation for virtual environments clearly explains the purpose and usage of the
devPythonBinaryPathoption, which aligns with the changes in the extension implementation.
155-182: Good explanation of environment variables handling.The documentation provides clear examples of both accessing environment variables in Python scripts and passing additional variables from TypeScript. The examples are practical and well-structured.
packages/python/src/extension.ts (6)
3-3: Good migration to using the centralized addAdditionalFilesToBuild function.The import of
addAdditionalFilesToBuildfrom the build package's internal module shows a move toward a more centralized approach for file management during the build process.
19-19: Improved property naming for clarity.Renaming
pythonBinaryPathtodevPythonBinaryPathis a good change as it clearly indicates that this option is only relevant for development environments.
59-67: Improved file handling with centralized function.Using the
addAdditionalFilesToBuildfunction with explicit identifier and parameters is a better approach than the previous implementation. This change enhances maintainability and consistency across extensions.
69-72: Updated reference to renamed property.The code correctly uses the renamed
devPythonBinaryPathproperty, ensuring consistency with the type definition change.
101-133: Enhanced requirements file handling with better structure.The implementation now copies the requirements file to the container and installs dependencies directly from it, which is more structured than the previous approach. It also includes proper warning messages when both requirements options are specified.
133-154: Clear conditional handling for different requirements scenarios.The code handles the different scenarios (requirements file vs. inline requirements) clearly with separate conditional blocks, making the logic easy to follow.
docs/config/extensions/syncEnvVars.mdx (4)
1-10: Clear introduction to the syncEnvVars extension.The documentation provides a good overview of the purpose and usage of the extension, explaining how it helps sync environment variables from external services.
11-27: Comprehensive basic usage example.The example clearly demonstrates how to use the extension with a simple callback function that returns environment variables to be synced.
39-68: Well-structured Infisical integration example.The example for integrating with Infisical is comprehensive and includes all necessary steps: authentication, fetching secrets, and transforming them to the required format.
70-116: Good documentation for the syncVercelEnvVars extension.The documentation for the Vercel integration is clear and includes both the default usage (with environment variables) and parameterized usage. The note about required environment variables is helpful.
packages/build/src/extensions/core/additionalFiles.ts (1)
1-15:❓ Verification inconclusive
Effective refactoring to use centralized file handling.
The refactoring to use the centralized
addAdditionalFilesToBuildfunction is a good improvement that reduces code duplication and enhances maintainability. The extension now delegates the file handling logic to a specialized function while maintaining its interface.It would be helpful to ensure that comprehensive tests are in place to verify that this refactoring doesn't change the existing functionality.
🏁 Script executed:
#!/bin/bash # Check if there are tests for the addAdditionalFilesToBuild function fd "additionalFiles" --glob "*.test.ts" --glob "*.spec.ts"Length of output: 211
Refactoring verification requires test coverage confirmation.
The initial search command produced errors due to invalid glob patterns. Please re-run the revised command above to verify that comprehensive tests exist for the centralized addAdditionalFilesToBuild functionality. This will help ensure that the refactoring does not inadvertently change existing behavior.
🏁 Script executed:
#!/bin/bash # Search for test files that reference the addAdditionalFilesToBuild function rg "addAdditionalFilesToBuild" -g "*spec.ts" -g "*test.ts" .Length of output: 182
Centralized file handling is well-implemented, but test coverage verification is needed.
The refactoring cleanly delegates file handling to the centralized
addAdditionalFilesToBuildfunction, reducing duplication and improving maintainability. However, our automated search did not turn up any test files explicitly referencing this function. Please confirm manually or add comprehensive tests to ensure that this refactoring doesn’t alter the existing functionality.
- File under review:
packages/build/src/extensions/core/additionalFiles.ts(Lines 1-15)packages/python/src/index.ts (6)
1-10: Good imports organization with clear dependencies.The imports are well-organized with proper separation between external packages and internal utilities. The explicit import of
AsyncIterableStreamand related types from "@trigger.dev/core/v3" shows good modularization of the streaming functionality.
16-57: Well-implemented Python execution with robust error handling and tracing.The Python execution is well-implemented with:
- Proper environment variable handling
- Comprehensive tracing with
logger.trace- Good error detection and messaging for non-zero exit codes
- Proper attribute labeling for telemetry
59-111: Strong script validation with informative error messaging.The script execution correctly:
- Validates script existence before attempting execution
- Logs the script path as an attribute for tracing
- Provides detailed error messages including both the command executed and the error output
- Properly handles environment variables
113-164: Improved inline script execution with better temporary file handling.The use of
withTempFileutility is a significant improvement over direct file operations, as it ensures proper file cleanup after execution. The content preview attribute provides good context for debugging without exposing the entire script content.
166-198: Good implementation of streaming Python execution.The streaming version of
runcorrectly:
- Creates a Python process with proper environment variables
- Sets up appropriate span tracking
- Creates an AsyncIterableStream with proper transformation and cleanup
199-238: Well-structured streaming script execution with validation.The implementation includes proper script validation, appropriate span tracking with attributes, and clean stream creation. The span is properly ended in the flush callback, ensuring telemetry resources are released.
|
|
||
| const span = logger.startSpan("python.stream.run()", { | ||
| attributes: { | ||
| pythonBin, | ||
| args: scriptArgs.join(" "), | ||
| [SemanticInternalAttributes.STYLE_ICON]: "brand-python", | ||
| }, | ||
| }); | ||
|
|
||
| return createAsyncIterableStreamFromAsyncIterable(pythonProcess, { | ||
| transform: (chunk, controller) => { | ||
| controller.enqueue(chunk); | ||
| }, | ||
| flush: () => { | ||
| span.end(); | ||
| }, | ||
| }); | ||
| }, | ||
| runScript( | ||
| scriptPath: string, | ||
| scriptArgs: string[] = [], | ||
| options: PythonExecOptions = {} | ||
| ): AsyncIterableStream<string> { | ||
| assert(scriptPath, "Script path is required"); | ||
| assert(fs.existsSync(scriptPath), `Script does not exist: ${scriptPath}`); | ||
|
|
||
| const pythonBin = process.env.PYTHON_BIN_PATH || "python"; | ||
|
|
||
| const pythonProcess = x(pythonBin, [scriptPath, ...scriptArgs], { | ||
| ...options, | ||
| nodeOptions: { | ||
| ...(options.nodeOptions || {}), | ||
| env: { | ||
| ...process.env, | ||
| ...options.env, | ||
| }, | ||
| }, | ||
| throwOnError: false, | ||
| }); | ||
|
|
||
| const span = logger.startSpan("python.stream.runScript()", { | ||
| attributes: { | ||
| pythonBin, | ||
| scriptPath, | ||
| args: scriptArgs.join(" "), | ||
| [SemanticInternalAttributes.STYLE_ICON]: "brand-python", | ||
| }, | ||
| }); | ||
|
|
||
| return createAsyncIterableStreamFromAsyncIterable(pythonProcess, { | ||
| transform: (chunk, controller) => { | ||
| controller.enqueue(chunk); | ||
| }, | ||
| flush: () => { | ||
| span.end(); | ||
| }, | ||
| }); | ||
| }, | ||
| runInline(scriptContent: string, options: PythonExecOptions = {}): AsyncIterableStream<string> { | ||
| assert(scriptContent, "Script content is required"); | ||
|
|
||
| const pythonBin = process.env.PYTHON_BIN_PATH || "python"; | ||
|
|
||
| const pythonScriptPath = createTempFileSync(`script_${Date.now()}.py`, scriptContent); | ||
|
|
||
| const pythonProcess = x(pythonBin, [pythonScriptPath], { | ||
| ...options, | ||
| nodeOptions: { | ||
| ...(options.nodeOptions || {}), | ||
| env: { | ||
| ...process.env, | ||
| ...options.env, | ||
| }, | ||
| }, | ||
| throwOnError: false, | ||
| }); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| export default { run, runScript, runInline }; | ||
| const span = logger.startSpan("python.stream.runInline()", { | ||
| attributes: { | ||
| pythonBin, | ||
| contentPreview: | ||
| scriptContent.substring(0, 100) + (scriptContent.length > 100 ? "..." : ""), | ||
| [SemanticInternalAttributes.STYLE_ICON]: "brand-python", | ||
| }, | ||
| }); | ||
|
|
||
| return createAsyncIterableStreamFromAsyncIterable(pythonProcess, { | ||
| transform: (chunk, controller) => { | ||
| controller.enqueue(chunk); | ||
| }, | ||
| flush: () => { | ||
| span.end(); | ||
| }, | ||
| }); | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for streaming execution.
The streaming methods don't handle Python execution errors, unlike their non-streaming counterparts. If the Python script exits with a non-zero code, this isn't properly reported to the consumer of the stream.
Consider enhancing the streaming methods to check for non-zero exit codes and propagate errors to the stream consumer. Here's a pattern you could apply to each streaming method:
// Example for stream.run
run(scriptArgs: string[] = [], options: PythonExecOptions = {}): AsyncIterableStream<string> {
const pythonBin = process.env.PYTHON_BIN_PATH || "python";
const pythonProcess = x(pythonBin, scriptArgs, {
...options,
nodeOptions: {
...(options.nodeOptions || {}),
env: {
...process.env,
...options.env,
},
},
throwOnError: false,
});
const span = logger.startSpan("python.stream.run()", {
attributes: {
pythonBin,
args: scriptArgs.join(" "),
[SemanticInternalAttributes.STYLE_ICON]: "brand-python",
},
});
+ // Create a controller and stream for managing the AsyncIterableStream
+ const { stream, controller } = createAsyncIterableStream<string>();
+
+ // Process the Python execution asynchronously
+ (async () => {
+ try {
+ for await (const chunk of pythonProcess) {
+ controller.enqueue(chunk);
+ }
+
+ // Check exit code after process completes
+ if (pythonProcess.exitCode !== 0) {
+ span.setAttribute("exitCode", pythonProcess.exitCode);
+ controller.error(
+ new Error(
+ `${scriptArgs.join(" ")} exited with a non-zero code ${pythonProcess.exitCode}:\n${
+ pythonProcess.stderr
+ }`
+ )
+ );
+ } else {
+ controller.close();
+ }
+ } catch (error) {
+ controller.error(error);
+ } finally {
+ span.end();
+ }
+ })();
+
+ return stream;
- return createAsyncIterableStreamFromAsyncIterable(pythonProcess, {
- transform: (chunk, controller) => {
- controller.enqueue(chunk);
- },
- flush: () => {
- span.end();
- },
- });
}This would allow consumers of the stream to properly handle Python execution errors.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/docs.json (1)
69-70: New "context" page addition under Writing tasks.
The addition of the "context" page expands the documentation for Writing tasks. Please verify that the corresponding documentation content exists and is integrated properly in the overall guide.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/docs.json(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
docs/docs.json (3)
42-42: Retention of the "apikeys" entry is appropriate.
The "apikeys" item remains in the "Fundamentals" group as expected. Ensure that its content stays consistent with any future updates to the documentation.
72-100: Reorganized "Configuration" group with a dedicated "Build extensions" subgroup.
The "Configuration" group now nests a "Build extensions" subgroup that includes both an overview and a further nested "Built-in extensions" group listing multiple extension pages (including the newpythonExtension). This new structure improves clarity and categorization. Please confirm that all referenced pages (such as "config/extensions/pythonExtension") are available and cross-linked correctly in the documentation.
167-175: Addition of the "Using the Dashboard" group.
The new group "Using the Dashboard" has been introduced with pages related to dashboard functionalities such as "run-tests", "troubleshooting-alerts", "replaying", and "bulk-actions". This change effectively differentiates dashboard-specific documentation from other sections. Verify that the migrated "troubleshooting-alerts" page and the new pages are correctly documented.
There's a new preview package available here:
@trigger.dev/python@0.0.0-prerelease-20250226191317Summary by CodeRabbit
New Features
syncEnvVarsandsyncVercelEnvVarsextensions for environment variable management.pythonExtensionbuild extension detailing integration and usage.Documentation
Refactor