From 471f50c5edf75ab812ba7e2c4c0f741b976e06ce Mon Sep 17 00:00:00 2001 From: abrichr Date: Mon, 27 Jul 2026 23:19:39 -0400 Subject: [PATCH] fix: stop reporting evaluation and grounding failures as empty results A failure rendered as a successful empty result is the defect class this project exists to catch in other people's software. This repository produces GRPO rewards, grounding metrics and exported artifacts, so here the same shape does not raise -- it yields a wrong number. Each site below could not tell "I looked and the answer is empty/zero" from "I could not look", and returned the first for the second. Each fix makes the distinction representable rather than logged. GRPO milestone reward (openadapt_ml/training/grpo/reward.py) -- highest blast radius, it is the training signal: * openadapt-evals not installed -> logged a warning and returned reward 0.0. * VLM judge raised -> caught per milestone; the milestone stayed in the denominator, so a 429 depressed the reward exactly like an unmet milestone. * Milestone with no description -> `continue`d past but still counted, so the task could never score 1.0 and nothing said why. * No milestones / none of type "screenshot" -> returned 0.0. All now raise MilestoneEvaluationError. GRPOTrainer catches it at the one place that can decide, and returns None ("no measurement, keep the existing reward"), which the caller already handles -- rather than feeding an invented 0.0 into compute_group_advantages and taking a gradient step on it. GRPOTrainer's unused `_compute_milestone_reward` raised the same 0.0 for an unknown task_id; it now raises KeyError, matching its sibling. Grounding (openadapt_ml/grounding/): `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 or a bad key was recorded as a grounding miss the model never made. These now raise GroundingError; the honest empty result still returns []. Grounding metrics (openadapt_ml/evals/grounding.py): a step whose screenshot could not be opened was `continue`d past. Every GroundingMetrics property divides by len(results), so the sample left the denominator and the reported hit rate stayed high over a silently smaller sample. Now raises. Parquet summary (openadapt_ml/export/parquet.py): `except ImportError: return` made `include_summary=True` a no-op that returned None exactly like success. Azure login probe (scripts/setup_azure.py): an unrecognised az error returned True -- "I could not tell, so assume you are logged in". Now re-raises. Also: PR #69 fixed `task_dir` being declared twice in GRPOConfig (PIE794) and kept the correct, documented declaration. Nothing in the configured rule set `B,E,F,I,W` catches a duplicated class field, so the defect could return the moment the audit stopped. PIE794 is adopted as a single rule (repository count: 0) and tests/test_failure_is_not_success.py adds an AST guard over every dataclass in the package. Every test in tests/test_failure_is_not_success.py is mutation-checked: with the production file reverted to its pre-fix form the matching test fails with "DID NOT RAISE" (behaviour, not a missing import -- the new exception types are resolved dynamically for exactly that reason). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NyCHrzA1psrKMFfroYbzaM --- openadapt_ml/evals/grounding.py | 23 +- openadapt_ml/export/parquet.py | 16 +- openadapt_ml/grounding/__init__.py | 2 + openadapt_ml/grounding/base.py | 19 +- openadapt_ml/grounding/detector.py | 64 +++- openadapt_ml/training/grpo/__init__.py | 2 + openadapt_ml/training/grpo/reward.py | 81 +++-- openadapt_ml/training/grpo/trainer.py | 34 +- pyproject.toml | 8 +- scripts/setup_azure.py | 11 +- tests/test_failure_is_not_success.py | 464 +++++++++++++++++++++++++ 11 files changed, 671 insertions(+), 53 deletions(-) create mode 100644 tests/test_failure_is_not_success.py diff --git a/openadapt_ml/evals/grounding.py b/openadapt_ml/evals/grounding.py index cd3ffbb..eef7ae7 100644 --- a/openadapt_ml/evals/grounding.py +++ b/openadapt_ml/evals/grounding.py @@ -210,6 +210,10 @@ def evaluate_grounder_on_episode( Returns: GroundingMetrics for click actions with bboxes. + + Raises: + ValueError: If a step records a screenshot path that cannot be opened. + Dropping it would shrink the metric denominator without saying so. """ from PIL import Image @@ -242,11 +246,24 @@ def evaluate_grounder_on_episode( if step.observation.screenshot_path is None: continue - # Load image + # Load image. + # + # Previously `except Exception: continue`. Every metric on + # GroundingMetrics divides by len(self.results), so a screenshot that + # cannot be opened used to disappear from the denominator: the reported + # hit rate stayed high and nothing said it was computed over fewer + # samples than the episode contains. An episode that records a + # screenshot path it cannot read is a broken episode, not a smaller one. try: image = Image.open(step.observation.screenshot_path) - except Exception: - continue + except Exception as e: + raise ValueError( + f"Episode {getattr(episode, 'episode_id', '?')!r} step " + f"{getattr(step, 'step_index', '?')} references screenshot " + f"{step.observation.screenshot_path!r}, which could not be " + f"opened ({type(e).__name__}: {e}). Refusing to score the " + "episode over a silently reduced sample set." + ) from e # Create target description from reasoning or action coordinates coords_x, coords_y = None, None diff --git a/openadapt_ml/export/parquet.py b/openadapt_ml/export/parquet.py index f40c3ca..29936fb 100644 --- a/openadapt_ml/export/parquet.py +++ b/openadapt_ml/export/parquet.py @@ -127,12 +127,22 @@ def to_parquet( def _write_summary(episodes: list[Episode], output_path: str) -> None: - """Write episode-level summary Parquet file.""" + """Write episode-level summary Parquet file. + + Raises: + ImportError: If pyarrow is unavailable. Previously this returned + silently, so ``to_parquet(..., include_summary=True)`` reported + success while writing no summary file at all -- the caller had no + way to tell an empty summary from a missing one. + """ try: import pyarrow as pa import pyarrow.parquet as pq - except ImportError: - return + except ImportError as e: + raise ImportError( + "Parquet summary export requires pyarrow. " + "Install with: pip install 'openadapt-ml[parquet]'" + ) from e summary_rows = [] for episode in episodes: diff --git a/openadapt_ml/grounding/__init__.py b/openadapt_ml/grounding/__init__.py index a4171ba..7dde99f 100644 --- a/openadapt_ml/grounding/__init__.py +++ b/openadapt_ml/grounding/__init__.py @@ -23,6 +23,7 @@ """ from openadapt_ml.grounding.base import ( + GroundingError, GroundingModule, OracleGrounder, RegionCandidate, @@ -35,6 +36,7 @@ ) __all__ = [ + "GroundingError", "GroundingModule", "OracleGrounder", "RegionCandidate", diff --git a/openadapt_ml/grounding/base.py b/openadapt_ml/grounding/base.py index cb87797..932e5dc 100644 --- a/openadapt_ml/grounding/base.py +++ b/openadapt_ml/grounding/base.py @@ -17,6 +17,18 @@ from PIL import Image +class GroundingError(RuntimeError): + """Raised when a grounding call could not be completed. + + Grounding returns an empty candidate list to mean "I looked and nothing + matched". That is a real, scoreable outcome: `evaluate_grounder` counts it + as a miss. An API error, a missing key, or an unparseable model response is + a different thing entirely -- nothing was looked at -- and must not be + reported with the same empty list, because the evaluation would then record + a grounding failure that the grounder never actually made. + """ + + @dataclass class RegionCandidate: """A candidate region for action execution. @@ -138,7 +150,12 @@ def ground( Returns: List of candidate regions, sorted by confidence descending. - Returns empty list if no candidates found. + An empty list means the grounder looked and found no match. It + must not be used to report that the grounder could not run. + + Raises: + GroundingError: If the grounding call could not be completed + (backend error, missing credentials, unparseable response). """ pass diff --git a/openadapt_ml/grounding/detector.py b/openadapt_ml/grounding/detector.py index 8d957e2..ec5c1d0 100644 --- a/openadapt_ml/grounding/detector.py +++ b/openadapt_ml/grounding/detector.py @@ -17,7 +17,11 @@ from typing import TYPE_CHECKING from openadapt_ml.config import settings -from openadapt_ml.grounding.base import GroundingModule, RegionCandidate +from openadapt_ml.grounding.base import ( + GroundingError, + GroundingModule, + RegionCandidate, +) if TYPE_CHECKING: from PIL import Image @@ -98,7 +102,14 @@ def _parse_bbox_response( image_height: Image height for normalization. Returns: - List of RegionCandidate objects. + List of RegionCandidate objects. Empty means the model replied + with a well-formed but empty match set. + + Raises: + GroundingError: If the response could not be parsed. An + unparseable reply is not "no elements matched"; nothing was + read at all, and returning an empty list would let the caller + score it as a grounding miss. """ candidates = [] @@ -106,7 +117,10 @@ def _parse_bbox_response( # Look for JSON array or object in the response json_match = re.search(r"\[[\s\S]*\]|\{[\s\S]*\}", response_text) if not json_match: - return candidates + raise GroundingError( + "Gemini response contained no JSON array or object; the model " + f"reply could not be read at all. Reply was: {response_text!r:.500}" + ) try: data = json.loads(json_match.group()) @@ -165,8 +179,11 @@ def _parse_bbox_response( ) ) - except (json.JSONDecodeError, KeyError, TypeError): - pass + except (json.JSONDecodeError, KeyError, TypeError) as e: + raise GroundingError( + f"Could not parse Gemini bbox response ({type(e).__name__}: {e}). " + f"Reply was: {response_text!r:.500}" + ) from e return candidates @@ -184,7 +201,12 @@ def ground( k: Maximum number of candidates to return. Returns: - List of RegionCandidate objects sorted by confidence. + List of RegionCandidate objects sorted by confidence. Empty means + Gemini looked and matched nothing. + + Raises: + GroundingError: If the Gemini call or its response could not be + completed. Never reported as an empty candidate list. """ model = self._get_model() @@ -230,10 +252,16 @@ def ground( candidates.sort(key=lambda c: c.confidence, reverse=True) return candidates[:k] + except GroundingError: + raise except Exception as e: - # Log error but don't crash - print(f"Gemini grounding error: {e}") - return [] + # Previously: printed the error and returned []. `evaluate_grounder` + # scores [] as best_iou=0.0 / centroid_hit=False, so a rate-limited + # or unreachable API silently became a reported grounding failure. + raise GroundingError( + f"Gemini grounding call failed for {target_description!r} " + f"({type(e).__name__}: {e})" + ) from e @property def supports_batch(self) -> bool: @@ -283,6 +311,9 @@ def extract_ui_elements( Raises: ImportError: If google-generativeai package not installed. ValueError: If GOOGLE_API_KEY not set. + GroundingError: If the extraction call or its response could not be + completed. An empty list is reserved for "the screenshot has no + interactive elements". """ try: import google.generativeai as genai @@ -384,12 +415,19 @@ def extract_ui_elements( return normalized_elements + # Previously both handlers printed and returned []. An empty list is the + # legitimate answer for "this screenshot has no interactive elements", so a + # quota error, a network failure, or a truncated reply became an assertion + # that the screen was empty -- and Set-of-Marks callers overlaid nothing. except (json.JSONDecodeError, KeyError, TypeError) as e: - print(f"Failed to parse Gemini response: {e}") - return [] + raise GroundingError( + f"Could not parse the Gemini element-extraction response " + f"({type(e).__name__}: {e})" + ) from e except Exception as e: - print(f"Error extracting UI elements: {e}") - return [] + raise GroundingError( + f"Gemini element extraction failed ({type(e).__name__}: {e})" + ) from e def overlay_element_marks( diff --git a/openadapt_ml/training/grpo/__init__.py b/openadapt_ml/training/grpo/__init__.py index 857a4a3..15c1d15 100644 --- a/openadapt_ml/training/grpo/__init__.py +++ b/openadapt_ml/training/grpo/__init__.py @@ -49,6 +49,7 @@ generate_cot_annotations, ) from openadapt_ml.training.grpo.reward import ( + MilestoneEvaluationError, binary_task_success, compute_group_advantages, evaluate_milestones_screenshot, @@ -84,6 +85,7 @@ def __getattr__(name: str): "GRPOConfig", "GRPOTrainer", "GRPORolloutCollector", + "MilestoneEvaluationError", "Rollout", "binary_task_success", "compute_group_advantages", diff --git a/openadapt_ml/training/grpo/reward.py b/openadapt_ml/training/grpo/reward.py index 1d90678..48caf26 100644 --- a/openadapt_ml/training/grpo/reward.py +++ b/openadapt_ml/training/grpo/reward.py @@ -20,6 +20,17 @@ logger = logging.getLogger(__name__) +class MilestoneEvaluationError(RuntimeError): + """Raised when milestone rewards could not be computed. + + This exists so that "the agent did not reach any milestone" (reward 0.0) + and "this evaluation could not run" are different outcomes. Returning 0.0 + for the second case makes an infrastructure failure look like a training + signal: the trainer would compute advantages, take a gradient step, and + log a reward mean, all from a number that measures nothing. + """ + + def binary_task_success(score: float, threshold: float = 0.5) -> float: """Convert evaluator score to binary reward. @@ -91,34 +102,57 @@ def evaluate_milestones_screenshot( vlm_provider: VLM provider (``"openai"`` or ``"anthropic"``). Returns: - Fraction of screenshot milestones that passed (0.0 to 1.0). - Returns 0.0 if there are no milestones or no screenshot milestones. + Fraction of screenshot milestones that passed (0.0 to 1.0). 0.0 means + every screenshot milestone was evaluated and none passed. + + Raises: + MilestoneEvaluationError: If the evaluation could not be run at all -- + no milestones, no locally evaluable milestones, ``openadapt-evals`` + missing, a milestone with no description, or a VLM judge failure. + These used to return (or silently contribute) 0.0, which is + indistinguishable from a genuine failed rollout and would be fed + straight into the GRPO advantage computation as if it were a + measurement. Callers that want to continue must catch this and + decide explicitly what to do with the missing measurement. """ milestones = getattr(task_config, "milestones", None) if not milestones: - return 0.0 + raise MilestoneEvaluationError( + f"Task config {getattr(task_config, 'id', task_config)!r} has no " + "milestones, so there is nothing to score. This is not a reward " + "of 0.0." + ) # Only evaluate screenshot-type milestones locally screenshot_milestones = [ ms for ms in milestones if getattr(ms.check, "check", None) == "screenshot" ] if not screenshot_milestones: - return 0.0 + raise MilestoneEvaluationError( + f"Task config {getattr(task_config, 'id', task_config)!r} has " + f"{len(milestones)} milestone(s) but none of type 'screenshot'. " + "Local screenshot evaluation cannot score this task; use the WAA " + "/evaluate endpoint instead." + ) try: from openadapt_evals.vlm_evaluator import vlm_judge - except ImportError: - logger.warning( - "openadapt-evals is not installed; cannot evaluate screenshot " - "milestones. Install with: pip install openadapt-evals" - ) - return 0.0 + except ImportError as exc: + raise MilestoneEvaluationError( + "openadapt-evals is not installed; screenshot milestones cannot be " + "evaluated. Install with: pip install openadapt-evals" + ) from exc passed = 0 for ms in screenshot_milestones: description = getattr(ms.check, "description", None) or "" if not description: - continue + raise MilestoneEvaluationError( + f"Milestone {getattr(ms, 'name', '?')!r} is a screenshot " + "milestone with no description, so the VLM judge has nothing " + "to check. Skipping it would leave it in the denominator and " + "silently depress the reward." + ) try: success, _confidence = vlm_judge( screenshot_bytes, @@ -126,22 +160,21 @@ def evaluate_milestones_screenshot( model=vlm_model, provider=vlm_provider, ) - if success: - passed += 1 - logger.debug( - "Milestone '%s': %s", - getattr(ms, "name", "?"), - "PASS" if success else "FAIL", - ) except Exception as exc: - logger.warning( - "Milestone '%s' evaluation failed: %s", - getattr(ms, "name", "?"), - exc, - ) + raise MilestoneEvaluationError( + f"VLM judge failed on milestone {getattr(ms, 'name', '?')!r}: " + f"{exc}. A judge failure is not a failed milestone." + ) from exc + if success: + passed += 1 + logger.debug( + "Milestone '%s': %s", + getattr(ms, "name", "?"), + "PASS" if success else "FAIL", + ) total = len(screenshot_milestones) - score = passed / total if total > 0 else 0.0 + score = passed / total logger.info( "Milestone evaluation: %d/%d screenshot milestones passed (%.2f)", passed, diff --git a/openadapt_ml/training/grpo/trainer.py b/openadapt_ml/training/grpo/trainer.py index 9de603d..51060e4 100644 --- a/openadapt_ml/training/grpo/trainer.py +++ b/openadapt_ml/training/grpo/trainer.py @@ -46,6 +46,7 @@ from openadapt_ml.datasets.next_action import SYSTEM_PROMPT from openadapt_ml.training.grpo.config import GRPOConfig from openadapt_ml.training.grpo.reward import ( + MilestoneEvaluationError, compute_group_advantages, evaluate_milestones_screenshot, ) @@ -395,8 +396,7 @@ def _compute_milestone_reward( """Compute milestone-based reward for a task using VLM judge. Evaluates screenshot-type milestones locally without needing the - WAA /evaluate endpoint. Falls back to 0.0 if the task has no - milestones or the task_id is not found in loaded configs. + WAA /evaluate endpoint. Args: task_id: The task ID to look up in loaded configs. @@ -404,10 +404,18 @@ def _compute_milestone_reward( Returns: Fraction of screenshot milestones passed (0.0 to 1.0). + + Raises: + KeyError: If ``task_id`` has no loaded task config. An unknown task + is a configuration error, not a rollout that scored zero. + MilestoneEvaluationError: If the evaluation could not be run. """ task_config = self._task_configs.get(task_id) if task_config is None: - return 0.0 + raise KeyError( + f"No task config loaded for task_id {task_id!r}; loaded ids are " + f"{sorted(self._task_configs)}. Cannot compute a milestone reward." + ) return evaluate_milestones_screenshot(task_config, screenshot_bytes) def _compute_milestone_reward_from_rollout( @@ -416,8 +424,11 @@ def _compute_milestone_reward_from_rollout( ) -> float | None: """Extract the last screenshot from a rollout and compute milestone reward. - Returns None if no task config or no screenshot is available, - signalling the caller to keep the existing reward. + Returns None -- not 0.0 -- when no milestone measurement exists: no task + config, no milestones, no screenshot, or an evaluation that could not + run. None tells the caller "there is no measurement here, keep the + reward you already have"; 0.0 would claim the rollout was measured and + scored nothing, and would then be normalised into a GRPO advantage. """ task_config = self._task_configs.get(rollout.task_id) if task_config is None or not getattr(task_config, "milestones", None): @@ -436,7 +447,18 @@ def _compute_milestone_reward_from_rollout( if not screenshot_bytes: return None - return evaluate_milestones_screenshot(task_config, screenshot_bytes) + try: + return evaluate_milestones_screenshot(task_config, screenshot_bytes) + except MilestoneEvaluationError: + # Deliberate: no measurement, so no override. The rollout keeps the + # reward the environment already gave it and the operator sees why. + logger.warning( + "Milestone evaluation could not run for task %r; keeping the " + "existing rollout reward instead of scoring it 0.0.", + rollout.task_id, + exc_info=True, + ) + return None def _make_agent_fn(self) -> Callable: """Create agent closure: observation -> BenchmarkAction. diff --git a/pyproject.toml b/pyproject.toml index c4217b4..bef2837 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -175,6 +175,12 @@ include = [ # a reviewable number and it guards code paths that pair model outputs with # ground truth, where a silent length mismatch is exactly the kind of quiet # wrong answer this project cares about. +# +# `PIE794` is added as a single rule, not the `PIE` family. The ruff 0.16 audit +# found `task_dir` declared twice in `GRPOConfig`; nothing in `B,E,F,I,W` +# catches a duplicated class field, so that defect could return the moment the +# audit stopped. The rest of `PIE` is style and is not adopted here. Repository +# count at the time of adoption: 0. ignore = [ # Line length is left to the formatter. `ruff format` already wraps what it # can; the residue is long string literals, URLs and comments that E501 @@ -182,7 +188,7 @@ ignore = [ # ignores E501 for this reason. "E501", ] -select = ["B", "E", "F", "I", "W"] +select = ["B", "E", "F", "I", "PIE794", "W"] [tool.semantic_release] version_toml = ["pyproject.toml:project.version"] diff --git a/scripts/setup_azure.py b/scripts/setup_azure.py index 4293747..4fab910 100644 --- a/scripts/setup_azure.py +++ b/scripts/setup_azure.py @@ -71,6 +71,10 @@ def check_az_logged_in() -> bool: """Check if user is logged into Azure CLI with valid token. We verify by actually making an API call, not just checking cached credentials. + + Returns True only for a call that succeeded, and False only for an error we + positively recognised as "not logged in". Any other CalledProcessError + propagates: this function reports what it checked, never what it guessed. """ try: # Try to list resource groups - this validates the token is still valid @@ -83,8 +87,11 @@ def check_az_logged_in() -> bool: # Check if it's an auth issue if "az login" in str(e.stderr).lower(): return False - # Other errors might be permission issues, but user is logged in - return True + # Anything else is an answer we do not have. Returning True here used + # to mean "I could not tell, so assume you are logged in", and the + # caller then skipped `az login` and failed further down with an + # unrelated error. Re-raise so the real failure is the one reported. + raise def get_subscriptions() -> list[dict]: diff --git a/tests/test_failure_is_not_success.py b/tests/test_failure_is_not_success.py new file mode 100644 index 0000000..90474c9 --- /dev/null +++ b/tests/test_failure_is_not_success.py @@ -0,0 +1,464 @@ +"""Regression tests for failures that used to be reported as successful results. + +Every test in this file guards one specific place where openadapt-ml could not +tell "I looked and the answer is empty/zero" apart from "I could not look", and +returned the first shape for the second case. In a package that produces +training rewards, grounding metrics and exported artifacts, that difference is +the difference between a number and a wrong number. + +Each test is mutation-checked against the pre-fix code: reverting the +production change makes the matching test fail. +""" + +from __future__ import annotations + +import ast +import sys +from dataclasses import fields +from pathlib import Path + +import pytest + + +def _error_type(module_path: str, name: str) -> type[BaseException]: + """Resolve an exception type by name, falling back to Exception. + + The tests below are mutation-checked by reverting the production file to its + pre-fix form. If they imported the new exception class directly, that revert + would make them fail with ImportError -- which proves only that a name was + removed. Resolving the type dynamically means the revert makes them fail + with "DID NOT RAISE", i.e. on the defect itself: a value was returned where + a failure should have been reported. + """ + import importlib + + module = importlib.import_module(module_path) + return getattr(module, name, Exception) + + +# --------------------------------------------------------------------------- +# 1. Duplicated dataclass field (`task_dir` declared twice in GRPOConfig) +# --------------------------------------------------------------------------- + +PACKAGE_ROOT = Path(__file__).resolve().parent.parent / "openadapt_ml" + + +def _duplicate_annotated_fields(tree: ast.AST) -> list[tuple[str, str, int]]: + """Return (class_name, field_name, lineno) for every re-declared field.""" + duplicates = [] + for node in ast.walk(tree): + if not isinstance(node, ast.ClassDef): + continue + seen: dict[str, int] = {} + for stmt in node.body: + if not isinstance(stmt, ast.AnnAssign): + continue + if not isinstance(stmt.target, ast.Name): + continue + name = stmt.target.id + if name in seen: + duplicates.append((node.name, name, stmt.lineno)) + else: + seen[name] = stmt.lineno + return duplicates + + +def test_grpo_config_declares_task_dir_exactly_once(): + """`task_dir` was declared twice in GRPOConfig (ruff PIE794). + + Both declarations spelled `str | None = None`, so the surviving default was + unchanged -- but a duplicate field is a live hazard: the *last* assignment + silently wins the default while the *first* keeps the field's position in + the generated __init__ signature, so the two halves of one field can drift + apart with nothing to flag it. The docstring documents exactly one + `task_dir`, and only the first declaration carried the explanatory comment + that matches it. + """ + source = (PACKAGE_ROOT / "training" / "grpo" / "config.py").read_text() + duplicates = _duplicate_annotated_fields(ast.parse(source)) + assert duplicates == [], ( + "GRPOConfig re-declares dataclass field(s): " + + ", ".join(f"{cls}.{name} at line {line}" for cls, name, line in duplicates) + ) + + +def test_no_dataclass_in_the_package_redeclares_a_field(): + """The same defect anywhere else in openadapt_ml/ is also a failure.""" + duplicates = [] + for path in sorted(PACKAGE_ROOT.rglob("*.py")): + tree = ast.parse(path.read_text()) + for cls, name, line in _duplicate_annotated_fields(tree): + duplicates.append(f"{path}:{line}: {cls}.{name}") + assert duplicates == [], "Re-declared class fields:\n" + "\n".join(duplicates) + + +def test_grpo_config_task_dir_keeps_its_documented_meaning(): + """The declaration that survived is the documented one.""" + from openadapt_ml.training.grpo.config import GRPOConfig + + task_dir_fields = [f for f in fields(GRPOConfig) if f.name == "task_dir"] + assert len(task_dir_fields) == 1 + assert task_dir_fields[0].default is None + assert GRPOConfig().task_dir is None + # The docstring is the contract the surviving declaration must match. + assert "task_dir: Path to a directory of YAML task config files" in ( + GRPOConfig.__doc__ or "" + ) + + +# --------------------------------------------------------------------------- +# 2. GRPO milestone reward: "could not evaluate" must not be reward 0.0 +# --------------------------------------------------------------------------- + + +class _Check: + def __init__(self, check="screenshot", description="a description"): + self.check = check + self.description = description + + +class _Milestone: + def __init__(self, name="m", check=None): + self.name = name + self.check = check if check is not None else _Check() + + +class _TaskConfig: + def __init__(self, milestones, task_id="task-1"): + self.milestones = milestones + self.id = task_id + + +def test_milestone_reward_raises_when_evals_package_is_missing(monkeypatch): + """Missing openadapt-evals used to log a warning and return reward 0.0. + + 0.0 is a legal reward. GRPO would have normalised it into an advantage and + taken a gradient step on a measurement that never happened. + """ + from openadapt_ml.training.grpo.reward import evaluate_milestones_screenshot + + MilestoneEvaluationError = _error_type( + "openadapt_ml.training.grpo.reward", "MilestoneEvaluationError" + ) + + monkeypatch.setitem(sys.modules, "openadapt_evals.vlm_evaluator", None) + config = _TaskConfig([_Milestone()]) + + with pytest.raises(MilestoneEvaluationError, match="openadapt-evals"): + evaluate_milestones_screenshot(config, b"png-bytes") + + +def test_milestone_reward_raises_when_the_vlm_judge_fails(monkeypatch): + """A judge exception used to be caught per-milestone and counted as a FAIL. + + The milestone stayed in the denominator, so an API timeout depressed the + reward by exactly as much as a genuinely unmet milestone. + """ + import types + + from openadapt_ml.training.grpo.reward import evaluate_milestones_screenshot + + MilestoneEvaluationError = _error_type( + "openadapt_ml.training.grpo.reward", "MilestoneEvaluationError" + ) + + def _boom(*args, **kwargs): + raise RuntimeError("429 rate limited") + + fake = types.ModuleType("openadapt_evals.vlm_evaluator") + fake.vlm_judge = _boom + monkeypatch.setitem(sys.modules, "openadapt_evals", types.ModuleType("oe")) + monkeypatch.setitem(sys.modules, "openadapt_evals.vlm_evaluator", fake) + + config = _TaskConfig([_Milestone("m1"), _Milestone("m2")]) + + with pytest.raises(MilestoneEvaluationError, match="not a failed milestone"): + evaluate_milestones_screenshot(config, b"png-bytes") + + +def test_milestone_reward_raises_on_a_milestone_with_no_description(monkeypatch): + """An undescribed milestone used to be `continue`d past. + + It stayed in `total`, so the task could never score 1.0 and nothing said so. + """ + import types + + from openadapt_ml.training.grpo.reward import evaluate_milestones_screenshot + + MilestoneEvaluationError = _error_type( + "openadapt_ml.training.grpo.reward", "MilestoneEvaluationError" + ) + + fake = types.ModuleType("openadapt_evals.vlm_evaluator") + fake.vlm_judge = lambda *a, **k: (True, 1.0) + monkeypatch.setitem(sys.modules, "openadapt_evals", types.ModuleType("oe")) + monkeypatch.setitem(sys.modules, "openadapt_evals.vlm_evaluator", fake) + + config = _TaskConfig( + [_Milestone("described"), _Milestone("blank", _Check(description=""))] + ) + + with pytest.raises(MilestoneEvaluationError, match="no description"): + evaluate_milestones_screenshot(config, b"png-bytes") + + +def test_milestone_reward_raises_when_there_is_nothing_locally_evaluable(): + """No milestones, or none of type 'screenshot', used to return 0.0.""" + from openadapt_ml.training.grpo.reward import evaluate_milestones_screenshot + + MilestoneEvaluationError = _error_type( + "openadapt_ml.training.grpo.reward", "MilestoneEvaluationError" + ) + + with pytest.raises(MilestoneEvaluationError, match="no milestones"): + evaluate_milestones_screenshot(_TaskConfig([]), b"png-bytes") + + server_only = _TaskConfig([_Milestone("m", _Check(check="http"))]) + with pytest.raises(MilestoneEvaluationError, match="none of type 'screenshot'"): + evaluate_milestones_screenshot(server_only, b"png-bytes") + + +def test_milestone_reward_still_returns_a_real_score(monkeypatch): + """The honest path is unchanged: a measured 0.5 is still 0.5.""" + import types + + from openadapt_ml.training.grpo.reward import evaluate_milestones_screenshot + + calls = [] + + def _judge(screenshot, description, **kwargs): + calls.append(description) + return (len(calls) == 1, 1.0) + + fake = types.ModuleType("openadapt_evals.vlm_evaluator") + fake.vlm_judge = _judge + monkeypatch.setitem(sys.modules, "openadapt_evals", types.ModuleType("oe")) + monkeypatch.setitem(sys.modules, "openadapt_evals.vlm_evaluator", fake) + + config = _TaskConfig([_Milestone("a"), _Milestone("b")]) + assert evaluate_milestones_screenshot(config, b"png-bytes") == 0.5 + assert len(calls) == 2 + + +# --------------------------------------------------------------------------- +# 3. Grounding: an API failure must not be reported as "no candidates matched" +# --------------------------------------------------------------------------- + + +def test_gemini_grounder_raises_instead_of_returning_no_candidates(monkeypatch): + """`ground()` used to print the error and return []. + + `evaluate_grounder` scores [] as best_iou 0.0 and centroid_hit False, so an + unreachable API was recorded as a grounding miss the model never made. + """ + from PIL import Image + + from openadapt_ml.grounding.detector import GeminiGrounder + + GroundingError = _error_type("openadapt_ml.grounding.base", "GroundingError") + + class _ExplodingModel: + def generate_content(self, *args, **kwargs): + raise RuntimeError("503 backend unavailable") + + grounder = GeminiGrounder(api_key="test-key") + monkeypatch.setattr(grounder, "_get_model", lambda: _ExplodingModel()) + + with pytest.raises(GroundingError, match="Gemini grounding call failed"): + grounder.ground(Image.new("RGB", (32, 32)), "the login button") + + +def test_gemini_grounder_raises_on_an_unreadable_response(monkeypatch): + """A reply with no JSON in it used to be indistinguishable from no match.""" + from PIL import Image + + from openadapt_ml.grounding.detector import GeminiGrounder + + GroundingError = _error_type("openadapt_ml.grounding.base", "GroundingError") + + class _Reply: + text = "I'm sorry, I can't help with that." + + class _ChattyModel: + def generate_content(self, *args, **kwargs): + return _Reply() + + grounder = GeminiGrounder(api_key="test-key") + monkeypatch.setattr(grounder, "_get_model", lambda: _ChattyModel()) + + with pytest.raises(GroundingError): + grounder.ground(Image.new("RGB", (32, 32)), "the login button") + + +def test_gemini_grounder_still_returns_an_empty_list_for_a_real_no_match( + monkeypatch, +): + """"I looked and matched nothing" must keep returning [].""" + from PIL import Image + + from openadapt_ml.grounding.detector import GeminiGrounder + + class _Reply: + text = "[]" + + class _EmptyModel: + def generate_content(self, *args, **kwargs): + return _Reply() + + grounder = GeminiGrounder(api_key="test-key") + monkeypatch.setattr(grounder, "_get_model", lambda: _EmptyModel()) + + assert grounder.ground(Image.new("RGB", (32, 32)), "the login button") == [] + + +def test_extract_ui_elements_raises_instead_of_claiming_an_empty_screen( + monkeypatch, +): + """Both handlers used to print and return [], i.e. "no elements on screen".""" + import types + + from PIL import Image + + from openadapt_ml.grounding.detector import extract_ui_elements + + GroundingError = _error_type("openadapt_ml.grounding.base", "GroundingError") + + class _ExplodingModel: + def generate_content(self, *args, **kwargs): + raise RuntimeError("quota exceeded") + + fake_genai = types.ModuleType("google.generativeai") + fake_genai.configure = lambda **kwargs: None + fake_genai.GenerativeModel = lambda *a, **k: _ExplodingModel() + fake_genai.GenerationConfig = lambda **kwargs: None + + # `import google.generativeai as genai` binds from the parent package's + # attribute when `google` is already imported, so patching sys.modules + # alone leaves the real client in place -- and the test would make a live + # API call. Patch both. + import google + + monkeypatch.setattr(google, "generativeai", fake_genai, raising=False) + monkeypatch.setitem(sys.modules, "google.generativeai", fake_genai) + + with pytest.raises(GroundingError, match="element extraction failed"): + extract_ui_elements(Image.new("RGB", (32, 32)), api_key="test-key") + + +# --------------------------------------------------------------------------- +# 4. Grounding metrics: an unreadable screenshot must not shrink the denominator +# --------------------------------------------------------------------------- + + +def test_evaluate_grounder_on_episode_refuses_an_unreadable_screenshot(tmp_path): + """A step whose screenshot cannot be opened used to be `continue`d past. + + Every GroundingMetrics property divides by len(results), so the sample + silently left the denominator and the reported hit rate stayed high. + """ + from openadapt_ml.evals.grounding import evaluate_grounder_on_episode + from openadapt_ml.grounding.base import GroundingModule + from openadapt_ml.schema import ( + Action, + ActionType, + BoundingBox, + Episode, + Observation, + Step, + UIElement, + ) + + class _NeverCalled(GroundingModule): + def ground(self, image, target_description, k=1): + raise AssertionError("grounder must not run on a broken episode") + + missing = tmp_path / "not-a-png.png" + missing.write_text("this is not an image") + + step = Step( + step_index=0, + observation=Observation(screenshot_path=str(missing)), + action=Action( + type=ActionType.CLICK, + element=UIElement( + bounds=BoundingBox(x=1, y=2, width=3, height=4), + ), + ), + ) + episode = Episode( + episode_id="ep-1", + instruction="click the login button", + steps=[step], + ) + + with pytest.raises(ValueError, match="could not be"): + evaluate_grounder_on_episode(_NeverCalled(), episode) + + +# --------------------------------------------------------------------------- +# 5. Parquet summary: "wrote nothing" must not look like "wrote the summary" +# --------------------------------------------------------------------------- + + +def test_write_summary_raises_when_pyarrow_is_missing(monkeypatch, tmp_path): + """`except ImportError: return` made include_summary=True a no-op. + + to_parquet() returned None either way, so the caller could not tell a + summary file that was written from one that never was. + """ + from openadapt_ml.export.parquet import _write_summary + + monkeypatch.setitem(sys.modules, "pyarrow", None) + monkeypatch.setitem(sys.modules, "pyarrow.parquet", None) + + with pytest.raises(ImportError, match="pyarrow"): + _write_summary([], str(tmp_path / "episodes.parquet")) + + +# --------------------------------------------------------------------------- +# 6. Azure login probe: "I could not tell" must not be reported as True +# --------------------------------------------------------------------------- + + +def test_check_az_logged_in_does_not_guess_true(monkeypatch): + """An unrecognised az error used to return True ("assume logged in").""" + import importlib.util + import subprocess + + spec = importlib.util.spec_from_file_location( + "_setup_azure_under_test", + Path(__file__).resolve().parent.parent / "scripts" / "setup_azure.py", + ) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + + def _fail(*args, **kwargs): + raise subprocess.CalledProcessError( + 1, "az", stderr="ERROR: the resource provider is not registered" + ) + + monkeypatch.setattr(module, "run_cmd", _fail) + + with pytest.raises(subprocess.CalledProcessError): + module.check_az_logged_in() + + +def test_check_az_logged_in_still_reports_false_for_a_known_auth_error(monkeypatch): + """The recognised "not logged in" answers are unchanged.""" + import importlib.util + import subprocess + + spec = importlib.util.spec_from_file_location( + "_setup_azure_under_test_2", + Path(__file__).resolve().parent.parent / "scripts" / "setup_azure.py", + ) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + + def _expired(*args, **kwargs): + raise subprocess.CalledProcessError( + 1, "az", stderr="AADSTS700082: the refresh token has expired" + ) + + monkeypatch.setattr(module, "run_cmd", _expired) + assert module.check_az_logged_in() is False