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

[Azure] Support Realtime API - Standalone client #1283

Closed
wants to merge 1 commit into from

Conversation

deyaaeldeen
Copy link
Contributor

Support Azure Realtime API

In order to do so, this PR adds new AzureOpenAIRealtimeWebSocket and AzureOpenAIRealtimeWS clients

Key Changes

1. Introduce New open Method

  • Previously, the WebSocket connection was established in the class constructor, which could not handle async operations like fetching Azure AD tokens.
  • The new open method enables asynchronous fetching of the Azure AD token and establishes the WebSocket connection afterward.

2. New getAzureADToken Method in AzureOpenAI

  • Adds a getAzureADToken method in the AzureOpenAI class to provide refreshed Azure AD tokens.
  • The new method is called in the new clients to retrieve necessary Azure AD token for establishing the WebSocket connection

@deyaaeldeen deyaaeldeen marked this pull request as ready for review January 23, 2025 20:29
@deyaaeldeen deyaaeldeen requested a review from a team as a code owner January 23, 2025 20:29
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think these examples should mention realtime in the path somewhere, e.g.

  • examples/realtime/azure/websocket.ts
  • examples/azure/realtime/websocket.ts
  • examples/azure/realtime-websocket.ts
    (applies to the ws.ts example as well)

@@ -2,6 +2,7 @@

import { AzureOpenAI } from 'openai';
import { getBearerTokenProvider, DefaultAzureCredential } from '@azure/identity';
import 'dotenv/config';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think it'd be better to reduce the amount of deps a user would need to add if they copy-paste this example file.

I don't feel super strongly here though, and dotenv is very standard... maybe @kwhinnery-openai has thoughts?

const rt = new AzureOpenAIRealtimeWebSocket(client);
await rt.open();

// access the underlying `ws.WebSocket` instance
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// access the underlying `ws.WebSocket` instance
// access the underlying `WebSocket` instance

}

async open(): Promise<void> {
async function getUrl({
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can you move this function outside of this class? would be much easier to read

if (model !== undefined && !this.baseURL.includes('/deployments')) {
options.path = `/deployments/${model}${options.path}`;
}
}
return super.buildRequest(options, props);
}

private async _getAzureADToken(): Promise<string | undefined> {
async getAzureADToken(): Promise<string | undefined> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: do you think this is helpful to expose to users and something we should make part of the public API?

If not and this is only public for usage in the realtime client implementation then we should at least keep the _ prefix or keep it private and just ts-ignore at the call-site

@@ -95,3 +95,106 @@ export class OpenAIRealtimeWebSocket extends OpenAIRealtimeEmitter {
}
}
}

export class AzureOpenAIRealtimeWebSocket extends OpenAIRealtimeEmitter {
Copy link
Collaborator

@RobertCraigie RobertCraigie Jan 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: does the Azure API support ephemeral session tokens? Asking as the OpenAI API client requires dangerouslyAllowBrowser: true if you're not using an ephemeral session token

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kwhinnery-openai curious if you have any opinions on the location of the azure classes, is it fine / good for them to be in the same file as the OpenAI ones?

It does feel a bit verbose to have to do import { AzureOpenAIRealtimeWebSocket } from 'openai/beta/realtime/azure/websocket';, but there are a couple advantages I can see to splitting them up is that:

  • very small reduction in bundle size for non-azure users
  • it would be easier to add optional peer dependencies for azure specific things down the line if needed

@deyaaeldeen
Copy link
Contributor Author

Based on feedback in the channel, I opened #1287 instead.

@deyaaeldeen deyaaeldeen deleted the feat/azure-rt-clients branch January 24, 2025 18:57
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

Successfully merging this pull request may close these issues.

2 participants