Skip to content

Dataset refresh: open draft PR with per-eval validation links#741

Merged
Groenbech96 merged 19 commits into
mainfrom
feature/dataset-refresh-draft-pr
Jul 15, 2026
Merged

Dataset refresh: open draft PR with per-eval validation links#741
Groenbech96 merged 19 commits into
mainfrom
feature/dataset-refresh-draft-pr

Conversation

@Groenbech96

@Groenbech96 Groenbech96 commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

What & why

Ships the semi-automated dataset refresh from work item 632829: keep BC-Bench current and resist contamination, with a human in the loop.

Haoran's collect-and-screen job already screens merged BCApps PRs and collects passing candidates onto a per-ISO-week branch dataset/week-XX. This PR turns that branch into a reviewed, validated draft PR.

Flow (wait → validate → prune → PR)

  1. Collect candidates (screened) into dataset/bcbench.jsonl + dataset/problemstatement, push dataset/week-XX.
  2. Dispatch dataset-validation on the branch and block, polling until it completes (~4h30m budget). Note: the collect job now waits on validation rather than running in parallel — this is intentional so failed entries never reach a human.
  3. Read each entry's validation conclusion (per-matrix-job, matched by job.name == instance_id).
  4. Prune entries whose conclusion isn't success from the branch (remove JSONL line + problemstatement dir, amend + force-push) — so the PR diff contains only entries that passed the fail→pass reference check.
  5. Open/update a draft PR when ≥1 entry passed. Body has ✅ Included (passed, in the diff) and ❌ Excluded (failed/incomplete, documented only, each linked to its validation job). If 0 pass: no PR, failures written to the run summary; the branch is deleted only when results are determinate and no open PR already points at it.

Validation is reference-based (gold patch, no agent), so this filter removes malformed/unbuildable entries only — it does not select by agent difficulty and does not skew eval results.

Changes

  • scripts/Collect-And-Screen.ps1-CollectedFile emits collected entries as JSON.
  • scripts/New-DatasetPrBody.ps1 — two-section (Included/Excluded) body by validation conclusion.
  • scripts/Remove-DatasetEntries.ps1 — prune entries by instance-id.
  • .github/workflows/collect-and-screen.yml — wait/prune/branch logic, pull-requests: write, draft-PR create/update.

Operational requirement

The org blocks GITHUB_TOKEN from opening PRs, so the PR step uses GH_TOKEN: ${{ secrets.DATASET_PR_TOKEN || secrets.GITHUB_TOKEN }}. A DATASET_PR_TOKEN secret (Contents + Pull requests: write) has been provisioned.

Validation

  • Scripts covered by fixture tests; Python suite green (608 passed, 1 skipped); workflow YAML parses.
  • Live smoke test: a full run collected microsoft__BCApps-9316, waited for validation (passed), pruned nothing, and opened draft PR Dataset refresh: week 29 candidates from microsoft/BCApps #742 with the ✅ Included section and a per-eval job deep-link — confirming job.name == instance_id matching end-to-end.

Magnus Hartvig Grønbech and others added 18 commits July 13, 2026 13:20
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…PRs)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
COPILOT_PAT5 returns 401 Bad credentials; use a dedicated PR token secret instead.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ty ids

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@gggdttt

gggdttt commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Pulled the branch and read through the workflow + the two new scripts. Overall this looks solid — the guards around the irreversible branch delete, the gh retries, and the idempotent create/update are all nicely done. A few things I'd want to sort out before this goes in:

1. The whole thing hinges on job.name == entry.Id. Both the prune step and New-DatasetPrBody match a collected entry to its validation job purely by name. dataset-validation.yml uses name: ${{ matrix.entry }} so today that resolves to the instance id and lines up — but it's a single point of failure. If that ever drifts (or bcbench collect gh writes a slightly different instance_id), every entry reads as "not passed" and the branch gets deleted. The end-to-end smoke test is deferred to post-merge, so I'd really like to see this matching confirmed on a live run before merging rather than after.

2. The results-determinate check is a bit too loose and can delete a good branch. $jobsCount = @($jobsJson | ConvertFrom-Json).Count counts all jobs in the run, including get-entries. So if the matrix ever comes up empty (e.g. modified-only doesn't pick anything up), you still get determinate=true with zero successes → branch deleted. Since the intent is "if we're not sure, keep the branch", I think determinate should require at least one job whose name actually matches a collected id, not just jobsCount > 0.

3. A same-week re-run that fails could kill an already-good PR. The branch is per ISO week. If run #1 opens a draft PR with passing entries, and a later same-week run happens to fail everything, the summarize step deletes the branch and closes that good PR. Maybe skip the delete if there's already an open PR pointing at the branch?

4. Description vs. behavior. The write-up says the collect job stays lightweight and validation runs in parallel, but the implementation actually blocks and polls for up to ~4h40m. That's fine cost-wise for a weekly job, but worth updating the description so it matches the wait-then-prune flow. Also timeout-minutes: 350 is cutting it close (280 wait + collect + the poll loops) — I'd give it more headroom.

Couple of nits, not blockers:

  • $existing -ne 'null' is redundant since // empty already gives you ''.
  • ConvertTo-Json -AsArray needs PS 6.2+ — fine on CI, just won't run under Windows PowerShell 5.1 locally.

Happy to pair on #2/#3 if useful.

… PR, headroom

- results-determinate now requires >=1 job matching a collected instance id (empty matrix / name drift => indeterminate => keep branch)

- 0-passed delete now skipped when an open PR already points at the branch

- trim validation wait budget 280->270 for timeout headroom; drop redundant -ne 'null' guard

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Groenbech96

Groenbech96 commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the careful read — pushed fixes for the substantive points.

2. results-determinate too loose (real bug). Fixed. It no longer uses jobsCount > 0 (which counted get-entries). Determinate now requires the run to have completed and at least one job whose name matches a collected instance_id. So an empty matrix, or a job.name/instance_id drift, now reads as indeterminate → branch kept, never deleted.

3. Same-week re-run could close a good PR. Fixed. The 0-passed branch-delete now runs only when there's no open PR pointing at the branch; if one exists we keep the branch (and write the failure summary) rather than auto-closing something under review.

1. Everything hinges on job.name == instance_id. Agreed it's the linchpin. It's now confirmed on a live end-to-end run: the smoke test collected microsoft__BCApps-9316, waited for validation, and opened #742 whose body carries a per-eval job deep-link (…/runs/29335506877/job/87093772200) — that URL only resolves if the match held. And with fix #2, a future drift degrades safely to "keep branch" instead of deleting.

4. Description vs. behavior + timeout. Updated the PR description to the wait-then-prune flow (it does block and poll, by design, so failed entries don't reach a reviewer). Trimmed the wait budget 280→270 (~4h30m) for more headroom under timeout-minutes: 350.

Nits: dropped the redundant $existing -ne 'null' (the // empty already yields ''). Left ConvertTo-Json -AsArray — CI and local dev both run pwsh 7, so the 6.2+ requirement is fine; shout if you want a 5.1-safe fallback.

@Groenbech96

Copy link
Copy Markdown
Collaborator Author

Running a new smoke test on this.

@Groenbech96
Groenbech96 merged commit d5586e2 into main Jul 15, 2026
14 checks passed
@Groenbech96
Groenbech96 deleted the feature/dataset-refresh-draft-pr branch July 15, 2026 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants