Skip to content

Migrate XR render sample to NAT and improve eval - #345

Open
blongs-nv wants to merge 2 commits into
mainfrom
blongs-nv/render-nat-refactor
Open

Migrate XR render sample to NAT and improve eval#345
blongs-nv wants to merge 2 commits into
mainfrom
blongs-nv/render-nat-refactor

Conversation

@blongs-nv

Copy link
Copy Markdown
Contributor

Modularization/NAT update for xr render sample, including eval harness changes to support testing real flow and individual components for debugging performance in the sample.

Signed-off-by: Brent Longstaff <blongstaff@nvidia.com>
Comment thread agent-samples/xr-render-demo/eval/xr_render_demo_eval/live_explore.py Dismissed
Comment thread agent-samples/xr-render-demo/eval/xr_render_demo_eval/live_garble.py Dismissed
Comment thread agent-samples/xr-render-demo/eval/xr_render_demo_eval/live_manip.py Dismissed
Comment thread agent-samples/xr-render-demo/eval/xr_render_demo_eval/live_pose_matrix.py Dismissed
Signed-off-by: Brent Longstaff <blongstaff@nvidia.com>
@blongs-nv
blongs-nv deployed to github-pages August 11, 2026 00:05 — with GitHub Actions Active
github-actions Bot added a commit that referenced this pull request Aug 11, 2026

@nvddr nvddr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The migration is directionally strong: the worker now composes typed NAT function groups, keeps model traffic behind xr-ai-models, avoids MCP/LiveKit in the worker, and is much easier to navigate than the previous monolith. I’m requesting changes before we treat it as the repository reference, primarily for the parallel tool-call recovery path, participant-concurrency correctness, NAT lifecycle ownership, and live-eval reliability.

Please also address the documentation gaps before merge: docs/xr-render-demo.md says managed processes start concurrently even though the repository contract says they start serially; agent-samples/xr-render-demo/main.py still describes the removed voice-loudness/sphere-radius behavior; and the sample needs a colocated README/file map explaining the NAT composition chain and how users add a function group, subagent, prompt, config, and eval case. The branch is currently conflicting with main, so these paths and instructions should be revalidated after rebasing.

candidates.append(tool.name)
if len(candidates) != 1:
return None
return {"name": candidates[0], "args": dict(data), "id": _recovery_id(), "type": "tool_call"}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This creates a second tool-call parser after the serving parser and NAT, including inferring an executable tool solely from the shape of ordinary assistant JSON. Because this sample performs scene mutations, reinterpreting unstructured content as a call is too risky and also puts a model-serving quirk in xr-ai-nat rather than xr-ai-models presets. Please remove shape-based recovery and fix the configured server/parser; if normalization is genuinely needed, keep it strictly structured, explicitly named, schema-validated, and owned by xr-ai-models.

if resolved is None:
return SceneReply(response="Okay, never mind that.")
transcript = resolved
ledger.reset()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VoiceSession runs participant turns concurrently, but this ledger is one supervisor-wide instance. A second participant can reset it while the first participant is inside the object agent, despite that agent’s own lock. The appearance guard is similarly shared and unprotected, and the global scene before/after comparison can mistake another participant’s mutation for this turn’s success. Please make these states invocation- or participant-scoped (prefer NAT per-user functions/groups or equivalent request context) and add a concurrent two-participant test.

await context.record_moves(request.participant_id, before)
return SceneReply(response=str(output or "Done."))

