feat(evaluator): add ATIF trace and log read handles for evidence - #432
Conversation
|
55ce862 to
15882d4
Compare
948dc8a to
9c8325f
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Pydantic ATIF data models ( ChangesATIF trace evidence and retry metrics
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/trials.py (1)
161-168: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftThis strips source format for file-based traces.
standard_evidence_descriptors()can now only emitatiforjson, so OTEL/OpenInference exports passed viatrace_pathcan never hit their normalizers on this path. This needs an explicit trace-format input instead of filename heuristics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/trials.py` around lines 161 - 168, The trace descriptor normalization is using filename heuristics in standard_evidence_descriptors() / the trace_path handling, which causes file-based OTEL/OpenInference traces to lose their original format. Update the evidence-building flow to accept an explicit trace-format input and pass that through when constructing the EvidenceDescriptor for EVIDENCE_TRACE, instead of inferring “atif” vs “json” from is_atif or the path name. Ensure normalize_trace_descriptor and the caller in trials.py preserve the source format so the correct normalizer can run.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/nemo_evaluator_sdk/examples/run_agent_eval/example_metrics.py`:
- Around line 19-21: The retry detection in the example metrics logic is
counting global frequency instead of consecutive repeats, so update the relevant
code in example_metrics.py to measure the longest streak of identical calls in
the trace rather than total occurrences. In the call-counting path around the
metrics computation (the logic that uses EVIDENCE_TRACE), canonicalize nested
args before comparing so semantically equivalent dicts with different insertion
order map to the same payload, and keep the streak bounded to adjacent entries
only. Use the existing symbols EVIDENCE_TRACE, EVIDENCE_INITIAL_STATE, and
EVIDENCE_FINAL_STATE to locate the trace-processing code and adjust the retry
metric accordingly.
In `@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/evidence.py`:
- Around line 490-510: normalize_trace() currently drops any list-shaped payload
unless source_format is explicitly otel/openinference, which can erase live
evaluator trajectory evidence. Update normalize_trace() in evidence.py to
preserve unknown list traces by either routing them through the appropriate
list-based normalizer or explicitly validating and rejecting unsupported list
shapes instead of returning an empty AtifTrace(). Keep the existing dict
handling for steps/events and ensure the fallback path for bare lists is handled
safely rather than silently discarding data.
- Around line 534-541: The ref-handling in `_local_filesystem_ref()` / the
`descriptor.ref is not None` branch currently raises for remote refs before the
`is_file()` no-op path can run, which conflicts with the documented behavior.
Update the logic so non-local or unresolvable refs are detected and returned
unchanged in `descriptor.model_copy` flow, and only call
`_local_filesystem_ref()` after confirming the ref is local or otherwise safe to
resolve. Keep the existing `normalize_trace` and `.atif.json` write path for
local files, but preserve the unchanged descriptor behavior for remote refs.
- Around line 671-682: The `Evidence.trace()` accessor is currently using
`self.require(name)` without verifying the descriptor kind, so a non-trace entry
can be wrapped as a `TraceHandle` and look valid. Update `trace()` to enforce
`kind="trace"` before constructing the handle, matching the stricter contract
already implied by `TraceHandle` and the existing `filesystem()`/`logs()`
accessors. Keep the cache behavior in `_trace_cache`, but ensure only true trace
descriptors are accepted and anything else fails immediately.
In `@packages/nemo_evaluator_sdk/tests/agent_eval/test_evidence.py`:
- Around line 5-7: The process cleanup assertion in test_evidence.py is using a
fixed 200 ms wait, which can make the test flaky if the child is briefly a
zombie on a busy runner. Update the test around the cleanup check to poll
`os.kill(pid, 0)` until a short deadline instead of sleeping once, so the
assertion waits for reaping to complete. Use the existing process-tree
cleanup/test logic in `test_evidence` to locate the affected check and keep the
polling window small.
---
Duplicate comments:
In `@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/trials.py`:
- Around line 161-168: The trace descriptor normalization is using filename
heuristics in standard_evidence_descriptors() / the trace_path handling, which
causes file-based OTEL/OpenInference traces to lose their original format.
Update the evidence-building flow to accept an explicit trace-format input and
pass that through when constructing the EvidenceDescriptor for EVIDENCE_TRACE,
instead of inferring “atif” vs “json” from is_atif or the path name. Ensure
normalize_trace_descriptor and the caller in trials.py preserve the source
format so the correct normalizer can run.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 739e49cd-e370-4b40-991d-c3b8b5189fc5
⛔ Files ignored due to path filters (4)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/evaluator.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/trials.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/values/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/values/evidence.pyis excluded by!sdk/**
📒 Files selected for processing (7)
packages/nemo_evaluator_sdk/examples/run_agent_eval/example_metrics.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/evaluator.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/trials.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/__init__.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/evidence.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_evidence.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_example_metrics.py
9c8325f to
2da7a67
Compare
2da7a67 to
8975314
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/evidence.py`:
- Around line 520-522: Reject non-local trace refs before caching the
TraceHandle in the trace lookup flow, so self.require(name, kind="trace") cannot
store a handle that will later fail in TraceHandle._load_payload() via
_local_filesystem_ref(). Update the relevant trace-loading path and any shared
helper used by the trace cache (including the related logic around
normalize_trace_descriptor() and the affected duplicate block) to validate the
ref is local first, and raise a trace-level contract error instead of letting a
filesystem ValueError surface on first read.
- Around line 368-380: The trace validation logic in the descriptor
normalization path is skipping any descriptor already stamped with
format="atif", which allows malformed inline or local ATIF payloads to bypass
validation. Update the validation branch in evidence.py so resolvable ATIF
descriptors (including those with descriptor.format already set to "atif" and a
local ref or inline data) still flow through
parse_atif(_read_trace_payload(descriptor)) before returning, while preserving
the existing early return for non-trace descriptors and non-local refs. Keep the
behavior in the same validation helper that currently uses
_local_filesystem_ref, _read_trace_payload, and parse_atif so persist-time
validation remains enforced consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e6403cd5-6d4a-45fc-8998-122585081f5e
⛔ Files ignored due to path filters (5)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/evaluator.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/trials.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/values/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/values/atif.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/values/evidence.pyis excluded by!sdk/**
📒 Files selected for processing (10)
packages/nemo_evaluator_sdk/examples/run_agent_eval/example_metrics.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/evaluator.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/trials.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/__init__.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/atif.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/evidence.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_evaluator.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_evidence.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_example_metrics.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_trials.py
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/init.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/trials.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/evaluator.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/evaluator.py`:
- Line 47: The evaluator is still persisting sample["trajectory"] with
format="atif" even though it is no longer normalized, which can break later
parsing in TraceHandle.trace(). Update the logic around
EvidenceDescriptor/CandidateEvidence creation to either normalize the trajectory
before saving or validate its shape and reject non-ATIF payloads; do not label
raw NAT/OpenTelemetry/OpenInference traces as ATIF unless they are actually
converted to that contract.
In `@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/trials.py`:
- Line 17: The trace-path handling in trials.py is hard-coding format="atif" for
every trace file, which bypasses the existing normalization/validation flow.
Update the trace-path helper that feeds TraceHandle to use the normalizer again,
and only preserve ATIF after validating the trace content rather than stamping
all inputs as ATIF. Make the fix in the trace_path-related logic and the related
import/use site so non-ATIF JSON traces are not persisted with the wrong
contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4c1f389a-2329-494d-abd7-212ca86c1fc4
⛔ Files ignored due to path filters (4)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/evaluator.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/trials.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/values/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/values/evidence.pyis excluded by!sdk/**
📒 Files selected for processing (5)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/evaluator.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/trials.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/__init__.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/evidence.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_evidence.py
💤 Files with no reviewable changes (3)
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/init.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_evidence.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/evidence.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/evaluator.py`:
- Line 47: The evaluator is still persisting sample["trajectory"] with
format="atif" even though it is no longer normalized, which can break later
parsing in TraceHandle.trace(). Update the logic around
EvidenceDescriptor/CandidateEvidence creation to either normalize the trajectory
before saving or validate its shape and reject non-ATIF payloads; do not label
raw NAT/OpenTelemetry/OpenInference traces as ATIF unless they are actually
converted to that contract.
In `@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/trials.py`:
- Line 17: The trace-path handling in trials.py is hard-coding format="atif" for
every trace file, which bypasses the existing normalization/validation flow.
Update the trace-path helper that feeds TraceHandle to use the normalizer again,
and only preserve ATIF after validating the trace content rather than stamping
all inputs as ATIF. Make the fix in the trace_path-related logic and the related
import/use site so non-ATIF JSON traces are not persisted with the wrong
contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4c1f389a-2329-494d-abd7-212ca86c1fc4
⛔ Files ignored due to path filters (4)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/evaluator.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/trials.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/values/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/values/evidence.pyis excluded by!sdk/**
📒 Files selected for processing (5)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/evaluator.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/trials.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/__init__.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/evidence.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_evidence.py
💤 Files with no reviewable changes (3)
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/init.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_evidence.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/evidence.py
🛑 Comments failed to post (2)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/evaluator.py (1)
47-47: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Don't relabel unnormalized inline traces as ATIF.
This path stopped normalizing
sample["trajectory"]but still persists it withformat="atif". Any NAT / OpenTelemetry / OpenInference payload will now be stored under the ATIF contract and fail later whenTraceHandle.trace()parses it. Normalize or validate before writing the descriptor.Proposed fix
-from nemo_evaluator_sdk.values.evidence import CandidateEvidence, EvidenceDescriptor +from nemo_evaluator_sdk.values.evidence import ( + CandidateEvidence, + EvidenceDescriptor, + normalize_trace_descriptor, +) ... - # Persist the producer's trajectory as-is (ATIF); TraceHandle validates on read. - trace = EvidenceDescriptor(kind="trace", format="atif", data=sample["trajectory"]) + trace = normalize_trace_descriptor( + EvidenceDescriptor(kind="trace", data=sample["trajectory"]) + )Also applies to: 329-331
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/evaluator.py` at line 47, The evaluator is still persisting sample["trajectory"] with format="atif" even though it is no longer normalized, which can break later parsing in TraceHandle.trace(). Update the logic around EvidenceDescriptor/CandidateEvidence creation to either normalize the trajectory before saving or validate its shape and reject non-ATIF payloads; do not label raw NAT/OpenTelemetry/OpenInference traces as ATIF unless they are actually converted to that contract.packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/trials.py (1)
17-17: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Don't stamp every
trace_pathas ATIF.
TraceHandleonly reads ATIF. This helper now setsformat="atif"for any trace file without normalizing or validating it first, so non-ATIF JSON traces will be persisted with the wrong contract and blow up when metrics read them. Restore the normalizer here instead of hard-coding the format.Proposed fix
-from nemo_evaluator_sdk.values.evidence import CandidateEvidence, EvidenceDescriptor +from nemo_evaluator_sdk.values.evidence import ( + CandidateEvidence, + EvidenceDescriptor, + normalize_trace_descriptor, +) ... - # Persist the producer-written trace as-is (ATIF); TraceHandle validates on read. - descriptors[EVIDENCE_TRACE] = EvidenceDescriptor( - kind="trace", - format="atif", - ref=str(trace_path), - ) + descriptors[EVIDENCE_TRACE] = normalize_trace_descriptor( + EvidenceDescriptor(kind="trace", ref=str(trace_path)) + )Also applies to: 159-163
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/trials.py` at line 17, The trace-path handling in trials.py is hard-coding format="atif" for every trace file, which bypasses the existing normalization/validation flow. Update the trace-path helper that feeds TraceHandle to use the normalizer again, and only preserve ATIF after validating the trace content rather than stamping all inputs as ATIF. Make the fix in the trace_path-related logic and the related import/use site so non-ATIF JSON traces are not persisted with the wrong contract.
599051d to
5998f9f
Compare
Add read handles over candidate evidence for agent-eval metrics: - TraceHandle exposes a trace descriptor as an ATIF Trajectory with step/tool-call/token-usage views; lightweight ATIF read models live in values/atif.py (validated on read via parse_atif, raw payload persisted, no normalization — producers emit conformant ATIF). - LogHandle for log-bundle access and LocalFilesystemEvidence helpers (diff, unified_diff, run_verifier with symlink hardening). - WellKnownEvidenceKey literal and example metrics updates. Vendored mirror synced via make vendor. Signed-off-by: Arpit Singh (SW-CLOUD) <arpsingh@nvidia.com>
5998f9f to
c94721e
Compare
- Restore the symlink-safety comment in run_verifier explaining why copytree(symlinks=True) is safe (the ignore hook drops escaping links). - Reference AgentEvalTrial (not AgentEvalAttempt) in the CandidateEvidence docstring. Vendored mirror synced via make vendor. Signed-off-by: Arpit Singh (SW-CLOUD) <arpsingh@nvidia.com>
c94721e to
fcd211d
Compare
Summary
Adds the portable trace/log read layer for agent-eval evidence so metrics can score off what the agent actually did (its trace and logs), independent of the producing runtime. Rebased onto
mainafter the filesystem handle /run_verifier/ taskset work landed via #447, so this PR is now scoped to trace and log read handles only.This PR ingests traces in ATIF (Agent Trajectory Interchange Format; RFC 0001). It does not normalize foreign formats — producers are responsible for emitting conformant ATIF, and the SDK validates on read and exposes typed handles.
values/atif.py):Trajectory/Step/ToolCall/Metrics/FinalMetrics— a permissive (extra="ignore") read view over just the subset of ATIF the evaluator consumes, rather than vendoring the full reference schema (avoids dependency/licensing bloat and stays forward-compatible).values/evidence.py):parse_atifvalidates a payload as aTrajectory(raisesValidationErrorif non-conformant). Trace evidence is persisted in its raw producer form; nothing is normalized at persist time.CandidateEvidence.trace()→TraceHandle(trace/steps/tool_calls/token_usage) and.logs()→LogHandle(list_files/read_text/tail), bothasyncand per-trial cached so sibling metrics share a materialized handle.WellKnownEvidenceKeyLiteral (initial_state/trace/logs/final_state/verifier_logs).examples/run_agent_eval/example_metrics.py, example-only):inefficient_retry_loopscoring offTraceHandle(consecutive-streak detection over a canonical tool-call key).make vendor.Test plan
ruff check+ruff format --checkclean on changed filesty check— no new diagnostics in changed filespytest packages/nemo_evaluator_sdk/tests/agent_eval/→ 67 passed (trace/log handle reads, ATIF validation, example retry-loop metric)make vendorregenerated mirror, no driftNotes
values/evidence.py+values/atif.py(no separate value module) per review preference.TraceHandleon first read.agent_eval/evaluator.pyandagent_eval/trials.pyare intentionally not modified by this PR — trace evidence is persisted as-is and read viaTraceHandle.