fix: stop reporting evaluation and grounding failures as empty results

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

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:

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",

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
 

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,15 +102,25 @@ 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 = []
 
         # Try to parse JSON from the 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(

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",

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,57 +102,79 @@ 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,
                 description,
                 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,