fix(embedding): stop forcing dimensions on OpenAI-compatible embedding calls - #2542
fix(embedding): stop forcing dimensions on OpenAI-compatible embedding calls#2542mesutoezdil wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Removes the hardcoded dimensions=768 parameter from OpenAI-compatible embedding requests so providers that don’t support exactly 768 dimensions can still be used, relying on existing truncate/normalize post-processing.
Changes:
- Drop the
dimensionsparameter from the Python OpenAI embedding call path. - Drop the
Dimensionsfield from the Go OpenAI embedding request payload. - Add/adjust unit tests to assert
dimensionsis not sent.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| python/packages/kagent-adk/tests/unittests/test_embedding.py | Adds a unit test asserting the OpenAI embedding call omits dimensions. |
| python/packages/kagent-adk/src/kagent/adk/models/_embedding.py | Removes dimensions=self.TARGET_DIMENSION from the OpenAI embeddings request. |
| go/adk/pkg/embedding/embedding_test.go | Updates the OpenAI provider test expectation to ensure dimensions is not sent. |
| go/adk/pkg/embedding/embedding.go | Removes Dimensions from the OpenAI embeddings request params. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…g calls The openai, azure_openai, and foundry embedding paths in both the Go and Python runtimes asked for exactly 768 dimensions unconditionally. Providers that only offer other sizes, such as Bedrock Titan behind an OpenAI-compatible gateway, reject the request outright, even though the existing truncate and normalize step already handles anything larger than 768. Drops the dimensions parameter on these paths, since the truncate and normalize step already covers it. Checks the raw JSON body for an absent dimensions field, since decoding into an int cannot tell an absent field from an explicit 0. Signed-off-by: mesutoezdil <mesudozdil@gmail.com>
2230e5e to
aa18bd1
Compare
…ons-openai-compatible Signed-off-by: mesutoezdil <mesudozdil@gmail.com>
supreme-gg-gg
left a comment
supreme-gg-gg
left a comment
There was a problem hiding this comment.
Non-blocking note: relying on truncation should work fine for most models, including OpenAI’s embedding models. However, a custom model behind an OpenAI-compatible gateway may accept the request but lose semantic quality after truncation if it was not trained with Matryoshka Representation Learning, as the existing code comment notes. Since a lot of modern embedding models, including many open-source ones, support MRL, I don’t think this should block the PR, but just placing a note here
The openai, azure_openai, and foundry embedding paths in both the Go and Python runtimes asked for exactly 768 dimensions unconditionally. Providers that only offer other sizes, such as Bedrock Titan behind an OpenAI-compatible gateway, reject the request outright, even though the existing truncate and normalize step already handles anything larger than 768. Drops the dimensions parameter on these paths, since the truncate and normalize step already covers it.