huggingface / huggingface/transformers-ci

serge verify verdicts are category-blind: correct import_or_config and OOM fixes are reported as failures

Open
#117 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
1
Forks
7
Avg merge
1h 30m
Merged PRs (30d)
39

Description

The serge GPU verify gate re-runs a failure group's own node-ids, 5× per tree, in isolation. That is the whole story for `output_mismatch` and crash groups. For two other ITF failure categories the **correct** fix cannot show up that way, so the verdict comes back as a rejection — and on the serge side anything that is not `fixed` and not in the unjudged set deletes the branch and opens no PR. The nightly then re-dispatches the same group and re-spends the budget.

## Evidence

serge ran for three nightlies (2026-09-04..06) with `VERIFY_ON_GPU` unset, so 11 fix PRs were opened without the gate. Re-verifying all 11 today gave 4 verdicts that do not mean what they say:

| PR | category | what happened | verdict | should be |
|---|---|---|---|---|
| huggingface/transformers#48582 | `import_or_config` | fix adds `@require_flash_attn`; baseline failed `ImportError: FlashAttention2 … doesn't seem to be installed`, patched **skipped** | `error` | `fixed` — the skip *is* the fix |
| huggingface/transformers#48551 | `OOM` (retained memory) | fix is `tearDown` → `cleanup(gc_collect=True)`; isolated re-run OOMed at *load* | `not_fixed` | unjudged |
| huggingface/transformers#48538 | `OOM` (retained memory) | both trees died in weight conversion before the test body ran | `not_fixed` | unjudged |
| huggingface/transformers#48581 | `OOM` (retained memory) | fix worked — test got past the OOM and then failed its own assertion | `not_fixed` | correct, but it flattens `OutOfMemoryError → AssertionError` into a flat red |

Three distinct problems: a **wrong verdict**, two **verdicts computed from runs that never exercised the patch**, and one where the verdict is right but the useful signal is discarded.

## Mechanism

In `src/transformersci/agentic/serge_verify_verdict.py` (~287–316) the patched tree's outcomes collapse into two buckets — `failed|error` → `not_fixed`, `missing|skipped` → `error`. Neither knows the failure **category**, and neither separates *"the test ran and stayed red"* from *"the test never ran"*.

## Proposed

1. **A guard-fix skip is a fix.** Patched-tree `skipped` whose baseline failed with `ImportError` / `ModuleNotFoundError` → `fixed`. Every other skip stays `error`. This is exactly the `import_or_config` fix shape, and it is what a maintainer would write by hand.
2. **Distinguish "could not tell" from "did not work."** When both trees fail with the same exception at the same frame *before* the test body (load, collection, weight conversion), emit a new unjudged verdict `not_exercised` instead of `not_fixed`.
3. **Report a changed failure mode.** When both trees fail with *different* exception types, carry `failure_changed: "OutOfMemoryError → AssertionError"` in the artifact and surface it in the PR body and the recap. #48581 is real progress that currently reads identically to a patch that did nothing.
4. **Run retained-memory groups the way they fail.** For `OOM` groups whose report says *retained memory (fixable)*, run the targeted test's **enclosing class in one pytest process** on both trees rather than the node-ids alone. Retained memory is by definition a property of tests sharing a process; an isolated re-run cannot reproduce it, which is why #48551 and #48538 produced verdicts about their runner rather than their patch.

(1)–(3) are local to the verdict computer; (4) touches the reusable workflow's pytest invocation.

## Validation

The four runs above are ready-made fixtures with known-correct answers — the same approach that pinned the expectation classifier to five real PRs in serge#111. Verify runs, for reference: [48582](https://github.com/huggingface/transformers/actions/runs/34200518951), [48551](https://github.com/huggingface/transformers/actions/runs/34200506255), [48538](https://github.com/huggingface/transformers/actions/runs/34200502707), [48581](https://github.com/huggingface/transformers/actions/runs/34200515867).

## Note on shipping

The verdict tool is installed from `transformersci_ref` (default `main`), so a change here is live on the very next verify run — no deploy, no image bump. Same "merging is shipping" property as the ITF nightly.

The serge half — accepting `not_exercised` as unjudged so the branch survives and gets reported — is tracked separately.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start in src/transformersci/agentic/serge_verify_verdict.py around lines 287–316 and compare the four linked verification runs with their expected verdicts. Then trace the reusable workflow’s pytest invocation for retained-memory OOM groups. Done means guard-fix skips are fixed, pre-body identical failures are not_exercised, changed failure types are surfaced, and retained-memory groups run through their enclosing class.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
ci-cd, testing
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.