fix: stop reporting evaluation and grounding failures as empty results

[abrichr]

What this is

A sweep of openadapt-ml for one defect class: a failure rendered as a successful empty result. This repository produces GRPO rewards, grounding metrics and exported artifacts, so a swallowed failure here does not raise — it yields a wrong number.

At each site below the code could not tell "I looked and the answer is empty/zero" from "I could not look", and returned the first shape for the second. Every fix makes the distinction representable (an exception, or an explicit None the caller already handles). No fix is "log it and return the same value".

Fixed, ranked by blast radius

1. GRPO milestone reward — this is the training signal

openadapt_ml/training/grpo/reward.py::evaluate_milestones_screenshot

Condition	Before	After
openadapt-evals not installed	logger.warning(...), return 0.0	MilestoneEvaluationError
VLM judge raised (429, timeout)	caught per milestone; milestone stayed in the denominator, so an API error depressed the reward exactly like an unmet milestone	MilestoneEvaluationError
Milestone with no description	continue, but still counted in total — the task could never score 1.0 and nothing said why	MilestoneEvaluationError
No milestones / none of type screenshot	return 0.0	MilestoneEvaluationError

0.0 is a legal reward. Every one of these fed an invented 0.0 into compute_group_advantages, which normalised it and took a gradient step on a measurement that never happened.

GRPOTrainer._compute_milestone_reward_from_rollout catches the new error at the one place that can decide, and returns None — "there is no measurement here, keep the reward the environment already gave you" — which the caller already handles. None and 0.0 are now different things. _compute_milestone_reward returned 0.0 for an unknown task_id; it now raises KeyError, matching its sibling, which already got this right.

2. Grounding — an API error was recorded as a grounding miss

openadapt_ml/grounding/base.py, detector.py

GeminiGrounder.ground printed the error and returned []. _parse_bbox_response swallowed JSONDecodeError/KeyError/TypeError. extract_ui_elements did both.

[] is the legitimate answer for "I looked and matched nothing", and evaluate_grounder scores it as best_iou=0.0, centroid_hit=False. So a rate limit, a bad key or an unparseable reply was written into the metrics as a grounding failure the model never made. These now raise the new GroundingError; the honest empty result still returns [] (guarded by its own test).

3. Grounding metrics — a shrinking denominator

openadapt_ml/evals/grounding.py::evaluate_grounder_on_episode

A step whose screenshot could not be opened was continued past. Every GroundingMetrics property divides by len(results), so the sample silently left the denominator: the reported hit rate stayed high, computed over fewer samples than the episode contains, with nothing saying so. Now raises.

4. Parquet summary — "wrote nothing" looked like "wrote it"

openadapt_ml/export/parquet.py::_write_summary

except ImportError: return made to_parquet(..., include_summary=True) a no-op that returned None exactly like success.

5. Azure login probe — True meaning "I did not check"

scripts/setup_azure.py::check_az_logged_in

An unrecognised az error returned True ("assume you are logged in"); the caller skipped az login and failed later with an unrelated error. Now re-raises. The two positively-recognised "not logged in" answers still return False, and that path has its own test.

The named defect from #69: task_dir

GRPOConfig declared task_dir twice (PIE794). Both spelled str | None = None, so the surviving default was unchanged and there is no user-visible misbehaviour to report — the duplicate was a latent hazard, not a live bug. #69 kept the correct one: the first declaration, carrying the block comment that matches the class docstring, and the one whose position already fixed the field's place in the generated __init__. It deleted the redundant one-line restatement. That is the right repair.

But nothing in the configured rule set B,E,F,I,W catches a duplicated class field — verified: ruff check --select B,E,F,I,W passes clean against the pre-#69 file. The defect could return the moment the audit stopped. So:

• PIE794 is adopted as a single rule, not the PIE family (repository count at adoption: 0), consistent with how [tool.ruff.lint] documents every other selection.
• tests/test_failure_is_not_success.py adds an AST guard over every dataclass in openadapt_ml/, not just this one.

Mutation checks

Every test here was mutation-checked: the production file was reverted to its pre-fix form (git show e1ff03e: for task_dir, git show origin/main: for the rest) and the matching test confirmed to fail, then restored.

The new exception types are resolved dynamically in the tests, on purpose — importing them directly would make the revert fail with ImportError, which proves only that a name was removed. As written, the revert fails with Failed: DID NOT RAISE, i.e. on the defect itself.

Sample of the recorded pre-fix output:

