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

fix(azure/audio): use model param for deployments #1297

Merged
merged 3 commits into from
Feb 3, 2025

Conversation

deyaaeldeen
Copy link
Contributor

Issue: #1289

The buildRequest method in AzureOpenAI is overridden to construct the Azure request URL using the deployment name, which can be set either on the client or in the request body. However, for audio operations, the request body is converted into a stream before buildRequest is called, making the deployment information inaccessible.

This PR resolves the issue by intercepting calls to the underlying create methods and capturing the deployment value before the request body is converted into a stream.

@deyaaeldeen deyaaeldeen requested a review from a team as a code owner January 31, 2025 21:43
@RobertCraigie
Copy link
Collaborator

Ah I see why this is difficult, I think instead we could retain the model information by passing it down as a separate request option. e.g. adding a _metadata?: Record<string, unknown> property, then instead of having to override the resource classes for the azure client we could just specify that in the main resource class, e.g.

    return this._client.post('/audio/transcriptions', Core.multipartFormRequestOptions({ body, ...options, _metadata: { model: body.model } }));

I think this is cleaner and I suspect your implementation won't work for retries fwiw

@deyaaeldeen
Copy link
Contributor Author

@RobertCraigie good point, addressed in 6a39d7e. I was initially hesitant to edit code under resources because I think it is auto-generated.

Speaking of retries, I think we're in a tricky situation there when the user passes in a stream and it gets exhausted in the initial request. When it is time to retry to the request (let's say because of a 429), the request gets stuck and nothing is sent to the server because the stream is already exhausted. To get a round this, we need to require the input to be a stream factory that can reproduce the stream between retries. Is there any chance we can address this in the upcoming v5?

@RobertCraigie
Copy link
Collaborator

I was initially hesitant to edit code under resources because I think it is auto-generated.

Ah yeah that used to be a concern but now we can make arbitrary changes to the generated code :)

Speaking of retries, I think we're in a tricky situation there when the user passes in a stream and it gets exhausted in the initial request. When it is time to retry to the request (let's say because of a 429), the request gets stuck and nothing is sent to the server because the stream is already exhausted. To get a round this, we need to require the input to be a stream factory that can reproduce the stream between retries. Is there any chance we can address this in the upcoming v5?

Ah..... thanks for the report, I don't think we would want to require a stream that can be retried but we definitely need to improve the DX here and provide some solution, we'll investigate, thanks!

Copy link
Collaborator

@RobertCraigie RobertCraigie left a comment

Choose a reason for hiding this comment

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

Looks great!

One last question, is this a breaking change? I think so?

@deyaaeldeen
Copy link
Contributor Author

we'll investigate, thanks!

Awesome, thanks!

is this a breaking change?

It shouldn't break existing working code that I know of, the order of places to get the deployment value from didn't change.

@RobertCraigie RobertCraigie changed the title [Azure] Use deployment in the request body for audio operations fix(azure/audio): use model param for deployments Feb 3, 2025
@RobertCraigie RobertCraigie changed the base branch from master to next February 3, 2025 17:21
@RobertCraigie RobertCraigie merged commit 85de382 into openai:next Feb 3, 2025
3 checks passed
@stainless-app stainless-app bot mentioned this pull request Feb 3, 2025
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