[refactor] Standardize task configuration as a first class schema - #227
Conversation
- Updated task configuration to replace `name` with `title` across various tasks for consistency. - Modified related components and examples to reflect the new `title` property. - Enhanced input handling in tasks to support dynamic configuration schemas, ensuring backward compatibility. - Improved documentation to clarify the changes in task configuration structure.
There was a problem hiding this comment.
Pull request overview
This PR refactors the task framework to treat task configuration as a first-class, schema-backed concept, standardizing instance metadata (title, description) and separating runtime execution options into a dedicated runConfig.
Changes:
- Replace
config.namewithconfig.titleacross tests/examples/docs and update JSON serialization/deserialization accordingly. - Introduce a base task config JSON schema (
baseConfigSchema) and per-taskconfigSchema()composition for config validation. - Move runtime-only concerns (e.g.,
runnerId,outputCache, runtimecacheable) out of serialized config intoIRunConfig/task.runConfig, and update runners/tasks to use it.
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/test/src/test/task/TaskRunnerStreaming.test.ts | Update streaming cache tests to pass outputCache via runConfig (3rd ctor arg). |
| packages/test/src/test/task/TaskJSON.test.ts | Update JSON serialization tests to expect title instead of name. |
| packages/test/src/test/task/SplitTask.test.ts | Replace name with title in task config assertions. |
| packages/test/src/test/task/SingleTask.test.ts | Replace name with title in configuration test. |
| packages/test/src/test/task/MergeTask.test.ts | Replace name with title in task config assertions. |
| packages/test/src/test/task/DelayTask.test.ts | Update DelayTask to take delay from config rather than input shape. |
| packages/test/src/test/task-graph/StreamingAccumulation.test.ts | Update cacheable streaming task tests to pass cache via runConfig. |
| packages/test/src/test/task-graph-output-cache/StreamingCache.test.ts | Update cache integration tests for new runConfig cache plumbing. |
| packages/tasks/src/task/OutputTask.ts | Switch schema overrides to config.inputSchema / config.outputSchema. |
| packages/tasks/src/task/LambdaTask.ts | Add configSchema() and schema-backed config shape for lambda execution hooks. |
| packages/tasks/src/task/InputTask.ts | Switch schema overrides to config.inputSchema / config.outputSchema. |
| packages/tasks/src/task/DelayTask.ts | Move delay control to config + introduce config schema for DelayTask. |
| packages/tasks/src/task/DebugLogTask.ts | Move log level control to config + introduce config schema for DebugLogTask. |
| packages/tasks/src/task/ArrayTask.ts | Update config destructuring to omit title instead of name when cloning tasks. |
| packages/tasks/README.md | Update docs/examples to pass configuration via config arg (and/or undefined input). |
| packages/task-graph/src/task/WhileTask.ts | Add whileTaskConfigSchema and configSchema() for WhileTask config validation. |
| packages/task-graph/src/task/TaskTypes.ts | Introduce baseConfigSchema; redefine TaskConfig from schema; add title/description. |
| packages/task-graph/src/task/TaskRunner.ts | Resolve caching from IRunConfig / task.runConfig instead of serialized config. |
| packages/task-graph/src/task/TaskJSON.ts | Rename JSON fields to title and add per-instance schema override fields. |
| packages/task-graph/src/task/Task.ts | Add config schema validation, runConfig, new run() signature, and description. |
| packages/task-graph/src/task/ReduceTask.ts | Add reduceTaskConfigSchema + configSchema() for ReduceTask. |
| packages/task-graph/src/task/MapTask.ts | Add mapTaskConfigSchema + configSchema() for MapTask. |
| packages/task-graph/src/task/JobQueueTask.ts | Add jobQueueTaskConfigSchema + configSchema() for JobQueueTask. |
| packages/task-graph/src/task/IteratorTask.ts | Add iteratorTaskConfigSchema + configSchema() for iterator base config validation. |
| packages/task-graph/src/task/ITask.ts | Introduce IRunConfig (runtime-only config) and extend task interface accordingly. |
| packages/task-graph/src/task/GraphAsTask.ts | Add config schema + incorporate runtime cacheable override precedence. |
| packages/task-graph/src/task/ConditionalTask.ts | Add conditionalTaskConfigSchema + configSchema() for ConditionalTask. |
| packages/task-graph/src/task-graph/TaskGraphRunner.ts | Move runnerId propagation to task.runConfig instead of serialized config. |
| packages/task-graph/src/task-graph/Conversions.ts | Strip non-schema config fields (isOwned) before config validation. |
| packages/ai/src/task/base/StreamingAiTask.ts | Use runConfig.runnerId for job run tracking. |
| packages/ai/src/task/base/AiTask.ts | Generate instance label into config.title; use runConfig.runnerId for jobs. |
| examples/web/src/graph/TaskNode.tsx | Update UI to display config.title instead of config.name. |
| examples/web/src/editor/JsonEditor.tsx | Update JsonTask construction to use title instead of name. |
| examples/cli/src/components/TaskUI.tsx | Update CLI UI display to use config.title instead of config.name. |
| examples/cli/src/TaskCLI.ts | Update example JSON/config to use title and new DelayTask input/config split. |
| docs/developers/01_getting_started.md | Update docs JSON examples from input to config for task options. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| async execute(input: Input, executeContext: IExecuteContext): Promise<Output> { | ||
| const delay = input.delay ?? 0; | ||
| const delay = this.config.delay ?? 0; |
There was a problem hiding this comment.
delayTaskConfigSchema declares a default of 1, but runtime behavior falls back to 0 when config.delay is unset. Since config-schema defaults aren't currently being applied in Task.validateAndApplyConfigDefaults, this creates a mismatch between schema/UI and execution. Consider either (a) changing the runtime fallback to 1, or (b) actually applying JSON-schema defaults during config validation.
| const delay = this.config.delay ?? 0; | |
| const delay = this.config.delay ?? 1; |
| @@ -70,8 +86,7 @@ export class DelayTask< | |||
| } else { | |||
| await sleep(delay); | |||
| } | |||
| const output = Object.fromEntries(Object.entries(input).filter(([k]) => k !== "delay")); | |||
| return output as Output; | |||
| return input as unknown as Output; | |||
| } | |||
There was a problem hiding this comment.
DelayTask previously appears to have taken delay from the input payload (and tests/docs historically use new DelayTask({ delay: ... })). After this change, input.delay is ignored and will also be passed through to outputs if provided, which can be a surprising breaking change. If backward compatibility is a goal, consider supporting input.delay as a fallback when config.delay is unset (and optionally stripping it from the passthrough output).
| async executeReactive(input: Input, output: Output) { | ||
| const log_level: LogLevel = this.config.log_level ?? DEFAULT_LOG_LEVEL; | ||
| const inputRecord = input as Record<string, unknown>; | ||
| const log_level: LogLevel = (inputRecord.log_level as LogLevel) ?? DEFAULT_LOG_LEVEL; | ||
| const loggable = Object.fromEntries( | ||
| Object.entries(inputRecord).filter(([k]) => k !== "log_level") | ||
| ); | ||
| if (log_level === "dir") { | ||
| console.dir(loggable, { depth: null }); | ||
| console.dir(inputRecord, { depth: null }); | ||
| } else { | ||
| console[log_level](loggable); | ||
| console[log_level](inputRecord); | ||
| } |
There was a problem hiding this comment.
DebugLogTask now reads log_level only from config. Existing callers that pass log_level in the input payload will silently get the default log level instead. If compatibility with the previous input shape matters, consider falling back to (input as any).log_level when config.log_level is undefined (and decide whether log_level should be excluded from passthrough output in that legacy path).
| private validateAndApplyConfigDefaults(config: Config): Config { | ||
| const ctor = this.constructor as typeof Task; | ||
| const schemaNode = ctor.getConfigSchemaNode(this.type); | ||
| if (!schemaNode) return config; | ||
|
|
||
| const result = schemaNode.validate(config); | ||
| if (!result.valid) { | ||
| const errorMessages = result.errors.map((e) => { | ||
| const path = (e as any).data?.pointer || ""; | ||
| return `${e.message}${path ? ` (${path})` : ""}`; | ||
| }); | ||
| throw new Error(`[${ctor.name}] Configuration Error: ${errorMessages.join(", ")}`); | ||
| } |
There was a problem hiding this comment.
validateAndApplyConfigDefaults throws a generic Error on config-schema validation failures. Elsewhere the codebase uses TaskConfigurationError for configuration problems, and TaskRunner only treats TaskError subclasses specially. Consider throwing TaskConfigurationError here (and optionally including the task type/id in the message) so callers can reliably distinguish misconfiguration from runtime failures.
| const taskConfig: TaskConfig = { | ||
| id: item.id, | ||
| name: item.name, | ||
| ...(item.title ? { title: item.title } : {}), | ||
| ...(item.inputSchema ? { inputSchema: item.inputSchema } : {}), | ||
| ...(item.outputSchema ? { outputSchema: item.outputSchema } : {}), | ||
| extras: item.extras, | ||
| }; |
There was a problem hiding this comment.
createSingleTaskFromJSON no longer accepts the legacy name field (only title). If older persisted graphs still contain name, they will silently lose the instance label. To preserve backward compatibility, consider mapping name → title when title is absent (while still emitting title on serialization).
…initions - Updated task configuration schemas across multiple tasks to utilize TaskConfigSchema instead of the deprecated baseConfigSchema. - Refactored related imports and error handling to align with the new configuration structure. - Ensured consistency in task configuration management, enhancing maintainability and clarity.
|
@copilot the test failure is a flakey test. any ideas on how to make it less so? |
a59f729 to
1005d57
Compare
- Added `clearPipelineCache` calls in multiple test files to ensure a clean state before running tests. - Updated imports in relevant test files to include `clearPipelineCache` from the `hf-transformers` module. - Improved test reliability by preventing potential interference from cached data during execution.
- Updated the `toJSON` method in the `Task` class to encapsulate task properties within a `config` object, enhancing organization and clarity. - Modified the `TaskGraphItemJson` type to reflect the new structure, removing direct properties like `title`, `inputSchema`, and `outputSchema`. - Adjusted the `createSingleTaskFromJSON` function to accommodate the new `config` structure, ensuring backward compatibility. - Updated related tests to validate the new serialization format and configuration handling.
…g object - Refactored task definitions to encapsulate properties like `title` within a `config` object, enhancing clarity and organization. - Adjusted the `JsonTaskItem` and `TaskGraphItemJson` types to reflect the new configuration structure, allowing for additional properties. - Modified the `createSingleTaskFromJSON` function to streamline task creation using the new `config` format. - Updated tests to ensure compatibility with the revised task serialization and configuration handling.
- Updated ConditionalTask to include a new optional `conditionConfig` property in its configuration schema, allowing for more flexible branch conditions. - Refactored WhileTask to remove reliance on `extras` for condition fields, integrating them directly into the task configuration for improved clarity and usability. - Adjusted related types and serialization structures to accommodate the new configuration format. - Updated tests to validate the new configuration handling for both ConditionalTask and WhileTask, ensuring consistent behavior across task executions.
- Updated all packages to version 0.0.101, reflecting the promotion of task configuration to a first-class schema and the removal of the old name property in favor of title. - Updated changelogs for each package to document the changes and updated dependencies.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 72 out of 73 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public toJSON(): TaskGraphItemJson { | ||
| const extras = this.config.extras; | ||
| let json: TaskGraphItemJson = this.stripSymbols({ | ||
| const json: TaskGraphItemJson = this.stripSymbols({ | ||
| id: this.config.id, | ||
| type: this.type, | ||
| ...(this.config.name ? { name: this.config.name } : {}), | ||
| defaults: this.defaults, | ||
| ...(extras && Object.keys(extras).length ? { extras } : {}), | ||
| config: { | ||
| ...(this.config.title ? { title: this.config.title } : {}), | ||
| ...(this.config.inputSchema ? { inputSchema: this.config.inputSchema } : {}), | ||
| ...(this.config.outputSchema ? { outputSchema: this.config.outputSchema } : {}), | ||
| ...(extras && Object.keys(extras).length ? { extras } : {}), | ||
| }, | ||
| }); | ||
| return json as TaskGraphItemJson; | ||
| return json; | ||
| } |
There was a problem hiding this comment.
The base Task.toJSON() method only serializes a subset of config properties (title, inputSchema, outputSchema, extras), but does not serialize custom config properties added by subclasses like WhileTask (maxIterations, chainIterations, conditionField, etc.), DelayTask (delay), DebugLogTask (log_level), etc.
This means tasks with custom config properties will not properly round-trip through JSON serialization/deserialization. When a task is serialized and then deserialized, the custom config properties will be lost.
The toJSON method should serialize the entire config object, or subclasses that add config properties should override toJSON to include their custom properties.
| @@ -38,15 +51,18 @@ export type DelayTaskOutput = FromSchema<typeof outputSchema>; | |||
| export class DelayTask< | |||
| Input extends DelayTaskInput = DelayTaskInput, | |||
| Output extends DelayTaskOutput = DelayTaskOutput, | |||
| Config extends TaskConfig = TaskConfig, | |||
| > extends Task<Input, Output, Config> { | |||
| > extends Task<Input, Output, DelayTaskConfig> { | |||
| static readonly type = "DelayTask"; | |||
| static readonly category = "Utility"; | |||
| public static title = "Delay"; | |||
| public static description = "Delays execution for a specified duration with progress tracking"; | |||
| static readonly cacheable = false; | |||
| public static passthroughInputsToOutputs = true; | |||
|
|
|||
| public static configSchema(): DataPortSchema { | |||
| return delayTaskConfigSchema; | |||
| } | |||
|
|
|||
| static inputSchema() { | |||
| return inputSchema; | |||
| } | |||
| @@ -56,7 +72,7 @@ export class DelayTask< | |||
| } | |||
|
|
|||
| async execute(input: Input, executeContext: IExecuteContext): Promise<Output> { | |||
| const delay = input.delay ?? 0; | |||
| const delay = this.config.delay ?? 1; | |||
| if (delay > 100) { | |||
| const iterations = Math.min(100, Math.floor(delay / 16)); // 1/60fps is about 16ms | |||
| const chunkSize = delay / iterations; | |||
| @@ -70,8 +86,7 @@ export class DelayTask< | |||
| } else { | |||
| await sleep(delay); | |||
| } | |||
| const output = Object.fromEntries(Object.entries(input).filter(([k]) => k !== "delay")); | |||
| return output as Output; | |||
| return input as unknown as Output; | |||
| } | |||
| } | |||
|
|
|||
| @@ -80,16 +95,16 @@ export class DelayTask< | |||
| * | |||
| * Delays the execution of a task for a specified amount of time | |||
| * | |||
| * @param {delay} - The delay in milliseconds | |||
| * @param config - Task configuration; use `config.delay` for the delay in milliseconds | |||
| */ | |||
| export const delay = (input: DelayTaskInput, config: TaskConfig = {}) => { | |||
| export const delay = (input: DelayTaskInput, config: DelayTaskConfig = { delay: 1 }) => { | |||
There was a problem hiding this comment.
The DelayTaskConfig type defines delay as a required property (line 33), but the delay helper function provides a default value { delay: 1 } on line 100. This creates inconsistency - the type system says delay is required, but the default suggests it's optional.
Either:
- Make
delayoptional in DelayTaskConfig:delay?: number, or - Remove the default value from the helper function signature
Additionally, the configSchema at line 25 defines a default value of 1, and the execute method at line 75 also has a fallback to 1, so the property should likely be optional throughout.
- Modified the DelayTaskConfig type to make the delay property optional, enhancing flexibility in task configuration. - This change aligns with recent updates to task configuration schemas, promoting a more adaptable task architecture.
namewithtitleacross various tasks for consistency.titleproperty.