refactor(voice): unify runtime input and output - #351
Conversation
a11fcb4 to
fb037a5
Compare
cabc380 to
765f371
Compare
Blockers
All 103 focused tests pass locally, and the current GitHub checks are green. PR 351 remains stacked on the unchanged PR 350 head, so PR 350's posted blockers also remain transitively relevant. |
yanziz-nvidia
left a comment
There was a problem hiding this comment.
Reviewed by yanziz-reviewer-bot
Summary
Adds VoiceSession.enqueue_response(...), a typed VoiceOutput mailbox payload, and a VoiceOutputWorker subscriber that routes complete and streaming responses through the existing participant-aware TTS/data-echo path without coupling producers to Pipecat. CI is all green (pytest 3.11 + 3.12, ruff, CodeQL, lock-check, SPDX, DCO); no dependency changes.
Legend: 🚫 Blocker · 💡 Suggestion · 🔍 Nit
| Finding | |
|---|---|
| 🚫 | None |
| 💡 | xr_ai_voice/_processors/handler.py:239 — _spawn_response can jump the per-participant queue when the inflight task is done but _start_next has not yet run. _finish_response awaits a frame push before popping _inflight; a caller landing in that window sees is_active=False and starts immediately, ahead of items already in _queued[pid]. |
| 🔍 | xr_ai_nat/events/voice.py:40 — Empty-text final chunks with interrupt=True are silently dropped. The empty-text guard fires before the interrupt path, so the cancel is a no-op with no error. |
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-sdk/xr-ai-voice/xr_ai_voice/_processors/handler.py:239— In_spawn_response, guard the direct_start_responsecall withif not self._queued.get(pid):and otherwise append to the queue, preserving FIFO across the_finish_responseawait gap.agent-sdk/xr-ai-nat/xr_ai_nat/events/voice.py:40— InVoiceOutput.validate_boundary, rejectfinal=True+ non-Noneresponse_id+ emptytext, or document thatinterrupthas no effect on empty-text terminators.
765f371 to
92764ae
Compare
fb037a5 to
dad5303
Compare
|
Updated in |
92764ae to
8bc07de
Compare
8bc07de to
2d277f7
Compare
|
Update: |
|
Reviewed at Blockers
Suggestions
The direction here is right: one bidirectional |
d299ef8 to
7dbd9b4
Compare
Signed-off-by: Devdeep Ray <devdeepr@nvidia.com>
ce09529 to
ef05580
Compare
Signed-off-by: Devdeep Ray <devdeepr@nvidia.com>
74f284a to
3703243
Compare
8bc695d to
d3caae4
Compare
|
Updated in 7e91705. The active voice/runtime findings are addressed:
I also fixed the current GitHub code-quality findings: direct transport type usage, explicit consumption of awaited results, idempotent callback removal without an empty except, and explicit async-iterator termination. Per review scope, the legacy NAT packaging/conversation-memory compatibility items and the runtime-wide participant-metadata compatibility suggestion are unchanged. Validation: 139 focused tests pass. The refreshed GitHub suite is green on Python 3.11 and 3.12, Ruff 0.15.16, both DCO checks, CodeQL, SPDX headers, dependency locks, and strict docs build. @blongs-nv please re-review the updated head. |
Signed-off-by: Devdeep Ray <devdeepr@nvidia.com>
d3caae4 to
7e91705
Compare
blongs-nv
left a comment
There was a problem hiding this comment.
Reviewed at 7e91705ebe0de6c759473276f7669611b7c9dc72 by a bot.
Blockers 1 through 6 from my previous review are resolved, and the fixes were verified in the code rather than by commit message: a cancelled producer no longer publishes its terminator and voice-closed keys are tombstoned, so barge-in no longer raises; send() now runs outside the output lock, with a real regression test that holds a genuinely blocked publisher at capacity 1 while unrelated output proceeds; failed and cancelled responses are closed on every exit path; post-eviction chunks are swallowed instead of re-opening a stream; query fan-out no longer occupies the cancellable response slot (with a delivery test); and the sample builds its vision tool lazily behind readiness, so hub sockets no longer open before probes. The suggestions on relocated coverage, mid-stream interrupt, on_data unregistration, the superseded-iterator drain, stream registration order, and the session's public surface were addressed as well.
On the deferred items: given xr-ai-nat's status as a migration-period compatibility surface and your scope call, I'm treating the [voice] packaging/DEPENDENCIES.md cleanup and the xr_conversation_memory test relocation as deferred to NAT's own cleanup rather than holding this PR for them. Please make sure the three recall_conversation cases aren't lost for good if that function group migrates rather than dies. The runtime-wide participant_id metadata suggestion is likewise noted as deferred.
Suggestions
await stream.aclose()is called twice in a row in the final-chunk path (_runtime.py:338-339); the second call is a no-op, drop it.
The fix round is thorough and well-tested, and the remaining items are deferred by agreement. This is ready to merge.
Signed-off-by: Devdeep Ray <devdeepr@nvidia.com>
Summary
VoiceAgentthat ownsVoiceSession, publishes acceptedUserQueryplus voice lifecycle events to injected typed topics, and subscribes tovoice.outputsimple-vlm-examplesoSimpleVlmAgentowns its vision tool, streamed turns, cancellation, and participant frame cleanup whileapp.pyonly composes agentsRuntime boundary
The runtime provides typed publish/subscribe routing only.
VoiceAgent.run(runtime)owns the voice session lifecycle, and application agents own their own resources and tasks.VoiceAgentowns theUserQuery,VoiceParticipantLeft, andVoiceInterruptedschemas while each sample chooses their topic names.Participant departure is published with participant scope. Interruption can be participant-scoped or global. Application agents subscribe directly and clean up their own tasks, state, and tools; the composition root installs no transport callbacks and contains no resource logic.
Voice producers publish either a complete
voice.outputmessage or correlated incremental chunks. Incremental identity is scoped by participant, producer, and response ID, so independent agents cannot merge output accidentally.VoiceAgentowns the lock protecting response aggregation and FIFO state. Runtime publication waits for subscriber delivery, so no duplicate runtime RPC orSPEAKAPI is needed.For simple VLM,
SimpleVlmAgentowns the injectedStreamingVisionTool, participant-scoped background tasks, cancellation, frame release, and nested stream cleanup.app.pyconstructs and registers the agents and supplies the sample-specific topic names only.Review feedback addressed
Validation
Rebased directly onto
mainafter #350 merged and squashed to one commit,ef05580.