Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
jjmaldonis opened this issue Apr 8, 2024 · 8 comments
Closed

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

jjmaldonis opened this issue Apr 8, 2024 · 8 comments
Assignees

Comments

@jjmaldonis
Copy link
Contributor

jjmaldonis commented Apr 8, 2024

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

@davidvonthenen
Copy link
Contributor

davidvonthenen commented Apr 8, 2024

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:

  • create an HTTP and WS specific ClientOptions classes similar to what I did in .NET (this will break backward compatibility). this explicitly then states which connection you intent to use
  • or don't append the https and just error out if http(s) or ws(s) is missing

@davidvonthenen
Copy link
Contributor

davidvonthenen commented Apr 8, 2024

We could do a combination of those options.

  • create DeepgramWsClientOptions and DeepgramHttpClientOptions classes
  • keep the older DeepgramClientOptions and remove appending https to preserve the backwards compatibility

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

@davidvonthenen
Copy link
Contributor

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

@davidvonthenen
Copy link
Contributor

Will have something in a bit

@davidvonthenen davidvonthenen self-assigned this Apr 8, 2024
@davidvonthenen
Copy link
Contributor

davidvonthenen commented Apr 17, 2024

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 🧐)

@davidvonthenen
Copy link
Contributor

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

No branches or pull requests

2 participants