Skip to content

Commit 38694bb

Browse files
jpalvarezljpalvarezl
committed
PR feedback 1st round
1 parent 67c9970 commit 38694bb

File tree

5 files changed

+48
-26
lines changed

5 files changed

+48
-26
lines changed

‎sdk/ai/azure-ai-agents/src/main/java/com/azure/ai/agents/AgentsClientBuilder.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -375,6 +375,7 @@ private OpenAIOkHttpClient.Builder getOpenAIClientBuilder() {
375375
builder.azureServiceVersion(AzureOpenAIServiceVersion.fromString(this.serviceVersion.getVersion()));
376376
builder.azureUrlPathMode(AzureUrlPathMode.UNIFIED);
377377
}
378+
// We set the builder retries to 0 to avoid conflicts with the retry policy added through the HttpPipeline.
378379
builder.maxRetries(0);
379380
return builder;
380381
}
@@ -388,6 +389,7 @@ private OpenAIOkHttpClientAsync.Builder getOpenAIAsyncClientBuilder() {
388389
builder.azureServiceVersion(AzureOpenAIServiceVersion.fromString(this.serviceVersion.getVersion()));
389390
builder.azureUrlPath(AzureUrlPathMode.UNIFIED);
390391
}
392+
// We set the builder retries to 0 to avoid conflicts with the retry policy added through the HttpPipeline.
391393
builder.maxRetries(0);
392394
return builder;
393395
}

‎sdk/ai/azure-ai-agents/src/main/java/com/azure/ai/agents/implementation/http/HttpClientHelper.java‎

Lines changed: 34 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import java.util.List;
3434
import java.util.Objects;
3535
import java.util.concurrent.CompletableFuture;
36+
import java.util.concurrent.TimeoutException;
3637

3738
/**
3839
* Utility entry point that adapts an Azure {@link com.azure.core.http.HttpClient} so it can be consumed by
@@ -83,14 +84,8 @@ public HttpResponse execute(HttpRequest request, RequestOptions requestOptions)
8384

8485
com.azure.core.http.HttpRequest azureRequest = buildAzureRequest(request);
8586

86-
Context requestContext = Context.NONE;
87-
Timeout timeout = requestOptions.getTimeout();
88-
if (timeout != null && !timeout.read().isZero() && !timeout.read().isNegative()) {
89-
requestContext = requestContext.addData("azure-response-timeout", timeout.read());
90-
}
91-
92-
com.azure.core.http.HttpResponse azureResponse = this.httpPipeline.sendSync(azureRequest, requestContext);
93-
return new AzureHttpResponseAdapter(azureResponse);
87+
return new AzureHttpResponseAdapter(
88+
this.httpPipeline.sendSync(azureRequest, buildRequestContext(requestOptions)));
9489
}
9590

9691
@Override
@@ -105,13 +100,7 @@ public CompletableFuture<HttpResponse> executeAsync(HttpRequest request, Request
105100

106101
final com.azure.core.http.HttpRequest azureRequest = buildAzureRequest(request);
107102

108-
Context requestContext = new Context("azure-eagerly-read-response", true);
109-
Timeout timeout = requestOptions.getTimeout();
110-
if (timeout != null && !timeout.read().isZero() && !timeout.read().isNegative()) {
111-
requestContext = requestContext.addData("azure-response-timeout", timeout.read());
112-
}
113-
114-
return this.httpPipeline.send(azureRequest, requestContext)
103+
return this.httpPipeline.send(azureRequest, buildRequestContext(requestOptions))
115104
.map(response -> (HttpResponse) new AzureHttpResponseAdapter(response))
116105
.onErrorMap(HttpClientWrapper::mapAzureExceptionToOpenAI)
117106
.toFuture();
@@ -128,26 +117,31 @@ private static Throwable mapAzureExceptionToOpenAI(Throwable throwable) {
128117
HttpResponseException httpResponseException = (HttpResponseException) throwable;
129118
int statusCode = httpResponseException.getResponse().getStatusCode();
130119
Headers headers = toOpenAIHeaders(httpResponseException.getResponse().getHeaders());
131-
Throwable cause = httpResponseException.getCause();
132120

133121
switch (statusCode) {
134122
case 400:
135-
return BadRequestException.builder().headers(headers).cause(cause).build();
123+
return BadRequestException.builder().headers(headers).cause(httpResponseException).build();
136124

137125
case 401:
138-
return UnauthorizedException.builder().headers(headers).cause(cause).build();
126+
return UnauthorizedException.builder().headers(headers).cause(httpResponseException).build();
139127

140128
case 403:
141-
return PermissionDeniedException.builder().headers(headers).cause(cause).build();
129+
return PermissionDeniedException.builder()
130+
.headers(headers)
131+
.cause(httpResponseException)
132+
.build();
142133

143134
case 404:
144-
return NotFoundException.builder().headers(headers).cause(cause).build();
135+
return NotFoundException.builder().headers(headers).cause(httpResponseException).build();
145136

146137
case 422:
147-
return UnprocessableEntityException.builder().headers(headers).cause(cause).build();
138+
return UnprocessableEntityException.builder()
139+
.headers(headers)
140+
.cause(httpResponseException)
141+
.build();
148142

149143
case 429:
150-
return RateLimitException.builder().headers(headers).cause(cause).build();
144+
return RateLimitException.builder().headers(headers).cause(httpResponseException).build();
151145

152146
case 500:
153147
case 502:
@@ -156,16 +150,18 @@ private static Throwable mapAzureExceptionToOpenAI(Throwable throwable) {
156150
return InternalServerException.builder()
157151
.statusCode(statusCode)
158152
.headers(headers)
159-
.cause(cause)
153+
.cause(httpResponseException)
160154
.build();
161155

162156
default:
163157
return UnexpectedStatusCodeException.builder()
164158
.statusCode(statusCode)
165159
.headers(headers)
166-
.cause(cause)
160+
.cause(httpResponseException)
167161
.build();
168162
}
163+
} else if (throwable instanceof TimeoutException) {
164+
return throwable;
169165
} else {
170166
return new OpenAIException(throwable.getMessage(), throwable.getCause());
171167
}
@@ -226,5 +222,19 @@ private static HttpHeaders toAzureHeaders(Headers sourceHeaders) {
226222
return target;
227223
}
228224

225+
/**
226+
* Builds the request context from the given request options.
227+
* @param requestOptions OpenAI SDK request options
228+
* @return Azure request {@link Context}
229+
*/
230+
private static Context buildRequestContext(RequestOptions requestOptions) {
231+
Context context = new Context("azure-eagerly-read-response", true);
232+
Timeout timeout = requestOptions.getTimeout();
233+
// we use "read" as it's the closes thing to the "response timeout"
234+
if (timeout != null && !timeout.read().isZero() && !timeout.read().isNegative()) {
235+
context = context.addData("azure-response-timeout", timeout.read());
236+
}
237+
return context;
238+
}
229239
}
230240
}

