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

Allow `wss://` in the onprem URL. #360

Closed jjmaldonis closed 2 months ago

jjmaldonis commented 2 months ago

What is the current behavior?

The Python SDK requires the onprem URL to be https://... and if the user tries to set it as wss://... because they are using streaming, the connection fails.

Steps to reproduce

Set the URL to wss://... rather than https://.... You can use wss://api.deepgram.com rather than an onprem URL to reproduce.

Expected behavior

The wss protocol should be accepted, in addition to https.

Please tell us about your environment

Reproducible in any env.

Other information

dvonthenen commented 2 months ago

You should be able to set the URL to wss://<your IP or domain>. If you omit the protocol, the default https is added because the config object doesn't know which API the object is destined for.

Only options would be to:

dvonthenen commented 2 months ago

We could do a combination of those options.

Longer term, this might be more sustainable.

The reason why I separated the classes out because it implies the options are the same for both REST and WS connections and that it's true. For example, KeepAlive can be set and passed into PreRecorded Client but it doesn't do/mean anything.

jjmaldonis commented 2 months ago

You should be able to set the URL to wss://.

This is not possible right now. I installed the latest version, published 45 min ago, and tested:

config: DeepgramClientOptions = DeepgramClientOptions(
    url="wss://api.deepgram.com",
)

deepgram: DeepgramClient = DeepgramClient(API_KEY, config)

This code returns an error. Replacing wss with https works!

Why can't we just check for wss and handle that edge case in a 2-line if statement?

jjmaldonis commented 2 months ago

The reason why I separated the classes out because it implies the options are the same for both REST and WS connections and that it's true. For example, KeepAlive can be set and passed into PreRecorded Client but it doesn't do/mean anything.

I think we're talking about different things here. The DeepgramClientOptions class is a higher level class than the streaming/prerecorded configuration classes. I'm pretty sure a short 2-line if statement will work.

dvonthenen commented 2 months ago

Yea, we are talking about the same thing. DeepgramClientOptions can be passed into LiveClient and PreRecorded/Speak/Manage/Analyze Clients.

dvonthenen commented 2 months ago

Will have something in a bit

dvonthenen commented 2 months ago

this slipped my mind... will have something for tomorrow. The workaround is to also supply the protocol with the hostname or IP which we discussed in slack (I believe 🧐)

dvonthenen commented 2 months ago

This fix is available in: https://github.com/deepgram/deepgram-python-sdk/releases/tag/v3.2.7