fix(tts): reuse the OpenAI streaming client across sentence calls - #71087
fix(tts): reuse the OpenAI streaming client across sentence calls#71087beardedeagle wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates Hermes’ streaming TTS provider layer to make the OpenAI streaming path usable when credentials come from the Nous managed audio gateway (rather than a local OpenAI API key), and to reduce per-sentence overhead by reusing an OpenAI client across multiple stream() calls.
Changes:
- Extend
OpenAIStreamer.available()to consider both direct keys and managed-gateway readiness. - Cache OpenAI client construction on the streamer instance and reuse it for all per-sentence
stream()calls. - Coerce unsupported models to the gateway-supported default when routing via the managed gateway.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
tools/tts_streaming.py |
Makes OpenAI streaming gateway-aware, adds cached client reuse, and applies managed-gateway model coercion. |
tests/tools/test_tts_streaming.py |
Reworks tests to cover managed-gateway availability, model coercion, and client reuse (but currently appears truncated vs prior broader coverage). |
Comments suppressed due to low confidence (1)
tests/tools/test_tts_streaming.py:140
- This test module appears to have been unintentionally truncated: it now ends right after a section header ("Dispatch: chunked streamer path") and no longer covers SentenceChunker, interruption latch, resolver/registry behavior, or the streaming-vs-sync dispatch pipeline that previously lived in this file. That’s a significant loss of regression coverage unrelated to the OpenAI gateway changes described in the PR.
# ── Dispatch: chunked streamer path ──────────────────────────────────────
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c19df6d to
525324a
Compare
|
Note on test file coverage: This PR's test file intentionally contains only the 3 gateway-specific tests (availability via managed gateway, model coercion, client reuse). The broader streaming pipeline tests (SentenceChunker, interruption latch, resolver/registry, dispatch pipeline, PCM alignment, PortAudio resilience) are in PR #71084. The test file was split between PRs to keep each PR's tests scoped to its own code changes. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing the managed-gateway streaming gap. The premise is confirmed on current main: OpenAIStreamer.available() only accepts configured/direct credentials at tools/tts_streaming.py:272, while the sync path resolves the managed gateway at tools/tts_tool.py:3241-3285.
Problems
- This patch deletes 207 lines of existing coverage from
tests/tools/test_tts_streaming.pyand leaves three tests. The referenced coverage PR #71084 is still open, so this PR must preserve its own baseline coverage. tools/tts_streaming.py:13-17describes background per-sentence prefetch, but this PR does not changetools/tts_tool.py; current main invokesstreamer.stream()serially attools/tts_tool.py:3445-3468.
Suggested changes
- Keep the existing streaming tests and add the gateway tests alongside them.
- Salvage the gateway resolver/client reuse onto current main without replacing its config-key and profile-scoped credential resolution (
tools/tts_streaming.py:255-293,tools/tool_backend_helpers.py:255-275).
Automated hermes-sweeper review.
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Two PRs address the same confirmed managed-gateway streaming gap and per-sentence OpenAI client recreation: both extend gateway-aware availability, reuse one client, and coerce unsupported gateway models. Their diffs also replace substantial baseline streaming coverage, while adding prefetch documentation without implementing the claimed background pipeline.
Related pull requests
- #71081 [closed]
duplicate— (+153/-218) — superseded by #71087: This closed PR contains the earlier near-identical gateway resolver, cached-client reuse, model-coercion changes, and three focused tests; it remains relevant as the implementation precursor, but was closed to reorganize the TTS fixes. - #71087
related— (+160/-218) — keep open with a salvage path: The gateway-aware resolver and cached OpenAI client directly address the confirmed cause, but the diff deletes 207 lines of baseline SentenceChunker, resolver, interruption, dispatch, fallback, and callback coverage and documents background prefetch that it does not implement. This follows the contributor keep_open review on #71087: preserve the existing tests, remove or correct the prefetch claims, and adapt the resolver/client-reuse work without replacing current config-key and profile-scoped credential resolution.
Duplicates
#71081 and #71087 are substantially duplicate implementations; #71081 is the closed precursor superseded by #71087.
Suggested consolidation
Keep #71087 open with a salvage path: retain its managed-gateway availability, client reuse, model coercion, and focused regression tests, while restoring the deleted baseline coverage, correcting the unsupported prefetch documentation, and preserving current config-key and profile-scoped credential resolution. Keep #71081 closed as superseded by the consolidated work in #71087.
Complex graph
flowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
subgraph Dup71081 ["PRs duplicating each other"]
P71081["PR #71081 (closed)"]
P71087["PR #71087 (open)"]
end
class P71081 closed
class P71087 open
class P71087 target
click P71081 "https://github.com/NousResearch/hermes-agent/pull/71081"
click P71087 "https://github.com/NousResearch/hermes-agent/pull/71087"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).
Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 36 kB of PR diffs, 3 kB of issue/PR text, 8 kB of discussion (18 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
|
Flagging a discrepancy I found while reconciling this PR against current main. The sync TTS path (tools/tts_tool.py) now routes through _resolve_openai_audio_client_config(), which reads the stored tts provider via read_selection("tts") and sends managed-gateway users through resolve_managed_tool_gateway("openai-audio") with the managed token and base_url. The streaming path (tools/tts_streaming.py) was not updated in that refactor. OpenAIStreamer.available() still checks only _openai_config_api_key() or resolve_openai_audio_api_key(), and resolve_openai_audio_api_key() returns direct credentials only, never the managed gateway token. So for a user on the Nous managed gateway, the streaming path reports available() == False and never fires. Was that intentional, or should the streaming path share the sync path's resolver? Edit: the streaming fix will be submitted in a separate PR instead. |
525324a to
81b0ad5
Compare
OpenAIStreamer.stream() built a fresh OpenAI client per call, and each client carries its own HTTP/2 connection pool. Every sentence in the streaming path paid a fresh TCP+TLS handshake. Hoist the client into a cached_property so a streamer instance shares one connection across its stream() calls.
81b0ad5 to
c97fe09
Compare
Problem
OpenAIStreamer.stream() constructs a fresh OpenAI client on every call. Each client carries its own HTTP/2 connection pool, so every sentence in the streaming path pays a full TCP+TLS handshake.
Solution
Hoist the client into a cached_property on the streamer instance. All stream() calls on that instance share one OpenAI client and one connection pool. The resolved api_key and base_url are unchanged.
Regression test
Scope note
The gateway-aware availability and model-coercion work that this PR previously carried is now handled upstream in the sync TTS path, so this PR is scoped to the pooled client only.