############ revert openadapt_ml/training/grpo/reward.py + __init__.py + trainer.py
E       Failed: DID NOT RAISE <class 'Exception'>
WARNING  openadapt_ml.training.grpo.reward:reward.py:137 Milestone 'm1' evaluation failed: 429 rate limited
WARNING  openadapt_ml.training.grpo.reward:reward.py:137 Milestone 'm2' evaluation failed: 429 rate limited
4 failed, 1 passed

############ revert openadapt_ml/grounding/{detector,base,__init__}.py
E       Failed: DID NOT RAISE <class 'Exception'>
Gemini grounding error: 503 backend unavailable
Error extracting UI elements: quota exceeded
3 failed, 1 passed

############ revert openadapt_ml/training/grpo/config.py to e1ff03e
E       AssertionError: GRPOConfig re-declares dataclass field(s): GRPOConfig.task_dir at line 94
2 failed, 1 passed

############ revert openadapt_ml/evals/grounding.py
E       Failed: DID NOT RAISE <class 'ValueError'>

############ revert openadapt_ml/export/parquet.py
E       Failed: DID NOT RAISE <class 'ImportError'>

############ revert scripts/setup_azure.py
E       Failed: DID NOT RAISE <class 'subprocess.CalledProcessError'>

The ruff gate was mutation-checked the same way: with the pre-#69 duplicate restored, uv run ruff check . now exits 1 with PIE794 Class field 'task_dir' is defined multiple times; on merged main it exits 0.

The three tests that pass in both directions are deliberate: they guard the honest paths (a real no-match still returns [], a measured 0.5 is still 0.5, a recognised auth error still returns False) so these fixes cannot be "improved" into raising on normal operation.

Fake gates

Grepped .github/workflows/ for || true, continue-on-error, exit 0, set +e, --exit-zero, || :. Zero hits. No gate in this repository is pretending to be a gate. #69's widened ruff check . is genuinely blocking — confirmed by making it fail on demand.

Sweep status of the items #69 surfaced

Checked against merged origin/main, not a local checkout:

Rule	Reported	On merged main
F821 undefined name (x3)	guaranteed NameError	0 — fixed by #69
F811 redefined-while-unused (x5)		0 — fixed by #69
F841 unused variable (x4)		0 — fixed by #69
E712, E722 bare except (x2)		0 — fixed by #69
PIE794	the task_dir duplicate	0 — fixed by #69, now gated by this PR
S110/S112 swallowed exceptions		37 live — 6 fixed here, 31 dismissed, see below

Judged NOT to be defects (with reasons)

• 28 of the 37 S110/S112 are in openadapt_ml/cloud/{local,lambda_labs,vast_ai,ssh_tunnel,modal_cloud}.py. These are best-effort status and cleanup probes on infrastructure that is allowed to be absent, and they already return False/None, which is an honest "not there" — not a success shape. Blast radius is a dashboard row, not a metric.
• ssh_tunnel._is_port_in_use returning True on OSError — reads like the pattern but is correct: bind() failing is the evidence the port is in use.
• _parse_vlm_output_to_action defaulting to DONE() on unparseable model output — logged, and DONE-terminates the episode. It is a deliberate rollout policy, not a swallowed failure; changing it changes training semantics and belongs in its own change with its own evidence.
• GRPOTrainer screen_size fallbacks and action._grpo_raw_text AttributeError — narrow, genuinely optional, and fall back to a configured value rather than to a fabricated measurement.
• setup_azure.py return True at the "quota already at level" and "role already assigned" branches — these are real success states positively recognised from the error text, not guesses.
• scripts/{check_quota,p1_episode_success_ab_test,run_demo_experiment_n30}.py S110s — genuine instances, but in one-off analysis scripts with no importers. Reported rather than fixed, to keep this PR reviewable.
• 81 PLW1510 (subprocess.run without check=) — every fix is a behaviour change needing per-call judgement, as pyproject.toml already records. Not adopted here.

Verification

uv run ruff check .                          -> All checks passed!
uv run ruff format --check openadapt_ml/     -> 109 files already formatted
uv run pytest tests/ --ignore=tests/integration
                                             -> 470 passed, 2 skipped

One test was found reaching the live Gemini API while being written (import google.generativeai as genai binds from the parent package attribute, so patching sys.modules alone leaves the real client in place). Fixed to patch both; the test now runs offline in 0.08s.

fix: — this will cut a patch release on merge, under the standing release authorization. PyPI publication will be verified from the index, not from a green run.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NyCHrzA1psrKMFfroYbzaM