Implement inference service (semantic reranking) into new azure-cosmos-ai module#48258
Implement inference service (semantic reranking) into new azure-cosmos-ai module#48258mbhaskar wants to merge 6 commits into
Conversation
aayush3011
left a comment
There was a problem hiding this comment.
Overall it looks good, left a few comments.
aayush3011
left a comment
There was a problem hiding this comment.
Deep Review — Semantic Reranking InferenceService
Summary: This PR adds semantic reranking via a new InferenceService class, with semanticRerank() on both CosmosAsyncContainer and CosmosContainer, retry logic, and unit tests. The core functionality is solid and the 22 prior review comments appear addressed. Below are new findings from a deeper review pass.
| Severity | Count | Areas |
|---|---|---|
| 🔴 Blocking | 3 | Resource leak, NPE, misleading Javadoc |
| 🟡 Recommendation | 5 | API surface, validation, User-Agent, sample, test leak |
| 🟢 Suggestion | 1 | Option constants |
| 💬 Observation | 1 | Minor consolidation |
Both CosmosAsyncContainer.semanticRerank and CosmosContainer.semanticRerank had @beta(value = Beta.SinceVersion.V4_78_0) but were missing the required warningText = Beta.PREVIEW_SUBJECT_TO_CHANGE_WARNING parameter. Addresses PR review comment: Azure#48258 (comment) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ack addressed) Squashed rebase of PR Azure#48258 onto upstream/main. Addresses outstanding review comments from kushagraThapar and FabianMeiswinkel: - Configs.java: INFERENCE_ENDPOINT_PROPERTY / INFERENCE_ENDPOINT_ENVIRONMENT_VARIABLE are now private (single source of truth, with comment); removed unused getDefaultInferenceServiceConfig(). - InferenceService.java: consolidated duplicate MAX_CONNECTION_LIMIT constant (MAX_CONNECTION_LIMIT_VARIABLE = MAX_CONNECTION_LIMIT_PROPERTY); rewrote withRetry to honour the server-supplied Retry-After header on 429 (delay-seconds form per RFC 7231, capped at RETRY_MAX_BACKOFF), falling back to exponential backoff with jitter — matches Python SDK behaviour. - SemanticRerankResult / SemanticRerankScore: removed @JsonProperty annotations (POJOs are populated via ModelBridgeInternal setters, not via Jackson deserialization). - SemanticRerankSample: removed import of internal InferenceService; redefined option-key constants as raw string literals locally in the sample. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
d850479 to
0976160
Compare
…n bump - Bump @beta annotations from V4_78_0 to V4_81_0 to match the current unreleased version (4.81.0-beta.1). - Use Preconditions.checkNotNull / checkArgument in CosmosAsyncContainer.semanticRerank for consistency with the rest of the class (addresses kushagraThapar feedback). - Fix duplicate constant strings in Configs and InferenceService: system property uses dot.notation and environment variable uses UPPER_SNAKE_CASE, matching the existing convention in Configs. - Replace the new ModelBridgeInternal bridge methods with the standard ImplementationBridgeHelpers accessor pattern for SemanticRerankResult and SemanticRerankScore. ModelBridgeInternal.java now has zero diff vs main (addresses FabianMeiswinkel API-review feedback). Response POJO setters are now private. - Update CHANGELOG to reflect the correct system property name versus environment variable name and to note that Retry-After is honoured on 429. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Update test constant INFERENCE_ENDPOINT_PROPERTY to match the new system-property name ('azure.cosmos.semanticReranker.inferenceEndpoint') used by Configs.getInferenceServiceEndpoint(). The previous value was the env-variable name and no longer matched after the property/env constants were split.
- Add input validation in InferenceService.semanticRerank(): synchronously throw NPE for null rerankContext/documents and emit Mono.error(IllegalArgumentException) for empty values. This restores the contract the unit tests assert and provides defense-in-depth alongside the container-level checks.
- Register SemanticRerankResult and SemanticRerankScore accessors via ModelBridgeInternal.initializeAllAccessors() so the ImplementationBridgeHelpers accessor pattern works when classes are first reached via the helper (fixes NPE 'scoreAccessor is null' in semanticRerankShouldIncludeAuthorizationHeader).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…filter
The unit-testng.xml suite filters tests to the 'unit' group. TestNG excludes
@BeforeMethod / @AfterMethod configuration methods that are not in an
included group (and don't have alwaysRun=true), so the bare @BeforeMethod
and @AfterMethod annotations were skipped in CI.
This caused mockTokenCredential to remain null, producing NPEs at
'new InferenceService(mockTokenCredential)' with the message 'Semantic
reranking requires AAD authentication' for all 28 tests that depend on the
mocked credential.
Annotate @BeforeMethod/@AfterMethod with groups = {"unit"} to match the
rest of the codebase (RxPartitionKeyRangeCacheTest, SqlQuerySpecLoggerTest,
etc.) and ensure the setup/teardown fire under the 'unit' group filter.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kushagraThapar
left a comment
There was a problem hiding this comment.
Thanks for the iteration here — caught up on the diff and verified that 41 of the 46 prior inline comments are fully resolved at HEAD 228b26965a7, including all the headline correctness items from earlier rounds (lazy-init race, missing close(), retry policy, AAD enforcement, leaked test executor, options key constants, etc.). Nice work landing those.
I have three correctness items I'd like to flag inline before this lands — all in the retry / lifecycle area. Happy to discuss any of them.
Three small but correctness-relevant fixes raised by @kushagraThapar: 1. InferenceService: payload serialization (Jackson) failures are now surfaced as BadRequestException(400) with sub-status CUSTOM_SERIALIZER_EXCEPTION instead of a generic CosmosException(500). 400 is not in RETRYABLE_STATUS_CODES, so the retry policy no longer wastes attempts on a deterministic payload failure. Matches the pattern already used for response-parse failures. 2. CosmosContainer.semanticRerank (sync): apply timeout_seconds as a whole-operation deadline via Mono.timeout(...) and map java.util.concurrent.TimeoutException to RequestTimeoutException (408) so callers see a typed CosmosException instead of Reactor's raw RuntimeException("Timeout on blocking read"). Javadoc updated to document the sync-vs-async timeout semantics. 3. CosmosAsyncClient: fix a race between close() and lazy initialization of InferenceService that could permanently leak a Netty event-loop group and pooled connections. close() now sets a volatile closed flag first, then atomically transfers the reference via getAndSet(null). getOrCreateInferenceService re-checks closed after a successful CAS and tears down any late-arriving instance, raising IllegalStateException. CHANGELOG entries added for all three under 4.81.0-beta.1. Verified: mvn install on azure-cosmos, checkstyle+spotbugs clean, InferenceServiceTest 29/29 passing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Create new standalone Maven module sdk/cosmos/azure-cosmos-ai with: - CosmosAIClientBuilder, CosmosAIAsyncClient, CosmosAIClient public API - InferenceService rewritten to use azure-core HttpPipeline - SemanticRerankResult/SemanticRerankScore models with package-private setters - InferenceResponseParser for JSON parsing (in models package) - Unit tests (22 passing) and sample code Remove all semantic rerank code from azure-cosmos: - CosmosAsyncContainer.semanticRerank() and CosmosContainer.semanticRerank() - CosmosAsyncClient.getOrCreateInferenceService() and close() cleanup - ImplementationBridgeHelpers SemanticRerankResult/ScoreHelper classes - ModelBridgeInternal.initialize() calls for SemanticRerank types - Configs.java inference endpoint properties - Old InferenceService, model classes, test, and sample files
|
|
||
| #### Breaking Changes | ||
|
|
||
| #### Bugs Fixed | ||
| * Semantic rerank: serialization failures are now surfaced as a typed `BadRequestException` (HTTP 400, sub-status `CUSTOM_SERIALIZER_EXCEPTION`) instead of a generic `500`, so the SDK retry policy does not retry deterministic payload failures. - [PR 48258](https://github.com/Azure/azure-sdk-for-java/pull/48258) |
There was a problem hiding this comment.
The four entries added to 4.81.0-beta.1 describe the prior in-tree design that was deleted later in this same PR (commit f405944):
Markdown
- Added beta
semanticRerankAPI onCosmosAsyncContainerandCosmosContainer… - Semantic rerank: serialization failures are now surfaced as a typed
BadRequestException(HTTP 400, sub-statusCUSTOM_SERIALIZER_EXCEPTION) … - Semantic rerank: the synchronous
CosmosContainer.semanticRerankAPI now appliestimeout_secondsas a whole-operation deadline and maps Reactor's blocking timeout toRequestTimeoutException(HTTP 408) … - Semantic rerank: fixed a race between
CosmosAsyncClient.close()and lazy initialization of the inference HTTP client …
None of these are true at the head SHA:
There is no semanticRerank method on CosmosAsyncContainer / CosmosContainer (grep confirms).
The new azure-cosmos-ai InferenceService throws IllegalArgumentException on serialization failure, not BadRequestException(400, CUSTOM_SERIALIZER_EXCEPTION).
CosmosAIClient.semanticRerank calls Mono.block(timeout), which throws IllegalStateException, not RequestTimeoutException(408).
CosmosAsyncClient.close() no longer manages an inference HTTP client at all (the lazy-init path was deleted with the old design).
There was a problem hiding this comment.
AI quirks! Will refresh the changelong once I get the changes approved
| * | ||
| * @param args command line arguments (not used). | ||
| */ | ||
| public static void main(String[] args) { |
There was a problem hiding this comment.
Is this main() functional i.e. usable by customer for async semantics?
| <dependencies> | ||
| <dependency> | ||
| <groupId>com.azure</groupId> | ||
| <artifactId>azure-core</artifactId> |
There was a problem hiding this comment.
Do we need to add explicit Jackson dependency in this package or we rely on azure-core's transitive version?
The reason I ask is InferenceService.java, InferenceResponseParser.java
There was a problem hiding this comment.
Tests on the Public API builder surface?
- CosmosAIClientBuilder endpoint resolution priority (explicit > sysprop > env > error)
- Missing-credential error path
- Custom pipeline() overriding credential() / httpClient() / additionalPolicies
- InferenceResponseParser edge cases (missing Scores, malformed score entries with missing index/score, empty body)
- CosmosAIClient (sync) block(timeout) behavior
Review —
|
Suggestion: have
|
| Library | How it takes the client |
|---|---|
| azure-spring-data-cosmos (this repo) | CosmosFactory(CosmosAsyncClient cosmosAsyncClient, String databaseName) |
Spring AI — CosmosDBVectorStore |
CosmosDBVectorStore.builder(CosmosAsyncClient cosmosClient, EmbeddingModel embeddingModel) |
| Spring AI — Cosmos chat-memory repository | CosmosDBChatMemoryRepositoryConfig.Builder.withCosmosClient(CosmosAsyncClient cosmosClient) |
LangChain4j — AzureCosmosDbNoSqlEmbeddingStore |
.builder().cosmosClient(CosmosAsyncClient cosmosClient) |
Suggested approach: keep azure-cosmos-ai as a separate module, but have the builder accept the user's existing client so it can reuse the already-configured credential, pipeline, and endpoint resolution:
CosmosAIAsyncClient ai = new CosmosAIClientBuilder()
.cosmosAsyncClient(existingClient) // reuse credential + pipeline
.buildAsyncClient();
ai.semanticRerank(query, documents);This keeps the dependency-isolation and independent-versioning benefits that motivated the separate module, while restoring the "wrap the existing CosmosClient" convention the rest of the Java ecosystem follows — so customers configure one credential, in one place. It would require azure-cosmos to expose the credential/pipeline to the companion module via a bridge, which is an established pattern in this codebase.
Two smaller, related notes:
- The
endpoint()+credential()+ system-property / env-var fallback resolution is largely re-deriving configuration the Cosmos client already holds; wrapping the client removes most of that surface (and the associated tests).
Summary
Implements the semantic reranking/inference functionality ina new standalone module
azure-cosmos-ai(com.azure:azure-cosmos-ai:1.0.0-beta.1) with packagecom.azure.cosmos.ai.Motivation
The semantic reranking feature uses an external inference endpoint (not the Cosmos DB service itself) and relies on AAD authentication. Keeping it as a separate module:
azure-coreHTTP pipeline (standard policies, retry, logging) instead of Cosmos-internal HTTP primitivesChanges
New module:
sdk/cosmos/azure-cosmos-aiCosmosAIClientBuilderendpoint(),credential(),pipeline(),httpClient(),addPolicy()CosmosAIAsyncClientsemanticRerank()returningMono<SemanticRerankResult>CosmosAIClientInferenceServiceHttpPipelinewith retry logic (429/5xx, exponential backoff)SemanticRerankResult/SemanticRerankScoreInferenceResponseParserThis is a change from earlier implemented design in
azure-cosmos. So removed the below which were part of earlier commitsCosmosAsyncContainer.semanticRerank()andCosmosContainer.semanticRerank()CosmosAsyncClient.getOrCreateInferenceService()and inference cleanup inclose()ImplementationBridgeHelpers.SemanticRerankResultHelper/SemanticRerankScoreHelperModelBridgeInternalinitialize calls for semantic rerank typesConfigs.javainference endpoint propertiesInferenceService, model classes, test, and sample filesHow to use the new module
Maven dependency
Quick start (async)
Quick start (sync)
Endpoint resolution
The endpoint can be provided via (in priority order):
builder.endpoint("...")— explicitazure.cosmos.semanticReranker.inferenceEndpointAZURE_COSMOS_SEMANTIC_RERANKER_INFERENCE_ENDPOINTTesting
Verification