Add support for specifying machine preset at trigger time #1608
Conversation
🦋 Changeset detectedLatest commit: 952d011 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
apps/webapp/app/v3/services/triggerTask.server.tsOops! Something went wrong! :( ESLint: 8.45.0 ESLint couldn't find the config "custom" to extend from. Please check that the name of the config is correct. The config "custom" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThe pull request introduces support for specifying machine presets when triggering tasks in the Trigger SDK. This enhancement allows users to override the default machine preset for individual tasks or batch triggers. The changes span multiple files across the SDK and webapp, adding a new Changes
Sequence DiagramsequenceDiagram
participant User
participant SDK
participant WebApp
participant TaskRunner
User->>SDK: Trigger task with machinePreset
SDK->>WebApp: Send task trigger request
WebApp->>WebApp: Validate machinePreset
alt Preset is valid
WebApp->>TaskRunner: Assign specified machine preset
else Preset is invalid
WebApp->>TaskRunner: Use default machine preset
end
TaskRunner->>User: Execute task
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (3)
👮 Files not reviewed due to content moderation or server errors (3)
⏰ Context from checks skipped due to timeout of 90000ms (7)
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/webapp/app/v3/machinePresets.server.ts (1)
34-42: Enhance error handling in machinePresetFromRun.The function silently returns null when parsing fails. Consider logging the error for better debugging.
export function machinePresetFromRun(run: { machinePreset: string | null }): MachinePreset | null { - const presetName = MachinePresetName.safeParse(run.machinePreset).data; + const parsed = MachinePresetName.safeParse(run.machinePreset); + if (!parsed.success) { + logger.debug("Failed to parse machine preset", { + preset: run.machinePreset, + error: parsed.error + }); + return null; + } - if (!presetName) { - return null; - } - - return machinePresetFromName(presetName); + return machinePresetFromName(parsed.data); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.changeset/gold-melons-fetch.md(1 hunks)apps/webapp/app/v3/machinePresets.server.ts(1 hunks)apps/webapp/app/v3/marqs/sharedQueueConsumer.server.ts(7 hunks)apps/webapp/app/v3/services/createTaskRunAttempt.server.ts(2 hunks)apps/webapp/app/v3/services/restoreCheckpoint.server.ts(3 hunks)apps/webapp/app/v3/services/triggerTask.server.ts(1 hunks)packages/core/src/v3/schemas/api.ts(3 hunks)packages/core/src/v3/types/tasks.ts(2 hunks)packages/trigger-sdk/src/v3/shared.ts(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (buildjet-8vcpu-ubuntu-2204 - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (buildjet-8vcpu-ubuntu-2204 - npm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
apps/webapp/app/v3/services/restoreCheckpoint.server.ts (1)
73-75: LGTM! Clean implementation of machine preset resolution.The code correctly prioritizes the run-level machine preset while maintaining backward compatibility with config-based presets.
packages/core/src/v3/schemas/api.ts (2)
95-95: LGTM! Clean schema extension.The
machinePresetoption is correctly added as an optional field with proper type safety.
136-136: LGTM! Consistent schema implementation.The same
machinePresetoption is correctly added to batch triggers, maintaining consistency across the API.packages/core/src/v3/types/tasks.ts (1)
779-783: LGTM! Well-documented type extension.The
machinePresetoption is properly typed and documented, clearly explaining its purpose and behavior..changeset/gold-melons-fetch.md (1)
1-34: Well-documented feature with clear examples!The changeset provides comprehensive documentation of the new machine preset feature, including:
- Clear version bumps for affected packages
- Detailed examples showing various usage patterns
- Good coverage of all trigger function variants
The patch version bumps are appropriate as this is a backward-compatible feature addition.
packages/trigger-sdk/src/v3/shared.ts (1)
616-616: Clean and consistent implementation of machine preset support!The implementation:
- Adds machinePreset support consistently across all trigger functions
- Maintains type safety through proper interface updates
- Follows existing patterns for option handling
Also applies to: 790-790, 952-952, 1207-1207, 1267-1267, 1361-1361, 1438-1438
apps/webapp/app/v3/marqs/sharedQueueConsumer.server.ts (1)
416-418: Well-structured machine preset handling with proper fallbacks!The implementation:
- Consistently uses machinePresetFromRun with fallback to machinePresetFromConfig
- Maintains backward compatibility through proper fallback mechanisms
- Integrates smoothly with existing queue consumer logic
Also applies to: 547-549, 1083-1085, 1305-1307
apps/webapp/app/v3/services/createTaskRunAttempt.server.ts (2)
9-9: LGTM! Import changes align with the PR objective.The addition of
machinePresetFromRunimport alongsidemachinePresetFromConfigsupports the new feature for specifying machine presets at trigger time.
176-178: LGTM! Clear precedence in machine preset determination.The implementation correctly prioritizes trigger-time machine preset specification with a proper fallback mechanism.
Let's verify the validation logic in the imported functions:
✅ Verification successful
Validation confirmed: Machine preset determination is robust and safe
The implementation includes proper validation with safe parsing, fallback mechanisms, and error logging:
- Invalid configs fall back to "small-1x" preset
- Invalid run presets return null, triggering the fallback to config-based preset
- All user inputs are safely parsed before use
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check validation logic in machinePresets.server.ts # Search for validation logic in machinePresetFromRun and machinePresetFromConfig ast-grep --pattern 'function machinePresetFrom$_($_) { $$$ }' # Look for any validation-related error handling rg -A 5 "throw.*Error.*machine.*preset" "../machinePresets.server.ts"Length of output: 203
Script:
#!/bin/bash # First, find the machinePresets file fd -t f machinePresets # Once found, we'll examine its contents # Look for both function definitions and any validation logic ast-grep --pattern 'function machinePresetFrom$_($_) { $$$ }' # Search for validation-related terms rg -i "machine.*preset.*valid|invalid.*machine.*preset"Length of output: 192
Script:
#!/bin/bash # Examine the full content of the machine presets file cat apps/webapp/app/v3/machinePresets.server.tsLength of output: 1652
@trigger.dev/react-hooks
@trigger.dev/rsc
trigger.dev
@trigger.dev/sdk
@trigger.dev/build
@trigger.dev/core
commit: |
|
Thank you for this! This will be great for us. Is there a reason you went with I know the other options in machine are deprecated, but it seems like it would be easier to maintain and document if the type was shared? |
🙌
Yep, precisely due to the deprecation many months ago when we added v3 billing. Final name TBD and yet to add docs. There will be some breaking changes to the SDK in the near future. Could be a good time to simplify new options even further, maybe in this case: This could go hand in hand with an upcoming option to select the region or "worker group" (internal name for now - there could be multiple groups in the same region, with different features / prereleases, incl self-hosted or dedicated ones). I think something like this would be quite nice: export const someTask = task({
id: "some-task",
machine: "small-2x",
worker: "us-east-1",
run: async (payload: any) => {
logger.info("Hello world");
},
});
export const someOtherTask = task({
id: "some-other-task",
run: async () => {
logger.info("Hello again");
await someTask.trigger("foo", { machine: "micro", worker: "eu-central-1" });
},
});At the same time, I think there could be some confusion around machine vs worker. There wouldn't be with machine vs region, but it wouldn't necessarily be accurate. What do you think? |
Got it! I think machine and worker could be confusing for some... region is a pretty standard term these days (aws has more than one eu region for example). Another option could be: export const someTask = task({
id: "some-task",
runsOn: {
machine: "small-2x",
inGroups: ["us-east-1", "uk-south-1"]
}
run: async (payload: any) => {
logger.info("Hello world");
},
}); |
|
Yeah, after a quick brainstorm I think we'll go with region 👍 |
What it says on the tin.
To keep the task options in line, they now also accept a string in addition to the old object:
Summary by CodeRabbit
Release Notes
New Features
Improvements
The changes provide developers with greater control over task execution by allowing custom machine preset selection during task triggering.