Skip to content

fix: migrate openai mllm to ga realtime api and add per-join greeting - #2278

Open
BenWeekes wants to merge 4 commits into
mainfrom
fix/openai-mllm-ga
Open

fix: migrate openai mllm to ga realtime api and add per-join greeting#2278
BenWeekes wants to merge 4 commits into
mainfrom
fix/openai-mllm-ga

Conversation

@BenWeekes

Copy link
Copy Markdown
Contributor

What

Two changes to openai_mllm_python:

1. Migrate to the GA Realtime API shape. OpenAI retired the beta protocol (beta_api_shape_disabled), which currently breaks this extension for everyone — sessions cannot even be created. This drops the OpenAI-Beta header, maps the renamed GA server events onto the existing message classes (response.output_audio.delta, response.output_audio_transcript.delta, conversation.item.added/done, …), and sends the GA session.update shape (type: realtime, output_modalities, nested audio.input/output with transcription and turn_detection).

2. Per-join greeting param. greeting config field (default "" = off), spoken once per join after the session config is applied, as a user message item plus a response request — the same semantics as gemini_mllm_python's existing greeting. This makes the extension compatible with platforms that supply a greeting per join instead of hardcoding it in graph properties.

Verification

  • Verified against the live GA service: session created and updated, audio in/out, transcripts, and spoken greeting all working in a voice-assistant graph.
  • The greeting delivery path (item.create + response.create) is the same code path main_control already exercises in the realtime example.

Ubuntu added 2 commits August 4, 2026 17:20
OpenAI retired the realtime beta shape (beta_api_shape_disabled), which
broke openai_mllm_python everywhere. Drop the OpenAI-Beta header, map
the renamed ga server events onto the existing message classes, and
send the ga session.update shape (type realtime, output_modalities,
nested audio.input/output with transcription and turn_detection).
Verified against the live service: session created and updated, event
stream parsed.
greeting config field (default empty = off) spoken once per join after
the session config is applied, as a user message item plus a response
request — same semantics as the gemini mllm extension. intended to be
supplied per join by the platform rather than hardcoded in graphs.
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown

Review: migrate openai mllm to GA realtime API + per-join greeting

Thanks for this — the GA migration is a genuine unblock (the extension cannot create a session at all today), and mirroring gemini_mllm_pythons greeting semantics is the right call for consistency. A few things I would want addressed before merge, plus some smaller notes.

BLOCKING

  1. Five imports are now dead and will fail lint.

_update_session() was the only consumer of SessionUpdate, SessionUpdateParams, InputAudioTranscription, ServerVADUpdateParams, and SemanticVADUpdateParams, and the rewrite drops all of those call sites — but the import block at extension.py:60-67 is untouched. agents/scripts/pylint.sh runs pylint-exit --warn-fail, and W0611 is not in the .pylintrc disable list, so this should fail CI. Removing them also makes the diff self-documenting about what the beta shape no longer needs.

  1. vendor: "azure" looks broken by this change.

_update_session() now sends the GA shape unconditionally, but the Azure realtime service is still on the beta shape (flat modalities / voice / input_audio_transcription, session-level turn_detection). connect() keeps the Azure api-key header path, so we connect and then send a payload Azure rejects. Related: the old payload carried model=self.config.model, and the new one drops it — for OpenAI that is fine because connection.py:55 appends ?model= to the URL, but that append is explicitly gated on not self.vendor, so Azure previously got its model only via session.update and now gets it nowhere.

README.md documents vendor: "azure" as supported, so this is a user-visible regression. Either branch on self.config.vendor and keep the beta payload for Azure, or — if Azure support is considered dead — say so in the PR and drop it explicitly rather than by accident.

WORTH CHANGING

  1. The transcription model is hardcoded. "model": "gpt-4o-mini-transcribe" is baked into the payload, and it also silently changes behavior: the old InputAudioTranscription dataclass defaulted to gpt-4o-transcribe. Please make this a config field (e.g. input_transcript_model: str = "gpt-4o-mini-transcribe") with a matching manifest.json property, so operators can pick accuracy vs. cost without a code change.

  2. Greeting replays on every reconnect, not every join. _greeting_sent resets in the SessionCreated branch, and _handle_reconnect() is an unbounded auto-retry loop (stop_connection then sleep(1) then start_connection). So a mid-call network blip produces a fresh session.created and the agent greets the user again in the middle of the conversation. Resetting the flag in on_start/on_init instead of per-session ties it to the actual join, which is what the config comment promises. (Gemini has the same structure, but its reconnect path is not this automatic.)

  3. The greeting docstring oversells what happens. Calling it a "Spoken opening line" reads as "the model says this verbatim," but the implementation posts it as a USER message and then requests a response — so the model replies to the greeting rather than speaking it. Worth rewording to something like "opening user turn used to trigger the assistant first response," so nobody configures "Hi, I am your assistant" and is surprised by what comes out.

  4. conversation.item.added and conversation.item.done both map to ItemCreated. Every item now yields two identical ItemCreated messages. Harmless today because the handler only log_debugs, but it is a trap for whoever next adds context tracking or dedup logic to that branch. Consider mapping only .added, or adding a comment noting the intentional double-delivery.

NITS

  • Hoist ga_aliases to module level. parse_server_message() runs for every server event including audio deltas; rebuilding the dict per call is wasted allocation on the hot path. A module-level constant is also easier to eyeball against the GA event list.
  • send_json duplicates send_request. The bodies are identical apart from json.dumps vs to_json. Having send_request delegate to send_json avoids the two drifting.
  • Raw dicts vs. struct.py. struct.py exists to be the typed protocol layer; hand-rolled dicts in extension.py skip it, so field-name typos become runtime 400s instead of type errors. Adding GA dataclasses would be more work but keeps the module design intact. If dicts are a deliberate interim step, a TODO saying so would help.
  • audio.output.voice is still sent when audio_out=False. The old code switched to text-only modalities and omitted the voice entirely; now output_modalities: ["text"] ships alongside a voice config. Probably ignored, but it is contradictory.
  • Docs/defaults consistency with Gemini. Gemini greeting appears in property.json ("greeting": "") and in its README property table. This PR adds the manifest.json property only — adding both would match.

TESTING

There is no tests/ directory for openai_mllm_python, and this PR is exactly the kind of change that wants one: the value delivered is entirely "we emit and parse the right protocol shapes." Live verification is great, but it does not survive the next refactor. Two cheap, dependency-free unit tests would lock in the behavior:

  1. parse_server_message() over one recorded GA payload per alias — assert the returned class and that message.type is the beta enum the match statement expects.
  2. The session.update payload — assert type == "realtime", output_modalities flips with audio_out, and audio.input.turn_detection picks up both the server_vad and semantic_vad shapes.

parse_server_message is a pure function, so (1) is nearly free. Since the alias table was derived by hand, a test is also the cheapest way to confirm no renamed event was missed — could you note which GA events you saw on the wire during verification (particularly conversation.item.input_audio_transcription.* and response.function_call_arguments.*), so we can confirm those genuinely kept their beta names?

Nothing here is architectural — mostly the dead imports, the Azure path, and a test to hold the shape in place. The direction is right and this needs to land.

@BenWeekes

Copy link
Copy Markdown
Contributor Author

Version bumped to 0.2.3 in both manifest.json and pyproject.toml.

No guarder suite exists for MLLM extensions (only asr_guarder/tts_guarder); verification was done against the live GA Realtime service in a voice-assistant graph: session create/update, bidirectional audio, transcripts, and the per-join greeting all confirmed working.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Review

Thanks for this — the GA migration is unblocking work, and the diff is small and readable. The alias-table approach for the renamed events is a pragmatic way to avoid rewriting the whole struct.py layer. A few things I think need addressing before merge, then some smaller notes.

1. Unused imports will fail CI (task lint)

_update_session no longer constructs any of the typed session objects, but the import block in extension.py is unchanged. These five names now have no remaining reference in the file (their only uses were the lines this PR deletes at ~591–614):

  • SessionUpdate
  • SessionUpdateParams
  • InputAudioTranscription
  • ServerVADUpdateParams
  • SemanticVADUpdateParams

AGENTS.md is explicit that a single W0611: unused-import fails the pylint job, so this is likely a red build rather than a style nit. Worth running the documented pre-push command:

sudo docker exec ten_agent_dev bash -c "cd /app && task format && task check && task lint"

2. Azure path now gets the GA payload unconditionally

_update_session always emits the GA shape (type: realtime, output_modalities, nested audio.input/output), but the extension still supports vendor: "azure" — the README documents it with gpt-4o-realtime-preview and an api-version-pinned path, and connect() still has the dedicated api-key header branch. An Azure endpoint pinned to an older api-version will very likely reject the GA session shape, so Azure users go from working to broken.

Was dropping Azure intended? If not, the shape probably needs gating on self.config.vendor (keep the old typed path for Azure, GA for OpenAI). If it was intended, that's a breaking change worth calling out in the PR body and the README rather than leaving implicit.

Related: in connect(), the elif not self.vendor: branch is now a single statement setting auth and the headers assignment is gone. That's correct (headers defaults to {}), just noting I checked it.

3. Greeting replays on reconnect, not "once per join"

_greeting_sent = False is reset in the SessionCreated case, and _handle_reconnect() calls start_connection() after any loop exit — so a mid-call network blip produces a fresh SessionCreatedSessionUpdated → greeting again. Worse, _resume_context() replays the conversation history on that same reconnect, so the model re-greets in the middle of an established conversation.

gemini_mllm_python has the same reset placement, so this is consistent with precedent — but if the intent is genuinely per-join, the flag should be reset on extension start (or on an explicit new-session signal), not on every SessionCreated. At minimum consider skipping the greeting when self.message_context is non-empty.

Separately: if a graph's main_control already sends its own greeting (as the realtime example does), setting this property yields two greetings. A one-line note in the README would save someone a debugging session.

4. conversation.item.done and .added both map to ItemCreated

Both GA events alias to the same class, so the case ItemCreated() handler fires twice per item. Harmless today because that branch only logs at debug level, but it's a trap for whoever next adds bookkeeping there. Either map only conversation.item.added, or add a comment noting the intentional double-delivery.

5. Smaller items

Hardcoded transcription model. "model": "gpt-4o-mini-transcribe" is baked into the payload. Everything else in this config surface is user-tunable, and Azure deployments won't necessarily host that model. Suggest a config field defaulting to the current value.

voice sent even in text-only mode. The old code set voice only when audio_out was true; the new payload always includes audio.output.voice alongside output_modalities: ["text"]. Probably tolerated, but it's a behavior change and I couldn't verify it against a text-only session.

model no longer sent in session.update. It's in the URL query string so this likely doesn't matter, but config.model is now inert for the session config — worth confirming that's deliberate. (temperature / max_tokens were already unused before this PR, so no regression there, but they're still dead config fields.)

Hoist ga_aliases to module level. It's rebuilt on every call to parse_server_message, which runs per event — including every audio delta. Negligible cost, but this is the hottest path in the extension and a module-level constant is simpler anyway.

cls.__dataclass_fields__["type"].default works but reaches into dataclass internals. Since type has a default on every aliased class, dropping the key is equivalent and reads better:

data = {k: v for k, v in data.items() if k != "type"}

Unknown GA events. The alias table is a fixed allowlist and parse_server_message still raises ValueError on anything unrecognized. GA added and renamed more events than the eight listed here. Worth confirming an unmapped event degrades gracefully rather than propagating out of the async for and triggering a full _handle_reconnect() — a session torn down by one unrecognized event would be a rough failure mode.

property.json and README. gemini_mllm_python ships "greeting": "" in property.json plus a README row; this PR adds only the manifest schema entry. Adding both keeps the two MLLM extensions symmetrical and makes the property discoverable in TMAN Designer.

send_json duplicates send_request. Same three lines minus serialization. Having send_request delegate to send_json would avoid the drift, though I understand the untyped escape hatch is deliberate here.

6. Test coverage

There's no tests/ directory in this extension, and verification is manual against the live service — which is valuable but not repeatable in CI, and won't catch the next protocol rename. The two things this PR adds are both pure functions and cheap to test:

  • parse_server_message for each GA alias → asserts the right class and that type is normalized
  • the session payload builder for server_vad / semantic_vad × audio_out true/false

Extracting the payload construction into a small helper that returns the dict (rather than building it inline before the send) would make the second one testable without a websocket.

