Repository navigation
Conversation
Signed-off-by: Frederic Lafrance <flafrance@nvidia.com>
Signed-off-by: Frederic Lafrance <flafrance@nvidia.com>
Signed-off-by: Frederic Lafrance <flafrance@nvidia.com>
Signed-off-by: Frederic Lafrance <flafrance@nvidia.com>
…ions Signed-off-by: Frederic Lafrance <flafrance@nvidia.com>
|
/review |
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
SHIP WITH CARE
Three RISK findings remain: conflicting tool/turn ownership can pass validation, and two v2 failure paths escape the exporter's error contract.
I reviewed all three changed files and their directly affected producer, projection, CLI/batch, and reverification paths. The strict-v1 refactor preserves its existing checks. This review used source inspection only; I couldn't execute tests.
Review generated by codex. This was a full review. Request a full review with /review reset=true.
| path = f"ng_trajectory.turns[{index}]" | ||
| if call_index in used_calls: | ||
| raise _path_error(path, "captured model call belongs to more than one exported step") | ||
| if call_ids and any( |
There was a problem hiding this comment.
RISK — Compare tool instances with the selected native turn
Could this association also reject contradictory tool-instance evidence? Starting with _v2_rollout(), leave the root and model-call instances null, set the first native turn's instance to B, and set both tools' instances to A. The decoder accepts this: it checks tools against the requesting model, whose instance is unknown. The exporter checks root/turn/model instances but never tools against the selected turn, so this conflicting v2 input exports successfully instead of rejecting known ownership disagreement.
Correction: after the requesting-model check, add this group-local validation in _apply_conversation_turns:
known_instances = {
invocation.agent_instance,
turn.agent_instance,
call.agent_instance,
*(tools[identity].agent_instance for identity in call_ids),
} - {None}
if len(known_instances) > 1:
raise _path_error(path, "native turn, captured call and tools have conflicting agent instance ownership")The invariant is that all known owners within one exported step agree. Null must remain unknown; don't require a globally populated root instance or merge owners across separate steps.
Proposed acceptance checks: the A/B example must raise AtifExportError; two tools with different known instances and otherwise null owners must also reject. Matching tool/turn instances with null root/model instances must still export, as must wholly unavailable instance evidence. Retain the nullable-owner success assertion that original tool records survive unchanged, and the text-only pair-reference assertions that a missing model-call ID remains supported. Static reasoning only; verify the suggested code before adopting it.
| projected["gaps"] = [gap for gap in projected.get("gaps", []) if gap["code"] not in _V2_TIMING_PROVENANCE_GAPS] | ||
| exported = _export_trajectory( | ||
| rollout, | ||
| TrajectoryRecord.model_validate(projected, strict=True), |
There was a problem hiding this comment.
RISK — Translate projected-record validation failures into export errors
Projection can introduce validation failures that weren't present in the source, but this validation is outside the exception wrapper. Starting with _v2_rollout(), leave the root instance null, give the two turns instances A/B and the same turn_no, give their respective model calls matching instances, and give both tools instance A. The v2 identity contract permits instance-qualified turn numbers. The projection removes those instances, creating duplicate v1 turn identities and a raw ValidationError here.
The batch handler catches only AtifExportError, so this aborts validation without row-numbered aggregate diagnostics. Cleanup still prevents publication; the defect is the public error boundary, not partial output.
Correction: validate separately immediately before _export_trajectory:
try:
projected_trajectory = TrajectoryRecord.model_validate(projected, strict=True)
except (TypeError, ValidationError) as exc:
raise _path_error(
"ng_trajectory", "v2.0 trajectory cannot be represented by the v1 compatibility projection"
) from excReplace the validation expression at this line with projected_trajectory. This preserves rejection of unrepresentable projections without blanket-catching unrelated runtime failures.
Proposed acceptance checks: the example must raise AtifExportError, not raw ValidationError. Put two such rows with distinct Gym rollout keys after a valid row: expect rejected 2 row(s), both failing line numbers, no output directory, and no staging directory. Retain the same aggregation/cleanup assertions in the invalid-container regression, plus successful nullable-owner exports and the existing exact scalar/extension assertions for partial, empty, and multiple timing boundaries. Static reasoning only; verify the suggested code before adopting it.
| calls: set[str] = set() | ||
| messages: set[str] = set() | ||
| for item in answer: | ||
| if not isinstance(item, dict) or item.get("type") not in {"function_call", "message"}: |
There was a problem hiding this comment.
RISK — Avoid hashing opaque native item types
TrajectoryTurn.answer intentionally accepts opaque JSON. Starting with _native_v2_rollout(), set the final turn's answer[0]["type"] to [] or {}. Source validation accepts it, but this set-membership expression raises TypeError: unhashable type before the missing-anchor check can produce AtifExportError. The row consequently escapes batch error aggregation and loses its line-numbered rejection.
Could you replace this condition with:
| if not isinstance(item, dict) or item.get("type") not in {"function_call", "message"}: | |
| if not isinstance(item, dict) or item.get("type") not in ("function_call", "message"): |
Tuple membership preserves the existing policy of ignoring unrecognized opaque items without hashing their values. A turn without a recognized persisted anchor then reaches the existing missing-anchor rejection; an extra opaque item alongside a valid anchor remains preserved.
Proposed acceptance checks: parameterize the mutation over [] and {} and require AtifExportError, not TypeError. Append each malformed-type item after a valid anchor and require successful export with the item unchanged in source_turns. Retain the [1, 0] source-index assertion in the reversed-turn regression, duplicate recognized-anchor rejection, and the unique pair-reference assertions. These checks are proposed, not executed; verify the replacement before adopting it.
ddb3251 to
1987b89
Compare
705abe9 to
f814495
Compare
Add the explicit
compatibility_profile=v2-conversationroute togym eval export atifandgym_rollout_to_atif. It exports canonical conversation content, associates each step with its native turn and captured model call through persisted IDs, and preserves original native turns, timing and identity evidence in documented ATIF extensions. Conversion stayspartial, with structured warnings explaining those extension boundaries.The strict v1 behavior introduced in #2767 remains the default. The v2 profile keeps native questions and counters unchanged, preserves actual captured requests, and rejects ambiguous anchors, conflicting ownership, retries, failed tools and unsupported conversation shapes. It uses the shared timing projection without choosing between timing boundaries or treating unknown duration as zero.
A pinned OpenCode 1.17.11 rollout exported successfully with source turns, captured requests and timing provenance preserved; its failure counterpart was rejected without publishing a partial batch. The guide documents enablement, supported anchors, compatibility losses and rollback.
Depends on the #5660 → #5661 → #5507 stack; this draft targets its branch to keep the exporter review separate from the shared format and OpenCode changes.
Failed-tool export is a pre-existing limitation, not a regression caused by the new failure-detail gap. Extending support requires preserving terminal failure status/error evidence and distinguishing incomplete executions; that remains follow-up work.