‎sdk/ai/azure-ai-agents/src/main/java/com/azure/ai/agents/implementation/http/OpenAiRequestUrlBuilder.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@
1919
* Utility methods that reconstruct the absolute {@link URL} required by the Azure pipeline from the
2020
* OpenAI request metadata. The builder keeps the low-level path/query handling isolated so that
2121
* {@link HttpClientHelper} can focus on the higher-level request mapping logic.
22+
* This class will be deprecated as soon as support is added in the OpenAI SDK as described in this issue:
23+
* <a href="https://github.com/openai/openai-java/issues/660"/>
2224
*/
2325
public final class OpenAiRequestUrlBuilder {
2426

@@ -30,6 +32,8 @@ private OpenAiRequestUrlBuilder() {
3032
/**
3133
* Builds an absolute {@link URL} using the base URL, path segments, and query parameters that are stored in the
3234
* OpenAI {@link HttpRequest} abstraction.
35+
* This method will be deprecated as soon as support is added in the OpenAI SDK as described in this issue:
36+
* <a href="https://github.com/openai/openai-java/issues/660"/>
3337
*
3438
* @param request Source request provided by the OpenAI client.
3539
* @return Absolute URL that can be consumed by Azure HTTP components.

‎sdk/ai/azure-ai-agents/src/test/java/com/azure/ai/agents/ConversationsAsyncTests.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121

2222
import java.time.Duration;
2323
import java.util.concurrent.ExecutionException;
24+
import java.util.concurrent.TimeoutException;
2425

2526
import static com.azure.ai.agents.TestUtils.DISPLAY_NAME_WITH_ARGUMENTS;
2627
import static org.junit.jupiter.api.Assertions.*;
@@ -143,6 +144,8 @@ public void timeoutResponse(HttpClient httpClient, AgentsServiceVersion serviceV
143144
RequestOptions requestOptions
144145
= RequestOptions.builder().timeout(Timeout.builder().read(Duration.ofMillis(1)).build()).build();
145146

146-
assertThrows(Exception.class, () -> client.getConversationServiceAsync().create(requestOptions).get());
147+
ExecutionException thrown = assertThrows(ExecutionException.class,
148+
() -> client.getConversationServiceAsync().create(requestOptions).get());
149+
assertInstanceOf(TimeoutException.class, thrown.getCause());
147150
}
148151
}

‎sdk/ai/azure-ai-agents/src/test/java/com/azure/ai/agents/ConversationsTests.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
import org.junit.platform.commons.util.StringUtils;
1616

1717
import java.time.Duration;
18+
import java.util.concurrent.TimeoutException;
1819

1920
import static com.azure.ai.agents.TestUtils.DISPLAY_NAME_WITH_ARGUMENTS;
2021
import static org.junit.jupiter.api.Assertions.*;
@@ -126,6 +127,8 @@ public void timeoutResponse(HttpClient httpClient, AgentsServiceVersion serviceV
126127

127128
RequestOptions requestOptions
128129
= RequestOptions.builder().timeout(Timeout.builder().read(Duration.ofMillis(1)).build()).build();
129-
assertThrows(Exception.class, () -> client.getConversationService().create(requestOptions));
130+
RuntimeException thrown
131+
= assertThrows(RuntimeException.class, () -> client.getConversationService().create(requestOptions));
132+
assertInstanceOf(TimeoutException.class, thrown.getCause());
130133
}
131134
}

0 commit comments

Comments
 (0)