Broad refactor of service settings and how they’re updated at runtime#3714
Broad refactor of service settings and how they’re updated at runtime#3714
Conversation
|
|
||
| self.set_model_name(model) | ||
| self.set_voice(voice_id) | ||
| self._voice_id = voice_id |
There was a problem hiding this comment.
Previously it wasn't clear whether set_voice/set_model/set_language were meant to be public APIs that users could directly invoke—in which case they should handle applying the change by reconnecting if necessary, etc.—or whether they were meant to only be "bookkeeping" methods, invokable only by subclasses, to update the underlying instance variables (_voice_id, etc).
I've made the decision here to make set_voice/set_model/set_language the "public" methods (even though we probably would suggest that users to use *UpdateSettingsFrames instead) and if we just need to update bookkeeping, access the instance variable directly.
There was a problem hiding this comment.
We do use set_voice() directly in one example.
There was a problem hiding this comment.
But I see that it is deprecated now, should we keep using it in our example ?
There was a problem hiding this comment.
Hmm...actually, in the example (example 35), we kind of need the voice change to take effect immediately. It can't be queued. The voice switches occur partway through the TTS service processing an LLM response. So using an STTUpdateSettingsFrame is not really a drop-in replacement for set_voice()...
But also...that example doesn't quite work? The timing of the voice switches is always wrong. I've confirmed that this was the case before the refactor, too.
With these things in mind, I'm going to punt on updating that example for now.
kompfner
force-pushed
the
pk/service-settings-refactor
branch
13 times, most recently
from
25536e8 to
b1f4c32
Compare
| language_codes: List of Google STT language code strings | ||
| (e.g. ``["en-US"]``). | ||
|
|
||
| .. deprecated:: 0.0.103 |
There was a problem hiding this comment.
TODO: update this (and all other occurrences of this) if it looks like these changes will land in a different version
kompfner
force-pushed
the
pk/service-settings-refactor
branch
10 times, most recently
from
7a8f453 to
6d7794f
Compare
| model: Any = field(default_factory=lambda: NOT_GIVEN) | ||
| """AI model identifier (e.g. ``"gpt-4o"``, ``"eleven_turbo_v2_5"``).""" | ||
|
|
||
| extra: Dict[str, Any] = field(default_factory=dict) |
There was a problem hiding this comment.
Note that this PR doesn't go through every service and add handling for the extra field. It will be up to each service to add it when it's a priority. This can be done piecemeal.
…ce settings discoverable and strongly-typed. Service settings can be updated at runtime with `*UpdateSettingsFrame`s. Does not (yet) touch `InputParams`, to avoid scope creep and touching something currently part of the public API. But there is a lot of overlap between `*Settings` object fields and `InputParams` fields. Other than discoverability/typing, these are some other improvements brought by this refactor: - There is now a single code path (see `_update_settings_from_typed`) where services can respond to settings changes (by, say, reconnecting if needed), improving maintainability and guaranteeing one and only one reconnection no matter which settings changed - `set_language`/`set_model`/`set_voice`—which we're assuming are usable as public methods, though *not* recommended over `*UpdateSettingsFrame`—all use the same code path as settings updates. They're also now all consistent in that, if a service needs to respond to a change (by, say, reconnecting if needed), any of these methods will kick off that process. Note that this is technically a behavior change. - Several services now properly react to changed settings by reconnecting: - `AWSTranscribeSTTService` - `AzureSTTService` - `SonioxSTTService` - `GladiaSTTService` - `SpeechmaticsSTTService` - `AssemblyAISTTService` - `CartesiaSTTService` - `FishAudioTTSService` (would previously only reconnect when `model` changed) - `GoogleSTTService` - `SpeechmaticsSTTService` (which previously only handled *some* settings updates through a nonstandard public `update_params` method) - `GradiumSTTService` - `NvidiaSegmentedSTTService` (which previously only handled changes to language) - Bookkeeping across various services has been reduced, mostly by deduping ivars; the `self._settings` ivar is treated as the source of truth NOTE: I pretty much guarantee that there are services missed in this PR in terms of bringing to consistency with how updates are handled (like whether changes in certain fields trigger reconnects when they need to). We can squash remaining inconsistencies as we stumble onto them, service by service. The goal here is to get things *mostly* in order, and establish the infrastructure and patterns we'll need going forward.
…d-service-settings path. `filter_incomplete_user_turns` and `user_turn_completion_config` were only handled in the legacy dict-based `_update_settings` code path. This adds them to `LLMSettings` and introduces `LLMService._update_settings_from_typed` so the typed path handles them too.
…or better editor support. Standardize all STT, TTS, and LLM service classes to declare `_settings` with the narrowed Settings type as a class-level annotation. This gives editors and type checkers the specific type when hovering or autocompleting on `self._settings` in each service and its subclasses. Inline `self._settings: Type = ...` assignments are replaced with plain `self._settings = ...`.
@kompfner understood and thank you for your reply. would this work in flows to swap deepgram keyterms as you navigate from node to node through the settings frame? |
…le settings Update docstrings for ServiceSettings, LLMSettings, TTSSettings, and STTSettings to make clear these capture only the subset of service configuration that can be changed while the pipeline is running via UpdateSettingsFrame, not all constructor parameters.
Simplify the reconnect example to show a common pattern (reconnect on any change) and improve the _warn_unhandled_updated_settings example to show selective handling of specific fields.
…settings at runtime)
…cessfully disconnect/reconnect to apply runtime settings updates. For now, marking them as not yet supporting runtime settings updates.
kompfner
force-pushed
the
pk/service-settings-refactor
branch
from
13731f8 to
bcf11ec
Compare
filipi87
left a comment
filipi87
left a comment
There was a problem hiding this comment.
Hi Paul, this PR is huge, amazing job. 👏🔥
There are still a couple of comments open that I believe haven’t been addressed yet.
I am not sure whether you’ve already added them to your TODO list to be implemented as follow up PRs, or if they’re not actually needed, but I believe these are the main ones:
- Move
_settingsto the initializer, as Aleix suggested, so we only need to invokeself._sync_model_name_to_metrics()inside AiService. - Make
ServiceUpdateSettingsFrameuninterruptible. - The name
updateinsideServiceUpdateSettingsFramefeels a bit generic. Would it make more sense to call itnew_settings,delta_settings, orservice_settings? GeminiTTSServicestill holds a reference toself._model, which is never updated. I believe the same might be true forHathoraSTTServiceandHathoraTTSService.- Inside
ServiceSettings.apply_update, I added two comments suggesting defaulting toNOT_GIVEN. It might be worth applying that change.
Other considerations:
- Inside
AsyncAITTSSettings, since the API expects a nestedoutput_format, would it be clearer to keep that nested structure closer to what the service API expects ? For example, keepingoutput_formatinsideAsyncAITTSSettingsinstead of flattening the fields. Maybe the same apply for other services.
In any case, I am leaving my approval, as I agree we should merge this as soon as possible. Any improvements can be implemented in follow up PRs, which should be much easier to maintain.
Again, great work. 🔥🚀
Split the single "changed" entry into separate "added", "changed", and "deprecated" entries for clarity. Add a note about the subtle behavior change in the deprecated set_model/set_voice/set_language methods.
… frame reference Use wildcard `*UpdateSettingsFrame` to cover all frame types. Clarify that NOT_GIVEN only appears in update deltas, not in the service's current settings state.
| input_params: Soniox ``SonioxInputParams`` for detailed configuration. | ||
| """ | ||
|
|
||
| input_params: SonioxInputParams | _NotGiven = field(default_factory=lambda: NOT_GIVEN) |
There was a problem hiding this comment.
TODO: this doesn't follow our new pattern. We should include the fields of the input params directly in the settings object. Audit the other services for this mistake, too.
There was a problem hiding this comment.
Found that Soniox STT and Gladia STT weren't following the new pattern. Updating in #3812.
| self.set_model_name(merged_options["model"]) | ||
| self._settings = merged_options | ||
| merged_live_options = LiveOptions(**merged_options) | ||
| self._settings = DeepgramSTTSettings( |
There was a problem hiding this comment.
TODO: previously looks like we stored all the LiveOptions fields directly in self._settings, meaning that users could specify those fields directly in the update dictionary, not nested under live_options. Fix this broken backward compatibility (use from_mapping), and audit other services for the same kind of mistake. To audit, maybe look for:
- other uses of
LiveOptions to_dict()in service initializers*Settingsobjects that contain any 3rd-party SDK objects in their fields
|
Thanks @filipi87! Will go ahead and merge just to make conflicts more manageable, but will open various follow-up PRs in a hopefully more targeted and rapid-fire manner. |
|
Though I did not test that specific thing, in theory, yes, the |
|
@filipi87 I think I addressed all the comments except the suggestion of passing settings to |
The ServiceSettings refactor (PR #3714) changed self._settings from dicts to dataclass subclasses, but tracing code still used .items(), in containment, and subscript access, causing AttributeError on every traced call. Use given_fields() for iteration and attribute access for named fields.
The ServiceSettings refactor (PR #3714) changed self._settings from dicts to dataclass subclasses, but tracing code still used .items(), in containment, and subscript access, causing AttributeError on every traced call. Use given_fields() for iteration and attribute access for named fields.
The ServiceSettings refactor (PR #3714) changed self._settings from dicts to dataclass subclasses, but tracing code still used .items(), in containment, and subscript access, causing AttributeError on every traced call. Use given_fields() for iteration and attribute access for named fields.
The ServiceSettings refactor (PR #3714) changed self._settings from dicts to dataclass subclasses, but tracing code still used .items(), in containment, and subscript access, causing AttributeError on every traced call. Use given_fields() for iteration and attribute access for named fields.
4 participants
Maybe the best way to understand these changes is to check out the COMMUNITY_INTEGRATIONS.md changes.
Does not (yet) touch
InputParams, to avoid scope creep, but what to do with those is an open question.InputParamsrepresents what you can specify at init time, and*Settingsrepresents what can (theoretically) be updated at runtime. These often overlap heavily. In the pattern introduced in this PR,self._settingsis the source of truth, and the init-timeInputParamsare used just to seedself._settings(which was kind of the pattern before, but never really formalized or adhered to especially strictly)NOTE: manual testing of every service in progress, there may be some more changes that creep in.