Security

Nothing new introduced — the GA payload logs instructions but no credentials. Pre-existing and out of scope, but flagging while you're in the file: on_init does ten_env.log_info(f"config: {self.config}"), which prints api_key in plaintext. The repo convention (04_conventions.md, 08_security.md) is a to_str(sensitive_handling=True) that encrypts secrets first. Might be worth a follow-up.


Items 1–3 are the ones I'd want resolved; the rest are discretionary.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Review

The GA migration is a real unblock — the extension cannot create a session at all against the current service, so this needs to land. The alias-table approach avoids rewriting struct.py wholesale, which is a reasonable trade for an urgent fix. Notes below are ordered by what I would actually want changed.

I verified against the PR head (33f7e44), so a couple of items from earlier review rounds are now resolved or were wrong — flagged inline.

Resolved since earlier review rounds

The five unused session imports (SessionUpdate, SessionUpdateParams, InputAudioTranscription, ServerVADUpdateParams, SemanticVADUpdateParams) are gone as of 33f7e44. That lint concern is closed.

Correction to an earlier claim: unmapped GA events do not tear down the session

Previous rounds flagged that parse_server_message's ValueError on an unrecognized event could propagate out of the async for and trigger a full _handle_reconnect(). That is not what happens. connection.py:119-124:

def handle_server_message(self, message: str) -> ServerToClientMessage:
    try:
        return parse_server_message(message)
    except Exception as e:
        self.ten_env.log_info(f"Error handling message {e}")

The bare except swallows it and falls through returning None, which is yielded into the loop and lands on case _ -> log_debug. So unmapped GA events already degrade gracefully. Worth knowing because it lowers the risk of an incomplete alias table — a missed rename is a silently dropped event, not a dropped session. Still worth fixing when found, but no defensive change is needed here.

1. Azure is broken by this change

_update_session() now emits the GA shape unconditionally, but vendor: "azure" is still a documented, supported path — connect() keeps the dedicated api-key branch and the README documents vendor: "azure" with gpt-4o-realtime-preview and an api-version-pinned path. An endpoint pinned to an older api-version will reject type: realtime / output_modalities / nested audio.*. So we connect successfully and then fail on session config.

There is a second-order problem specific to Azure. The old payload carried model=self.config.model; the new one drops it. For OpenAI that is harmless because connection.py:55 appends ?model= to the URL — but that append is gated on not self.vendor:

if not self.vendor and "model=" not in self.url:
    self.url += f"?model={model}"

Azure previously received its model only via session.update. Now it receives it nowhere, so config.model is fully inert for Azure.

Either branch on self.config.vendor and keep the typed beta payload for Azure, or — if Azure is considered dead — drop it deliberately and say so in the README and PR body. Right now it breaks by accident, which is the worst version.

2. Greeting fires on every reconnect, and collides with main_control's greeting

Two separate problems with "once per join."

Reconnect replay. _greeting_sent = False resets in the SessionCreated branch, and _handle_reconnect() retries unconditionally after any loop exit (stop_connection -> sleep(1) -> start_connection). A mid-call network blip produces a fresh SessionCreated -> SessionUpdated -> greeting again. And _resume_context() replays history on that same reconnect, so the model re-greets on top of an established conversation. Resetting the flag on extension start rather than per-session would match what the config comment promises.

Double greeting in the shipped example. This one I confirmed concretely, and it is the more likely support burden. agents/examples/voice-assistant-realtime/tenapp/property.json:32 already sets a greeting on main_control, and main_python/extension.py:162 implements it:

async def _greeting_if_ready(self):
    if (self._rtc_user_count == 1 and self.config.greeting and self.session_ready):
        await self._send_message_item(
            MLLMClientMessageItem(role="user", content=f"say {self.config.greeting} to me")
        )
        await self._send_create_response()

That is triggered by the MLLMServerSessionReady this extension sends on SessionUpdated — the same event that now triggers the extension-level greeting. Setting greeting on the extension in that graph yields two user items and two response.create calls back to back. Worth at least a README note that the two are mutually exclusive; better would be picking one layer to own greetings.

