Repository navigation
[BREAKING] FEAT: Add scenario dataset controls to CLI, REST, and GUI - #3143
Richard Lundeen (richlundeen) wants to merge 2 commits into
Conversation
…faces Expose scenario-owned dataset options, shared requests, and source-level controls in Python, REST, CLI, shell, and GUI. Preserve read-only estimates and resume behavior. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 16cbdc1b-2bf6-4286-be66-c9f30c7ed72b
Retain the dataset controls and current backend-loop preparation. Adapt ProPILE and responsiveness tests to the shared dataset request path. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 16cbdc1b-2bf6-4286-be66-c9f30c7ed72b
|
|
||
| VERSION: int = 1 | ||
| DATASET_OPTIONS = ScenarioDatasetOptions( | ||
| mode=DatasetSelectionMode.FIXED, per_dataset_limit=False, total_limit=False |
There was a problem hiding this comment.
🔴 Must Fix: This advertises filters=True, but the scenario does not use the filtered seeds to build its prompts. _collect_named_seeds_async() checks the filters, then _load_system_prompts_async() and _load_templates_by_category_async() read the datasets again without those filters.
I reproduced this with two rows per dataset, tagged selected and excluded, and datasets={"filters": {"harm_categories": ["selected"]}}. The filtered reads returned one row per dataset, but the scenario still built 8 combinations instead of 2, including the excluded rows. The estimate also reported the filter as effective. WebInjection has the same filtered-check followed by an unfiltered read.
Could we either build from the filtered results or set filters=False for these scenarios so unsupported filters fail before preparation? Please add a regression test with both matching and nonmatching rows, not only a filter that matches nothing.
| setErrors({}) | ||
| onValidationChange(null) |
There was a problem hiding this comment.
Must Fix: Resetting sources also clears validation errors for global limits that it does not reset. For example, enter 3 in Max total, then change it to invalid text 0, and click Use default sources. The field still shows 0, but its error is cleared and the form can launch with the previous valid limit 3. If the previous value was all, this can launch an unlimited run.
invalidText lives in each DatasetLimitField, while this button clears every parent error and calls onValidationChange(null). Please clear only errors for source controls that are actually reset, or reset the corresponding field state and request value together.
The remove-source handler at line 160 has the opposite issue: it removes the row but leaves its validation error in errors, so a remaining valid source selection still cannot launch. Please cover both paths: unchanged invalid global limits must keep blocking launch, and errors for removed source rows must stop blocking it.
| const load = async (): Promise<void> => { | ||
| try { | ||
| const data = await datasetsApi.list(controller.signal) | ||
| if (controller.signal.aborted) return |
There was a problem hiding this comment.
Should Fix: Please update the mock E2E fixtures for this new request. The current-head Frontend E2E (mock, shard 2/2) job fails with Unexpected API request: GET /api/datasets in numeric-controls.spec.ts. Its strict response map does not include this endpoint, so opening a scenario fails before the stepper assertions can finish.
Add a dataset metadata response to the affected scenario-page fixtures. The test at numeric-controls.spec.ts:167 also still looks for the removed Max dataset size spinbutton, so adding the endpoint alone will not fix the suite. Update that test for the new Max total textbox and its default, all, and invalid-input behavior while keeping the remaining numeric-control coverage.
The failed job is 114591620149, for the reviewed commit 4121aeb21c7.
| ) | ||
| return ( | ||
| <section className={styles.root} aria-label="Dataset configuration"> | ||
| <Text weight="semibold">Datasets</Text> |
There was a problem hiding this comment.
Could you add screenshots of the new Datasets table to the PR description? Please show the default view, an edited selection with limits and fetch policies, and a restricted or invalid-selection state so we can review how the controls look in practice.
This branch has not been deployed
2 participants
Description
Implements PR 2 of the scenario dataset proposal, using the preparation and limit APIs merged in #2956.
Adds scenario-owned dataset options and one dataset override path for Python, REST, scanner, and shell. The GUI dataset table shows source limits, fetch policies, stored origins, counts, and provider availability. Estimates remain read-only. Resume uses the existing launch record and stored run plan; there is no v2 format or separate settings snapshot.
Breaking: dataset overrides that a scenario cannot apply now fail instead of being ignored. This includes total limits on web injection and system-prompt extraction. Existing dataset defaults and inline data paths remain unchanged. Deprecated request fields and CLI flags remain supported; incompatible combinations fail. Generator configuration and refresh are out of scope.