return LambdaFunction.from_info(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a reference NAT sample, the top-level workflow should not bypass the registry and builder lifecycle with private nat.builder.function.LambdaFunction. SceneSupervisorConfig is currently never registered, so the builder cannot own or inspect this function’s configuration/dependencies consistently. Please register the supervisor with the stable plugin API, add it through builder.add_function(...), and express its dependencies as explicit refs/config rather than a custom factory with hidden function handles.

print(f"{verdict} {case['name']:22s} {detail}", flush=True)
passed += verdict == "PASS"
failed += verdict == "FAIL"
print(f"\nlive manipulation: {passed} passed, {failed} failed", flush=True)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This command counts failures but still returns exit status 0. The same problem exists in live_garble, live_explore, and the no-change path in live_smoke, so these advertised eval commands cannot act as regression gates or reliable agent automation. Please make every eval entry point exit nonzero when any case fails, after cleanup has completed.

await clear_scene(scene)
before = {i.id for i in (await scene.get_scene_state(EmptyRequest())).objects}
await endpoint.inject_data(DataMessage(
participant_id=participant, topic="live.smoke.text",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reuses the single participant created before the loops for every pose/prompt case. That accumulates transcript history across cases, directly violating the eval README’s isolation rule and contaminating later results. Please create a fresh participant per independent case (and emit a leave event afterward), as the other multi-case live drivers attempt to do.

@nvddr nvddr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Traceability follow-up after comparing the tea-making sample. Tea’s useful invariant is one correlated record per turn, agent, tool call, retry, and failure; its sample-local emitter/viewer should not become a second observability framework here. NAT already emits IntermediateStep FUNCTION/LLM/TOOL events and its Workflow runner supplies workflow_run_id/trace_id. Please wire the render sample through that supported trace/exporter path, adding only XR-specific context such as participant, supersession/cancellation, and verified scene diff.

Please also add a regression that injects a failing leaf function and proves the durable trace identifies the turn, participant, subagent, tool, sanitized arguments, exception type/message, retry decision, and terminal outcome.

await builder.add_function_group("vision", vision)

supervisor = await scene_supervisor(builder=builder, llm=llm)
handler = as_voice_handler(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please route each turn through a built NAT Workflow/runner (or add that support to the shared xr_ai_nat voice adapter) instead of invoking a bare Function. The bare path emits intermediate steps into an unconsumed stream and never establishes a per-turn workflow_run_id/trace ID or exporter, so concurrent supervisor, subagent, and tool records cannot be correlated. Bind participant_id as request metadata and record terminal outcomes including superseded/cancelled. This is the integration point for NAT tracing; please do not add a render-only event framework.

llm_name=_LLM_NAME,
tool_names=[FunctionRef(name) for name, _config in subagents],
system_prompt=_PROMPT.read_text(encoding="utf-8").strip(),
handle_tool_errors=True,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handle_tool_errors=True makes LangGraph convert tool exceptions into ToolMessages and continue. With detailed logs off and no IntermediateStep subscriber/exporter in this sample, handled validation/RPC failures can disappear into model scratch and leave only the final reply. Keep recovery only for expected, user-correctable errors and ensure the NAT trace records every tool start/result/failure with run/participant ID, agent, tool, attempt, duration, sanitized arguments/result, and exception classification. Apply this once through shared NAT observability/middleware rather than wrapping each tool.

# pass below then gets a chance to complete the turn.
try:
output = await reasoning.ainvoke(message, to_type=str)
except Exception as error:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This catch collapses model, service, schema, and programming failures into the same user reply, and logger.error(..., error) preserves no traceback. Catch only a documented recoverable exception; for unexpected failures emit the correlated terminal error, use logger.exception (or equivalent structured exception capture), and re-raise so the runtime remains fail-fast. Running the verification pass after an unknown failure is also unsafe because the first pass may already have mutated the scene.

# against scene data alone, then degrade explicitly.
try:
output = await reasoning.ainvoke(message, to_type=str)
except Exception as error:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assumes every exception means ‘camera unavailable,’ then the second broad catch discards that failure entirely. A VLM timeout, invalid tool schema, service/config error, or code defect is therefore mislabeled and untraceable, and the first exception text is injected into the retry prompt. Catch only the typed no-frame/unavailable condition, emit a correlated retry record, and retain both attempt outcomes; unexpected exceptions should keep their traceback and propagate.

Comment thread docs/xr-render-demo.md
attached with `as_voice_handler`, and `record_voice_transcripts` persists
each completed turn into native text memory, which `recall_conversation`
reads back as the `[Recent conversation]` block. A new utterance while TTS
is playing supersedes the previous turn.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a reference sample, please add a concise trace/debug contract here: where the durable trace is written, how one ID links voice query → supervisor → subagent → leaf NAT tool → verification/cancellation, which inputs/results are redacted, and the exact command to inspect one turn or tool failure. Document the recoverable-vs-fatal error and retry policy too. The tea sample’s event table and human-test loop are useful inspiration; a separate viewer is not required.

@yanziz-nvidia yanziz-nvidia left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed by yanziz-reviewer-bot

Summary

Replaces the single-agent pipecat pipeline with a supervisor plus five focused subagents over xr-ai-voice/NAT, and promotes the flat eval.py script into a four-tier xr-render-demo-eval package. All 14 CI checks green; DEPENDENCIES.md and AGENTS.md updated alongside the pyproject.toml changes.

Legend: 🚫 Blocker · 💡 Suggestion · 🔍 Nit

Finding
🚫 None
💡 xr_render_demo_eval/harness.py:918-927audit_prompts guards both sibling imports with one try/except ImportError, so a failure in either tier silently skips the else block for both. The audit then passes while covering none of the subagent/supervisor cases.
🔍 xr_render_demo_eval/harness.py:595_NO_MUTATION is byte-identical to _MUTATING at line 446. Two names for one set will drift if either is edited.

Actionables (for bots — copy-paste-ready for AI)

Fix if it makes sense in context — these are agent-generated suggestions, not human-vetted obligations. Skip anything that's wrong, already addressed, or not worth the churn.

  • agent-samples/xr-render-demo/eval/xr_render_demo_eval/harness.py:918 — Split the try/except ImportError in audit_prompts into one block per sibling module so a missing dep in one tier doesn't silently drop the other tier's cases from the audit.
  • agent-samples/xr-render-demo/eval/xr_render_demo_eval/harness.py:595 — Either alias _NO_MUTATION = _MUTATING or drop one name; the duplicated literal is a drift hazard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants