feat: add stable evaluator error codes - #115
Conversation
Signed-off-by: Mohit Gupta <mohgupta@nvidia.com>
Signed-off-by: Mohit Gupta <mohgupta@nvidia.com>
Signed-off-by: Mohit Gupta <mohgupta@nvidia.com>
chrisknvidia
left a comment
chrisknvidia
left a comment
There was a problem hiding this comment.
Reviewed exact head 1a98dfce and a clean synthetic merge with current main (14a888e). The focused merged suite passes, but real CLI, Botocore credential-process, Bedrock classification, and result-resolution reproductions confirm four P1 blockers and three P2 contract defects below. No paid provider credentials were required for these reproductions.
| """Build the common serializable shape for an evaluator execution failure.""" | ||
| return { | ||
| "execution_status": "failed", | ||
| "error_code": validate_error_code(error_code), |
There was a problem hiding this comment.
[P1] Propagate terminal codes through every documented public output
This adds error_code to the engine return dictionary, but the public command boundary, terminal renderer, combined Tier 3 adapter, and canonical JSON/HTML payloads do not carry it. On exact head, an in-engine configuration failure exited 1 in standalone mode with no code and no result.json; advisory and blocking validate --tier3 JSON also contained no error_code. Please thread the typed code through those consumers and provide a machine-readable artifact/output for failures before run reservation.
| "agents": {}, | ||
| "run_config": run_config, | ||
| } | ||
| failed_result = _terminal_failure_result(errors, error_code=error_code) |
There was a problem hiding this comment.
[P1] Make persisted pre-execution failures valid current results
This failure payload still writes agents: {} and omits attempt_policy and report_status, while _current_result_identity_is_valid requires configured/result agent equality and both fields. The early return also bypasses publish_latest_results. In a direct runner reproduction, the failed run's completion key was None; symlinkless lookup and even a forced latest link to the failure resolved to an older successful run. Please define a valid failure schema, publish it as latest, and add a regression with an existing older success.
| and _bedrock_credential_process(session) is not None | ||
| ): | ||
| failure_kind = ModelCatalogFailureKind.UNAVAILABLE | ||
| failure_kind = ModelCatalogFailureKind.LOCAL_PROCESS |
There was a problem hiding this comment.
[P1] Supervise and classify the real AWS credential helper
Only an injected transient _popen OSError reaches this LOCAL_PROCESS branch. Real Botocore paths produced: missing helper -> CONFIG-001 (expected RUNTIME-005), nonzero helper -> DEPENDENCY-001 (expected RUNTIME-008), and hanging helper -> DEPENDENCY-005 (expected RUNTIME-007) while the helper and bedrock-model-catalog-probe worker remained alive. Please execute/supervise the helper in a killable process group, terminate and reap it on deadline, and preserve spawn/timeout/exit signals explicitly.
There was a problem hiding this comment.
i need to check on this one. thanks for bringing this point.
| if self.error_code is not None or timed_out: | ||
| raise ValueError("successful model probe cannot carry failure metadata") | ||
| return | ||
| expected = provider_failure_error_code( |
There was a problem hiding this comment.
[P1] Preserve Bedrock subtypes before deriving the stable code
The Bedrock ClientError path collapses AWS error subtypes before this mapping and never marks structured service timeouts as timed_out. Direct Botocore cases yielded ThrottlingException HTTP 400 -> DEPENDENCY-001, RequestTimeout -> UNKNOWN-001/DEPENDENCY-001, and an arbitrary HTTP 418 -> UNKNOWN-001; these should be DEPENDENCY-006, DEPENDENCY-005, and DEPENDENCY-009. Preserve the AWS timeout/rate-limit/other-HTTP subtype until the final code is derived.
| timed_out: InitVar[bool] = False | ||
|
|
||
| def __post_init__(self, timed_out: bool) -> None: | ||
| if self.ok: |
There was a problem hiding this comment.
[P2] Reject all failure metadata on successful probes
The success branch rejects only error_code and timed_out; ModelProbeResult(ok=True, failure_kind="authentication", http_status=401) is accepted and can subsequently be treated as verified. Please also require failure_kind is None and http_status is None when ok is true, with a regression covering contradictory success metadata.
There was a problem hiding this comment.
ack thanks. Good catch
|
|
||
| def primary_error_code(codes: Iterable[object]) -> str: | ||
| """Return a consensus code, or UNKNOWN when failures conflict or are absent.""" | ||
| registered = {str(code) for code in codes if is_registered_error_code(code)} |
There was a problem hiding this comment.
[P2] Treat absent or invalid classifications as failed consensus
Filtering invalid entries means both [AUTH-002, None] and [AUTH-002, "invalid"] return AUTH-002. That contradicts the documented contract that conflicting or absent classifications produce UNKNOWN-001. Materialize the supplied votes and return unknown if any supplied classification is absent or unregistered; otherwise require all codes to agree.
| ): | ||
| return False | ||
| attempt_policy = result.get("attempt_policy") | ||
| error_code = result.get("error_code") |
There was a problem hiding this comment.
[P2] Distinguish an omitted code from explicit JSON null
Using .get() makes a missing backward-compatible field indistinguishable from "error_code": null. A succeeded current result containing explicit null was accepted as authentic even though the public schema requires a string whenever the field is present. Check key presence separately, then require a registered string and failed execution status when present.
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
Reviewed the current draft head. The seven existing review threads remain reproducible and blocking: four affect public failure-code propagation, persisted failure validity, AWS helper supervision, and Bedrock classification; three affect contradictory probe metadata, consensus handling, and explicit null codes. I did not add duplicate inline comments. Focused suites passed 635 tests, with the remaining failures limited to sandbox-denied localhost binds; Ruff and diff checks are clean. Please address the open threads before re-review.
|
@mohgupta-ship-it : I see this PR is still in draft, just to make sure if this PR ready for further reviews ? |
3 participants
Prepared by Codex.
Summary
Validation