Conversation
6f21bcd to
06d15e8
Compare
wenjin272
left a comment
There was a problem hiding this comment.
Thanks for adding the Java OpenAI embedding model and for the thorough tests.
The overall scope is acceptable to keep in this PR, but could you clean up the commit history before merging? Please consolidate the review-round commits into a few meaningful commits, ordered as follows:
- Chat E2E API-key gating and test cleanup.
- Python OpenAI embedding refactoring and behavioral hardening.
- Java OpenAI embedding implementation, registration, documentation, and tests.
This keeps the preparatory fixes and refactoring before the main Java feature, making the change history easier to review and trace.
5ad6530 to
1b87141
Compare
|
Commit history consolidated as requested, in this order: (1) |
wenjin272
left a comment
There was a problem hiding this comment.
Thanks for cleaning up the commit history and updating the documentation—the provider reference now focuses on the current contract.
There is one remaining commit-ordering issue: the first [e2e] commit adds the OpenAI embedding module dependency and references ResourceName.EmbeddingModel.OPENAI_*, but those are introduced only by the third Java commit. As a result, the first commit cannot build independently. Could you keep only the Chat E2E API-key gating/helper change in the first commit and move the embedding E2E files and POM dependency into the Java feature commit (or a later commit)?
I also left one inline comment about keeping malformed partial Base64 padding rejected consistently in Java and Python.
ChatModelIntegrationTest gates its API-key providers on a non-blank <PROVIDER>_API_KEY through a new shared OllamaPreparationUtils.hasApiKey helper. This re-arms the OPENAI_RESPONSES case, which was gated on a variable nothing sets (the agent reads OPENAI_API_KEY), and clears MODEL_PROVIDER after each run. Generated-by: Claude Code 2.1.273 (Claude Fable 5.1)
… connection additional_kwargs are sent as extra request body properties and may not repeat a typed field; request_timeout 0 disables the timeout as in the OpenAI chat models; request_timeout, max_retries, dimensions, encoding_format and model are validated at construction and per call; blank or null strings mean absent or default; base64 responses are decoded (previously the characters of the base64 string were returned) with padding restored only for completely unpadded values, so partially padded input is rejected as it is by Java's decoder; responses are checked for size, index consistency and malformed vectors; token usage tolerates partial or string counts. The mocked tests run in the unit suite; only the live-API test keeps the integration marker. Compatibility: additional_kwargs entries that are not typed fields were dropped and are now sent; entries that repeat a typed field previously overrode it and now fail validation; model_kwargs nests additional_kwargs; dimensions must be an integer. Documented in embedding_models.md. Generated-by: Claude Code 2.1.273 (Claude Fable 5.1)
Adds integrations/embedding-models/openai with OpenAIEmbeddingModelConnection and OpenAIEmbeddingModelSetup on com.openai:openai-java, mirroring the Python connection and setup arguments (api_key, base_url, request_timeout, max_retries, organization, project; connection, model, encoding_format, dimensions, user, additional_kwargs). Batches are one request placed by response index with size, range and consistency checks; base64 responses are decoded; token usage is reported through embedWithUsage. Arguments are validated once at setup construction and per call by the same package-private parsers; additional_kwargs may not repeat a typed request field. Registers ResourceName.EmbeddingModel.OPENAI_*, the Python mirror and the `openai` YAML alias in both languages, dist and ide-support dependencies. EmbeddingIntegrationTest gains an OPENAI provider (text-embedding-3-small, dimensions 256) gated on a non-blank OPENAI_API_KEY through OllamaPreparationUtils.hasApiKey, with the Ollama case gated on the local daemon only, and the e2e module depends on the new integration. Docs: Java tabs in the OpenAI section of embedding_models.md, FAQ support matrix, YAML alias table. Generated-by: Claude Code 2.1.273 (Claude Fable 5.1)
1b87141 to
8212eb6
Compare
Closes #1103.
Adds a Java OpenAI embedding model integration, closing the Java/Python parity gap for embeddings: Python already had
OpenAIEmbeddingModelConnection/OpenAIEmbeddingModelSetup; Java had only Ollama and Bedrock, so a Java agent needed the Python wrapper (and a Python runtime on the TaskManagers) for a plain HTTPS call.What's in the change
integrations/embedding-models/openaiwithOpenAIEmbeddingModelConnectionandOpenAIEmbeddingModelSetup, built oncom.openai:openai-java(already a dependency of the OpenAI chat module; no new third-party dependency).api_key(required),base_url,request_timeout(seconds,0disables),max_retries,organization,project. Setup:connection,model,encoding_format(floatorbase64),dimensions,user,additional_kwargs(forwarded as extra body properties; keys that repeat a typed request field are rejected, as in the Watsonx chat connection).indexso input order is preserved even if the API reorders.embedWithUsagereportsprompt_tokens/total_tokens, so the embedding token metrics from [api][python][java] Track embedding token usage metrics #870 cover this provider.ResourceName.EmbeddingModel.OPENAI_CONNECTION/OPENAI_SETUP, the PythonResourceName.EmbeddingModel.Javamirror, YAML aliases (openai) in both the JavaAliasestable andaliases.py,dist,ide-support, and the e2e integration module.embedding_models.mdOpenAI section gains Java tabs (usage, connection and setup parameters); the Python-only hint, the FAQ support matrix, the cross-language example, and the YAML alias table are updated.OpenAIEmbeddingModelTest(29 tests, SDK client mocked, followingBedrockEmbeddingModelTest): client wiring for defaults and explicit options (base URL, timeout, retries, organization, project, blank strings), missingapi_key, timeout/retry parsing bounds, setup parameters and argument-type validation (including a missing or non-stringconnection/modeland anadditional_kwargsvalue the SDK cannot serialize),encoding_formatvalidation, reservedadditional_kwargskeys, single and batch embeddings with token usage, a response withoutusage, with partial usage, or with decimal-string counts, out-of-order, index-less, consistent and contradictory partially indexed, non-integer-index, and out-of-range-index response handling, null input rejection with its position, dimensions range validation per call, parameter forwarding (encoding format, dimensions, user,additional_kwargsas body properties, other per-call keys not forwarded), a base64 response decoded to floats (with a truncated payload rejected rather than silently shortened), a malformed vector reported with its position, an empty batch validated without a request, a response withoutdata, with a malformeddatafield, with a null item, or with a string vector when float was requested, mismatched response size, missing model.AliasesTestand the Pythontest_aliases.pycover the newopenaiembedding alias in both languages. The Pythontest_openai_embedding_model.pygains mocked tests forrequest_timeout: 0, invalid timeout/retry bounds,additional_kwargssent asextra_body, reserved-key rejection,encoding_format/dimensionsvalidation, base64 decoding with index ordering and the prompt-token fallback, short/contradictory/duplicate-index, malformed or corrupted-base64 vector, unusable-usage and missing-dataresponses, empty batches, and blank/null-string default handling.EmbeddingIntegrationTestgains anOPENAIprovider (gated onOPENAI_API_KEY, like the chat model integration test).Review follow-ups deliberately left out of this PR: chunking batches at
batch_size(the OpenAI per-request input cap applies equally to the Python connection and to how the vector stores callembed), unwrappingInvocationTargetExceptioninJavaResourceProviderso constructor errors surface their message, validatingconnection/modelinBaseEmbeddingModelSetupfor every setup, shared numeric-argument, reserved-key and token-usage helpers for the OpenAI family, a provider-to-required-env-vars table for the integration test gates, coercing quoted numeric YAML scalars in Java (or rejecting them in the Python connection'srequest_timeout/max_retries, which still use pydantic's lax coercion), probingadditional_kwargsvalues for JSON-serializability in the Python setup as the Java setup does, and rejecting fractional response indices that the Java SDK's Jackson mapper truncates to integers. Each is a cross-cutting change beyond #1103. One review suggestion was declined on purpose: requestingbase64on the wire by default would cut response size, but both connections send the configuredencoding_formatverbatim (defaultfloat) so the wire format is the same in both languages and for OpenAI-compatible servers. Another: falling back to response order when a server returns duplicate or shiftedindexvalues (some proxies do) would silently risk attributing vectors to the wrong texts, so both connections fail the call instead.Naming follows the Python embedding connection (
base_url,request_timeout) rather than the Java chat connection (api_base_url,timeout), as proposed in the issue, so the two embedding implementations align across languages.Design notes from self-review: only the setup's
additional_kwargsreach the request body, and other per-call parameters such as a caller'stimeoutare ignored. Per-call parameters override setup parameters entry by entry (the base class contract), so a per-calladditional_kwargsmap replaces the setup's map; the javadoc says so. The PythonOpenAIEmbeddingModelConnectionis aligned in this PR: it now sendsadditional_kwargsasextra_body, rejects keys that repeat the typed fields, boundsrequest_timeout/max_retrieslike the Python OpenAI chat models, validatesencoding_formatanddimensions, decodesbase64responses (previously the characters of the base64 string were returned; both languages now reject a payload whose length is not a multiple of four instead of returning a short vector), and applies the same response size and index checks as Java. A usage block missingprompt_tokensreports prompt = total in both languages, as the Tongyi connection does.Compatibility impact (Python
OpenAIEmbeddingModelSetup):additional_kwargsentries that are not typed fields were previously dropped and are now sent in the request body; entries that repeat a typed field (for example{"dimensions": 8}) previously overrode it silently and now fail validation;encoding_formatoutsidefloat/base64, non-integer or non-positivedimensions, a blankconnection, non-string typed arguments, malformedadditional_kwargs, and non-string batch elements now fail validation instead of reaching the API. The docs state the current contract only; this description carries the compatibility notes for the release notes. Blankuser/encoding_format/base_urlare treated as absent/default as in Java (a blank Pythonapi_keystays accepted for unauthenticated OpenAI-compatible servers, as before this PR; Java rejects it like the Java chat connection); the resolvedencoding_formatis always sent on the wire in both languages. No Java compatibility impact (new module). Responses withoutusage, with partial usage, or without per-itemindex(OpenAI-compatible servers) are tolerated; an item without an index keeps its response position and every slot must be filled exactly once, so out-of-range, duplicated or contradictory indices fail the call rather than being guessed (same rule in Python).request_timeout: 0disables the timeout like the Java and Python OpenAI chat connections; the Python embedding connection gets the same one-line0 -> Nonemapping (with a test) so both languages agree. Numeric parsing follows the Java chat connection's exact-BigDecimal rules (sub-millisecond values round up, integrality viatoBigIntegerExact). Argument validation has one owner, package-private parsers in the connection, used both by the setup at construction and per call, so per-call overrides cannot bypass it. TheOPENAIcase ofEmbeddingIntegrationTestis gated onOPENAI_API_KEYlike the chat provider cases; CI does not supply that secret, so it is manual-only, and the results below come from a local run.Test commands and results
Unit tests for the new module:
Complete Java unit suite (all non-e2e modules, as
tools/ut.shruns it):Complete Python unit suite (e2e tests excluded, as
tools/ut.shdoes) plus the cross-language resource-name consistency check:End-to-end, real OpenAI API, embedded Flink 2.3 cluster (
OPENAI_API_KEYexported from a local file, never printed):End-to-end, real OpenAI API, standalone local Flink 2.3.0 cluster (
start-cluster.sh,flink-agents-dist-flink-2.3inlib/, throwaway job submitted withflink run, results written by aFileSink):The requested
dimensionsare honored, OpenAI's unit-normalized vectors come back with norm 1.000, batch calls return one vector per input, and token usage is reported for single and batch calls.The key-gate fix in
OllamaPreparationUtils.hasApiKeyalso re-arms theOPENAI_RESPONSEScase ofChatModelIntegrationTest(it was gated on a variable nothing sets); that test was run once locally withOPENAI_API_KEYexported:Also run:
ruff checkandruff format --checkon the touched Python files, Hugo build of the docs, spotless on the touched modules.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code 2.1.273 (Claude Fable 5.1)
🤖 Generated with Claude Code