search

deepgram / deepgram-python-sdk

Official Python SDK for Deepgram's automated speech recognition APIs.
https://developers.deepgram.com
MIT License
178 stars 48 forks source link

Keepalive thread throws errors even when the "keepalive" option is not set #344

Closed roerohan closed 3 months ago

roerohan commented 3 months ago

In AsyncLiveClient, the following code snippet checks if the keepalive option is set to true:

https://github.com/deepgram/deepgram-python-sdk/blob/2d0ddf12935c8b00136883689d4674536e6c01f8/deepgram/clients/live/v1/async_client.py#L292-L297

However, even if keepalive is not set in the options, or set to something other than true, the _keep_alive_thread is still created.

https://github.com/deepgram/deepgram-python-sdk/blob/2d0ddf12935c8b00136883689d4674536e6c01f8/deepgram/clients/live/v1/async_client.py#L111

Now, if the _keep_alive_thread exits for some reason, an exception is thrown that can not be handled.

This was causing a problem in v3.1.5 as the following would throw an error if the socket connection closed, even if _keep_alive was not set to true. https://github.com/deepgram/deepgram-python-sdk/blob/f0ed1ec0a14acf56a9ef73aefebf55f7ea65cfd5/deepgram/clients/live/v1/async_client.py#L252

Even in the newer versions, if there is an exception raised from this _keep_alive_thread coroutine, there's no way to handle the exception and it will flood the user's logs with

Task exception was never retrieved
future: <Task finished name='Task-277' coro=<AsyncLiveClient._keep_alive() done, defined at /usr/local/lib/python3.12/dist-packages/deepgram/clients/live/v1/async_client.py:228> exception="...">
Traceback (most recent call last):
...

Also, if keepalive is not set to true in options, it's possible to avoid running a coroutine with while True. Is it possible for you to check the DeepgramClientOptions before creating the _keep_alive_thread coroutine, or is there something I'm missing?

roerohan commented 3 months ago

Also, in v3.2.1, there's no way for me to handle exceptions raised by the send method, because they're caught within the SDK and logged, as you can see here https://github.com/deepgram/deepgram-python-sdk/blob/2d0ddf12935c8b00136883689d4674536e6c01f8/deepgram/clients/live/v1/async_client.py#L358-L361

Now, let's say I want to reconnect if there's an exception in self._socket.send, there's no way for me to do it using the SDK, other than using self._socket.send directly instead of AsyncLiveClient.send, which beats the purpose of the class.

dvonthenen commented 3 months ago

Ooops. That's definitely a bug after the refactor. We should not be kicking off the thread off if the keepalive isn't enabled. Will also toggle post an error on the callback which is missing and provide a switch to handle the exception yourself.

Will have that fixed this morning.

dvonthenen commented 3 months ago

hi @roerohan

This should address your issue: https://github.com/deepgram/deepgram-python-sdk/releases/tag/v3.2.2

If you want to enable passing the exceptions up, there are options to do that now on top of fixing the keepalive issue. If you have any problems with this, catch me in discord for real-time help.

roerohan commented 3 months ago

Thanks! I'll take a look at it after some time.

Also, I noticed a minor mistake in the logs, so I created a PR #347

dvonthenen commented 3 months ago

Here are some examples to demonstrate expected failures when not using and also how to utilize the feature correctlyhttps://github.com/deepgram/deepgram-python-sdk/pull/348