Conversation
| via ``STTUpdateSettingsFrame``), only the non-None fields of | ||
| ``live_options`` are merged into the stored options rather than | ||
| replacing them wholesale. | ||
| class LiveOptions: |
There was a problem hiding this comment.
@markbackman, @kompfner, Deepgram doesn’t provide this class anymore in the most recent version. Should we keep using it for backward compatibility?
There was a problem hiding this comment.
I'd say no. If Deepgram doesn't give us a type anymore to use for its connection-time parameters, then we might as well revert the decision made here and go back to having the (known) connection parameters reside directly as DeepgramSTTSettings fields and having any provided extra parameters get funneled into the connection kwargs. Having settings fields living at the top level of the settings objects rather than nested one layer in would be more standard, and would get rid of the concern, expressed here, that "it might be surprising/unexpected to the user if live_options (a nested field) was treated as a delta, and not swapped wholesale".
We of course have to be careful to map legacy-style dict settings updates to the modern settings object (see
)There was a problem hiding this comment.
Btw, not sure you saw but the last couple of commits in #3817 added some refactor/clean up changes which could make for a somewhat complicated rebase.
From that refactor it'd be nice to preserve the close sharing of logic between Deepgram STT and Deepgram SageMaker STT.
There was a problem hiding this comment.
Hi @kompfner , I have rebased now and applied all the changes here.
There was a problem hiding this comment.
Maybe the only reason to keep LiveOptions for now is backward compatibility. That said, I agree, regarding the settings, it would probably make sense to revert our decision.
There was a problem hiding this comment.
Interesting. Yes, if Deepgram broke their types, then we probably have to follow suit.
The other option is to move away from the SDK supported types and vend our own. We could keep the API more consistent that way, but it adds a bigger maintenance burden.
There was a problem hiding this comment.
Yep, now everything is passed directly inside connect, instead of inside live_options.
There was a problem hiding this comment.
backward compatibility
Ah right backward compatibility with __init__, not backward compatibility with DeepgramSTTSettings (the only backward compatibility we have to worry about there is with dict-based settings updates via STTSettingsUpdateFrame). Yeah, I guess we do have to worry about that...
...but, with the work that @markbackman and I were discussed on the call today, we will soon be able to mark the live_options init parameter deprecated, too, in favor of the new DeepgramSTTSettings.
There was a problem hiding this comment.
Ah right backward compatibility with init
Exactly.
|
|
||
| Parameters: | ||
| live_options: Deepgram ``LiveOptions`` for STT configuration. | ||
| live_options: class ``LiveOptions`` for STT configuration. |
There was a problem hiding this comment.
I know it's a bit of short-term (i.e. only until v1) pain, but: let's duplicate the LiveOptions fields here instead of nesting live_options. When we deprecate LiveOptions and the init-time live_options argument, DeepgramSTTSettings is what we'll be left with as the supported API.
There was a problem hiding this comment.
And when you do that, don't forget to also handle in the STT service logic the extra param in this object (they'll go in as extra connection kwargs).
There was a problem hiding this comment.
@kompfner is this what you had in mind:
I have pushed as a follow up PR so it is easy to review.
There was a problem hiding this comment.
That PR looks mostly good! Just had one comment about maybe not porting over all current LiveOptions fields to DeepgramSTTSettings.
Another thought, re:
When we deprecate
LiveOptionsand the init-timelive_optionsargument,DeepgramSTTSettingsis what we'll be left with as the supported API
...how do y'all feel about making this service the first one we migrate to the new pattern (init-time settings in favor of init-time live_options?) That way we can avoid having to maintain LiveOptions by marking it as deprecated right away. I think the changes we'd need are:
- marking
live_options(and the new shimLiveOptions) as deprecated - marking
voiceas deprecated - marking
encodingas deprecated
(Also happy to punt on this and stick to the plan of having that change come in a future (but near-future) PR, where we give a whole slew of services the same treatment.)
There was a problem hiding this comment.
I think it makes sense, since we’re already upgrading the Deepgram version and LiveOptions is no longer supported by Deepgram.
@markbackman, any thoughts on this?
There was a problem hiding this comment.
One option would be to wait to upgrade to Deepgram v6 until Pipecat 1.0 since it's also breaking. WDYT? That should be very soon.
There was a problem hiding this comment.
With this PR, the only breaking change so far, is that users who imported LiveOptions directly from deepgram, must now import it from pipecat: from pipecat.services.deepgram.stt import LiveOptions.
If we are happy with the results, I think it should be ok to already include these changes. This way we would not introduce another breaking change as soon as Pipecat 1.0 is released.
There was a problem hiding this comment.
This way we would not introduce another breaking change as soon as Pipecat 1.0 is released.
I think what @markbackman is proposing is skipping adding this breaking change to 0.0.104, and targeting 1.0 instead, not targeting post-1.0.
There was a problem hiding this comment.
Correct. Introducing the breaking change with 1.0.
Codecov Report❌ Patch coverage is
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| class TestDeepgramConnectKwargsPrecedence: |
There was a problem hiding this comment.
Given my last suggestion, I think we wouldn't need these tests.
To test direct field precedence over conflicting extra, you would test it instead in:
TestDeepgramSTTSettingsApplyUpdate- Bonus points (optional): a new test for init-time stt settings
kompfner
left a comment
There was a problem hiding this comment.
One final suggestion. Otherwise, LGTM!
I guess we'll hold off on merging until the final decision re whether to skip landing this in 0.0.104 and target 1.0 instead.
Also: I agree with punting on the deprecating live_options init param for now. Let's bundle that change with the bigger set of init signature refactors we have planned. Things will be slightly awkward in the short term (with DeepgramSTTSettings having slightly different ergonomics—with extra—than LiveOptions) but that's probably OK.
I guess this would be a small breaking change if we include it now. So my vote would be to merge it before 1.0. This way, we can validate all these changes already and ensure that 1.0 is completely stable. cc: @markbackman for thoughts on this. |
Nice, so, we're maintaining our own LiveOptions class now, which helps with compatibility. I guess the breaking change is isolated to where the LiveOptions are imported from; developers need to change from Deepgram to Pipecat. It's really not ideal to have a breaking change, but I'm not sure about how to avoid it. Services can break the API at any point in time and Pipecat can only do so much to keep up. I think in terms of versioning, we need to think of Pipecat's semantic versioning being core framework API and service API, not including service class init signatures, as we don't control when those break. WDYT? Is that a reasonable POV? If so, then we're probably good to make this change. (The only way I can think around this is to independently package and version the services as "plugins" but that's messy, too.) |
d0fee5c to
8b09f7b
Compare
|
Hi @markbackman, exactly, if the services break the API, there isn’t much we can do on the Pipecat side. Unless we never allow users to use any of the service APIs directly and instead wrap everything inside Pipecat. However, that would be a lot to maintain and probably not worth the benefits. And +1 on Pipecat’s semantic versioning covering both the core framework API and the service APIs. I think this makes sense. 👍 So that’s basically why I don’t think we need to hold this until 1.0, if we’re happy with the results. This way, we can validate all these changes early and ensure that 1.0 is completely stable. |
|
@kompfner, @markbackman, I’ve addressed all the comments and rebased on the latest changes from main, so this is ready to merge when we think the time is right. |
@filipi87 let's merge right after the 0.0.104 release. That will give us time to use the new SDK release before it becomes available in Pipecat. |
# Conflicts: # src/pipecat/services/deepgram/stt_sagemaker.py
Summary
deepgram-sdkfrom~=4.7.0to~=6.0.1LiveOptionscompatibility class inside pipecat (deepgram-sdk v6 removed it) so existing user code requiringLiveOptionscontinues to work without changesasync with client.listen.v1.connect()running in a single background task (_connection_handler), which handles the full WebSocket lifecycle including automatic reconnection after transient errorsKeepAlivemessages every 5 seconds — deepgram-sdk v6 removed the automatic keepalive that v4 provided viaoptions={"keepalive": "true"}, and Deepgram closes inactive connections after 10 seconds (NET-0001); this directly fixes keepalive ping timeout disconnectionssend()→send_media(),finalize()→send_finalize(),finish()→send_close_stream()LiveTranscriptionEventsis replaced by a singleEventType.MESSAGEhandler withisinstancechecks forListenV1Results,ListenV1SpeechStarted, andListenV1UtteranceEndDeepgramSageMakerSTTServiceto importLiveOptionsfrom the new pipecat-internal locationBreaking Changes
LiveOptionsdirectly fromdeepgram(from deepgram import LiveOptions) must now import it from pipecat:from pipecat.services.deepgram.stt import LiveOptions