diff --git a/docs/configuration.md b/docs/configuration.md index be1de3b9..4831cc4f 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -86,9 +86,11 @@ Defaults only apply to the **build** command where Dockerfile selection requires ## Run phase: log error pattern scan -After a successful container run, madengine may scan the **run log file** for fixed substrings (for example `RuntimeError:`, `OutOfMemoryError`, `Traceback (most recent call last)`). If a match is found, the run can be marked `FAILURE` even when performance metrics exist—intended as a safety net when logs show obvious Python or OOM errors. +After a successful container run, madengine may scan the **run log file** for fixed substrings (for example `RuntimeError:`, `OutOfMemoryError`, `Traceback (most recent call last)`)—intended as a safety net when logs show obvious Python or OOM errors. If a match is found **and no valid performance metrics were extracted**, the run is marked `FAILURE`. -Some suites (for example layer unit tests) intentionally print benign `RuntimeError:` text while pytest still passes. In those cases you can **disable** the scan or **narrow** what counts as an error. +If valid performance metrics *were* extracted, a pattern match no longer fails the run: the log scan cannot distinguish madengine/framework diagnostics from a model's own generated stdout, so a generative model whose output happens to contain a banned substring (e.g. an LLM writing `"ValueError:"` in a code sample) no longer produces a false `FAILURE` (see ROCM-27774). The match is still printed (in yellow) for triage visibility. + +Some suites (for example layer unit tests) intentionally print benign `RuntimeError:` text while pytest still passes, with no performance metrics to fall back on. In those cases you can **disable** the scan or **narrow** what counts as an error. Keys can be set in `--additional-context` / `--additional-context-file`, or on the **model** entry in `models.json` (same keys). **Runtime context overrides the model** when both are set. diff --git a/src/madengine/execution/container_runner.py b/src/madengine/execution/container_runner.py index 625188f3..eab4af7d 100644 --- a/src/madengine/execution/container_runner.py +++ b/src/madengine/execution/container_runner.py @@ -45,6 +45,7 @@ log_text_has_error_pattern, make_run_log_file_path, resolve_log_error_scan_config, + resolve_run_status, resolve_run_timeout, ) @@ -1862,11 +1863,13 @@ def run_container( "(log_error_pattern_scan).[/dim]" ) - # Status logic: Must have performance AND no errors to be considered success + # Status logic: valid performance metrics take priority over a log + # error-pattern match, since the scan cannot tell framework/harness + # diagnostics apart from a model's own generated stdout (ROCM-27774). # Exception: Worker nodes in multi-node training (MAD_COLLECT_METRICS=false) # are not expected to report global performance metrics performance_value = run_results.get("performance") - has_performance = ( + has_performance = bool( performance_value and performance_value.strip() and performance_value.strip() != "N/A" @@ -1875,36 +1878,39 @@ def run_container( # Check if this is a worker node (not collecting metrics) is_worker_node = os.environ.get("MAD_COLLECT_METRICS", "true").lower() == "false" - if has_errors: - run_results["status"] = "FAILURE" - self.rich_console.print( - f"[red]Status: FAILURE (error patterns detected in logs)[/red]" - ) - elif has_performance: - run_results["status"] = "SUCCESS" + # Multi-node/SLURM in-job run: the login node aggregates the richest + # per-node multiple_results CSV and writes the authoritative perf/status + # record (slurm.collect_results + _select_best_multiple_results_csv). + # Primus emits throughput only on the last global rank, which may land on a + # different node than the designated collector (MAD_COLLECT_METRICS=true), + # so an empty local perf here is not authoritative and must not fail the job. + skip_perf_collection = bool( + self.additional_context.get("skip_perf_collection", False) + ) + + status, status_reason = resolve_run_status( + has_performance=has_performance, + has_errors=has_errors, + is_worker_node=is_worker_node, + skip_perf_collection=skip_perf_collection, + ) + run_results["status"] = status + + if status == "FAILURE": self.rich_console.print( - f"[green]Status: SUCCESS (performance metrics found, no errors)[/green]" + f"[red]Status: FAILURE ({status_reason})[/red]" ) - elif is_worker_node: - # Worker nodes don't report global performance metrics - this is expected - run_results["status"] = "SUCCESS" + elif has_errors: + # SUCCESS despite a log error-pattern match: performance metrics + # were valid, so the match is likely benign model-generated text. + # Surfaced in yellow (rather than plain green) for triage visibility. self.rich_console.print( - f"[green]Status: SUCCESS (worker node, no errors detected)[/green]" + f"[yellow]Status: SUCCESS ({status_reason})[/yellow]" ) - elif self.additional_context.get("skip_perf_collection", False): - # Multi-node/SLURM in-job run: the login node aggregates the richest - # per-node multiple_results CSV and writes the authoritative perf/status - # record (slurm.collect_results + _select_best_multiple_results_csv). - # Primus emits throughput only on the last global rank, which may land on a - # different node than the designated collector (MAD_COLLECT_METRICS=true), - # so an empty local perf here is not authoritative and must not fail the job. - run_results["status"] = "SUCCESS" + else: self.rich_console.print( - f"[green]Status: SUCCESS (perf collection deferred to login-node aggregation)[/green]" + f"[green]Status: SUCCESS ({status_reason})[/green]" ) - else: - run_results["status"] = "FAILURE" - self.rich_console.print(f"[red]Status: FAILURE (no performance metrics)[/red]") except Exception as e: self.rich_console.print(f"[yellow]Warning: Error in status determination: {e}[/yellow]") diff --git a/src/madengine/execution/container_runner_helpers.py b/src/madengine/execution/container_runner_helpers.py index dfa99be5..120d3144 100644 --- a/src/madengine/execution/container_runner_helpers.py +++ b/src/madengine/execution/container_runner_helpers.py @@ -149,6 +149,57 @@ def log_text_has_error_pattern( return False +def resolve_run_status( + has_performance: bool, + has_errors: bool, + is_worker_node: bool = False, + skip_perf_collection: bool = False, +) -> typing.Tuple[str, str]: + """ + Decide the final run status ("SUCCESS"/"FAILURE") and a short human-readable reason. + + Priority (see ROCM-27774): + + 1. Valid extracted performance metrics are the strongest evidence a run actually + completed successfully. A post-hoc log error-pattern match cannot distinguish + madengine/framework diagnostics from a model's own generated stdout (e.g. an LLM + benchmark whose response text contains ``"ValueError:"``), so it must not override + a run that already produced valid performance data. The match is still reported so + it remains visible for triage, without failing an otherwise-successful run. + 2. Otherwise, a matched error pattern fails the run (no performance data to + contradict it). + 3. Otherwise, worker nodes / deferred perf-collection runs are expected to have no + local performance and are not failed for that reason. + 4. Otherwise, no performance metrics and no exemption applies -> FAILURE. + + Args: + has_performance: Whether valid performance metrics were extracted from the log. + has_errors: Whether a configured error pattern was matched in the log. + is_worker_node: Whether this is a non-collecting worker node + (``MAD_COLLECT_METRICS=false``) in multi-node training. + skip_perf_collection: Whether local perf collection is deferred to a login-node + aggregator (e.g. multi-node SLURM runs). + + Returns: + (status, reason) tuple, e.g. ``("SUCCESS", "performance metrics found, no errors")``. + """ + if has_performance: + if has_errors: + return ( + "SUCCESS", + "performance metrics found; error pattern also matched in logs " + "(likely model-generated output, not treated as failure)", + ) + return "SUCCESS", "performance metrics found, no errors" + if has_errors: + return "FAILURE", "error patterns detected in logs" + if is_worker_node: + return "SUCCESS", "worker node, no errors detected" + if skip_perf_collection: + return "SUCCESS", "perf collection deferred to login-node aggregation" + return "FAILURE", "no performance metrics" + + def resolve_run_timeout( model_info: typing.Dict, cli_timeout: int, diff --git a/tests/unit/test_container_runner_helpers.py b/tests/unit/test_container_runner_helpers.py index a4d96539..0ff617e2 100644 --- a/tests/unit/test_container_runner_helpers.py +++ b/tests/unit/test_container_runner_helpers.py @@ -6,6 +6,7 @@ DEFAULT_LOG_ERROR_PATTERNS, log_text_has_error_pattern, resolve_log_error_scan_config, + resolve_run_status, ) @@ -130,3 +131,50 @@ def test_user_benign_literal_parentheses(self): ["(benign marker)"], (), ) + + +class TestResolveRunStatus: + """ROCM-27774: valid performance metrics must win over a log error-pattern match, + since the scan cannot tell framework diagnostics apart from a model's own + generated stdout (e.g. an LLM benchmark response containing "ValueError:").""" + + def test_performance_and_no_errors_is_success(self): + status, reason = resolve_run_status(has_performance=True, has_errors=False) + assert status == "SUCCESS" + assert "no errors" in reason + + def test_performance_with_errors_is_still_success(self): + status, reason = resolve_run_status(has_performance=True, has_errors=True) + assert status == "SUCCESS" + assert "error pattern also matched" in reason + + def test_errors_without_performance_is_failure(self): + status, reason = resolve_run_status(has_performance=False, has_errors=True) + assert status == "FAILURE" + assert "error patterns detected" in reason + + def test_worker_node_without_performance_is_success(self): + status, reason = resolve_run_status( + has_performance=False, has_errors=False, is_worker_node=True + ) + assert status == "SUCCESS" + assert "worker node" in reason + + def test_worker_node_with_errors_is_still_failure(self): + # Worker-node exemption only covers missing performance data, not error matches. + status, reason = resolve_run_status( + has_performance=False, has_errors=True, is_worker_node=True + ) + assert status == "FAILURE" + + def test_skip_perf_collection_without_performance_is_success(self): + status, reason = resolve_run_status( + has_performance=False, has_errors=False, skip_perf_collection=True + ) + assert status == "SUCCESS" + assert "deferred" in reason + + def test_no_performance_no_errors_no_exemption_is_failure(self): + status, reason = resolve_run_status(has_performance=False, has_errors=False) + assert status == "FAILURE" + assert "no performance metrics" in reason