Related coupling worth being aware of: _update_session() is also called from send_client_register_tool, so a mid-session tool registration produces another SessionUpdated. _greeting_sent guards the extension side, but send_server_session_ready is re-sent on every SessionUpdated (pre-existing), which re-arms main_control's path. Not introduced here, but it is the same wire.

3. Transcription model is hardcoded and silently changes behavior

"model": "gpt-4o-mini-transcribe" is baked into the payload. The old InputAudioTranscription dataclass defaulted to gpt-4o-transcribe (struct.py:75), so this quietly swaps every user onto the smaller model — an accuracy regression nobody opted into. Everything else on this config surface is tunable; suggest input_transcript_model: str = "gpt-4o-transcribe" with a matching manifest.json entry, preserving the prior default.

4. conversation.item.added and .done both map to ItemCreated

Every item yields two identical ItemCreated messages. Harmless today since that branch only log_debugs, but it is a landmine for whoever adds context tracking there. Map only .added, or add a comment noting the double-delivery is intentional.

5. Smaller items

  • Hoist ga_aliases to module level. It is rebuilt on every parse_server_message call, which includes every audio delta — the hottest path in the extension. A module constant is also easier to diff against the GA event list.
  • voice sent in text-only mode. Old code omitted voice when audio_out=False; now audio.output.voice ships alongside output_modalities: ["text"]. Probably ignored, but contradictory.
  • send_json's verbose logging is dead. verbose defaults to False and extension.py:143 never passes it, so neither send_json nor send_request ever logs. Fine, just noting the new logging line buys nothing as wired.
  • cls.__dataclass_fields__["type"].default reaches into dataclass internals for no benefit — dispatch in extension.py is by class via match, not by the type string, so the value does not affect behavior. Dropping the key entirely is equivalent since every aliased class defaults it.
  • send_json duplicates send_request apart from json.dumps vs to_json. Having the latter delegate avoids drift.
  • property.json and README. gemini_mllm_python ships "greeting": "" in property.json plus a README row; this PR adds only the manifest schema entry. Adding both keeps the two extensions symmetrical and makes the property discoverable in TMAN Designer.
  • Greeting doc comment oversells it. "Spoken opening line" reads as verbatim, but it is posted as a user message that the model replies to. Someone will configure "Hi, I'm your assistant" and be surprised. Reword to something like "opening user turn used to trigger the assistant's first response."
  • Raw dicts bypass struct.py. That module exists to be the typed protocol layer; hand-rolled dicts turn field-name typos into runtime 400s. Acceptable as an interim step given the urgency, but a TODO saying so would help the next person.

Test coverage

No tests/ directory for this extension, and the entire value of this PR is "we emit and parse the right protocol shapes" — precisely the thing manual live verification will not preserve through the next refactor. Two dependency-free unit tests would lock it in:

  1. parse_server_message() over one recorded GA payload per alias, asserting the returned class. It is a pure function, so this is nearly free.
  2. The session.update payload: type == "realtime", output_modalities flips with audio_out, and audio.input.turn_detection picks up both VAD shapes. Extracting the dict construction into a helper that returns it (rather than building inline before the send) makes this testable without a websocket.

Since the alias table was derived by hand, (1) is also the cheapest way to confirm nothing was missed. Could you note which GA events you actually saw on the wire during verification — particularly conversation.item.input_audio_transcription.* and response.function_call_arguments.*, which the table assumes kept their beta names? If those renamed too, transcripts and tool calls are silently dropped (per the correction above, dropped rather than fatal, which makes it harder to notice).

Security

Nothing new introduced. While you are in the file: on_init does ten_env.log_info(f"config: {self.config}"), which prints api_key in plaintext. Repo convention (04_conventions.md, 08_security.md) is to_str(sensitive_handling=True). Pre-existing and out of scope — worth a follow-up.

Conventions

Commits follow conventional-commits correctly (fix(mllm):, feat:, chore:), lowercase, present tense. One nit: the commit author is ubuntu@ip-172-31-22-252.us-west-2.compute.internal, an internal EC2 hostname, rather than a real address.


Items 1 and 2 are the ones I would want resolved before merge — Azure is a silent regression on a documented path, and the greeting double-fire will hit anyone using the realtime example. Item 3 is a one-line default change with real user impact. The rest is discretionary.

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.

1 participant