inference: add embedding provider foundation#69903
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-tests-checked
release-note-none
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 similar comment
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds shared embedding contracts and utilities, a concurrent batching embedder, deterministic mock and OpenAI providers, extensive tests, Bazel targets, and a bindinfo test-sharding adjustment. ChangesEmbedding subsystem
Bazel test sharding adjustment
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Batch
participant Provider
Caller->>Batch: CreateEmbeddings(provider/model, texts, opts)
Batch->>Batch: Group compatible requests
Batch->>Provider: CreateEmbeddings(merged texts, opts)
Provider-->>Batch: embeddings or error
Batch-->>Caller: request-specific embeddings or error
sequenceDiagram
participant Caller
participant OpenAIEmbedder
participant OpenAIAPI
Caller->>OpenAIEmbedder: CreateEmbeddings(model, texts, opts)
OpenAIEmbedder->>OpenAIAPI: POST /embeddings
OpenAIAPI-->>OpenAIEmbedder: Response or ErrorResponse
OpenAIEmbedder-->>Caller: decoded embeddings or error
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
pkg/inference/embedding/mock/mock.go (1)
74-78: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePrevent timer leak by using
time.NewTimer.When
ctx.Done()is selected before the duration elapses,time.Afterleaks the underlying timer until the duration fires (which is 1 hour in one of your tests). It is a good practice to usetime.NewTimerand explicitly stop it to prevent accumulating lingering timers in the test environment.♻️ Proposed refactor
- select { - case <-time.After(dur): - case <-ctx.Done(): - return nil, ctx.Err() - } + timer := time.NewTimer(dur) + select { + case <-timer.C: + case <-ctx.Done(): + timer.Stop() + return nil, ctx.Err() + } + timer.Stop()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/inference/embedding/mock/mock.go` around lines 74 - 78, Update the delay select in the mock embedding flow to use a timer created with time.NewTimer instead of time.After, and stop the timer when the context cancellation branch wins. Preserve the existing duration completion and ctx.Err() return behavior.pkg/inference/embedding/batcher/batcher_test.go (1)
203-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the new test targets deterministic instead of relying on flaky retries.
pkg/inference/embedding/batcher/batcher_test.go#L203-L247: synchronize all calls entering the batch before triggering processing.pkg/inference/embedding/batcher/batcher_test.go#L578-L630: replace elapsed-time upper bounds with an explicit provider-start signal.pkg/inference/embedding/batcher/BUILD.bazel#L16-L16: removeflaky = Trueafter fixing the timing assumptions.pkg/inference/embedding/base/BUILD.bazel#L15-L15: removeflaky = True; these utility tests are already deterministic.As per coding guidelines, test changes should remain deterministic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/inference/embedding/batcher/batcher_test.go` around lines 203 - 247, Make the batching tests deterministic by synchronizing all CreateEmbeddings calls in TestBatchEmbedder_BatchingSameModel before allowing batch processing; at pkg/inference/embedding/batcher/batcher_test.go:203-247, use an explicit barrier or equivalent coordination. At pkg/inference/embedding/batcher/batcher_test.go:578-630, replace elapsed-time upper-bound assertions with an explicit signal that the provider has started. Remove flaky = True from pkg/inference/embedding/batcher/BUILD.bazel:16 and pkg/inference/embedding/base/BUILD.bazel:15.Source: Coding guidelines
pkg/inference/embedding/openai/openai_test.go (1)
69-88: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAvoid
requireinside thehttptest.Serverhandlers
requirecallsFailNow, which must run on the test goroutine. Capture request data in the handler and assert in the test body, or useassertfor in-handler checks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/inference/embedding/openai/openai_test.go` around lines 69 - 88, Update the httptest.Server handler in the OpenAI embedding test to avoid all require calls, since FailNow must run on the test goroutine. Capture the request path, method, headers, body, and any read error in the handler, then perform the corresponding require assertions after the request completes in the test body.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/inference/embedding/base/base.go`:
- Around line 62-77: Update JSONFieldsWithOptions so fixed fields always take
precedence over provider options when keys collide; preserve all non-conflicting
options. Ensure MarshalJSONWithOptions uses this corrected merge behavior,
preventing options such as input, model, or encoding_format from changing the
actual request payload.
In `@pkg/inference/embedding/batcher/batcher.go`:
- Around line 101-118: Update RegisterEmbedder and MustRegisterEmbedder to
reject nil base.Embedder values before storing them, using a shared registration
path where practical. Preserve the existing provider normalization and
duplicate/format validation, and ensure neither method inserts a nil embedder
into b.embedders.
- Around line 254-281: Update the batching flow around thisBatch.calls and
startIndices to exclude calls whose contexts are already canceled before
building allTexts, while preserving offsets for the remaining active callers. If
every call is canceled, skip embedder.CreateEmbeddings entirely and return the
corresponding canceled results; otherwise dispatch only the active texts and
retain the existing cancellation watcher behavior for them.
In `@pkg/inference/embedding/openai/openai.go`:
- Around line 115-118: Update the response-reading logic in the OpenAI embedding
request flow around io.ReadAll to enforce a configurable maximum body size for
both successful and error responses. Use a bounded reader, reject responses
exceeding the limit, and preserve the existing error propagation behavior.
- Around line 156-159: Update the error handling in the embedding decode flow
around base.DecodeFloat32ArrayBytes to wrap and preserve the returned err while
retaining the item index context. Ensure callers can inspect the underlying
decoding failure, including malformed base64 errors.
---
Nitpick comments:
In `@pkg/inference/embedding/batcher/batcher_test.go`:
- Around line 203-247: Make the batching tests deterministic by synchronizing
all CreateEmbeddings calls in TestBatchEmbedder_BatchingSameModel before
allowing batch processing; at
pkg/inference/embedding/batcher/batcher_test.go:203-247, use an explicit barrier
or equivalent coordination. At
pkg/inference/embedding/batcher/batcher_test.go:578-630, replace elapsed-time
upper-bound assertions with an explicit signal that the provider has started.
Remove flaky = True from pkg/inference/embedding/batcher/BUILD.bazel:16 and
pkg/inference/embedding/base/BUILD.bazel:15.
In `@pkg/inference/embedding/mock/mock.go`:
- Around line 74-78: Update the delay select in the mock embedding flow to use a
timer created with time.NewTimer instead of time.After, and stop the timer when
the context cancellation branch wins. Preserve the existing duration completion
and ctx.Err() return behavior.
In `@pkg/inference/embedding/openai/openai_test.go`:
- Around line 69-88: Update the httptest.Server handler in the OpenAI embedding
test to avoid all require calls, since FailNow must run on the test goroutine.
Capture the request path, method, headers, body, and any read error in the
handler, then perform the corresponding require assertions after the request
completes in the test body.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e606833c-58e5-4420-a40c-559951d09559
📒 Files selected for processing (13)
pkg/inference/embedding/base/BUILD.bazelpkg/inference/embedding/base/base.gopkg/inference/embedding/base/base_test.gopkg/inference/embedding/batcher/BUILD.bazelpkg/inference/embedding/batcher/batcher.gopkg/inference/embedding/batcher/batcher_test.gopkg/inference/embedding/mock/BUILD.bazelpkg/inference/embedding/mock/mock.gopkg/inference/embedding/mock/mock_test.gopkg/inference/embedding/openai/BUILD.bazelpkg/inference/embedding/openai/openai.gopkg/inference/embedding/openai/openai_test.gopkg/inference/embedding/openai/protocol.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #69903 +/- ##
================================================
- Coverage 76.3198% 73.9091% -2.4108%
================================================
Files 2041 2058 +17
Lines 559955 579713 +19758
================================================
+ Hits 427357 428461 +1104
- Misses 131697 150767 +19070
+ Partials 901 485 -416
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest |
|
/retest |
|
/retest |
| if message != "" { | ||
| return nil, fmt.Errorf("OpenAI: %s", message) | ||
| } | ||
| return nil, fmt.Errorf("OpenAI: status code %d", resp.StatusCode) |
There was a problem hiding this comment.
maybe merge into one OpenAI: status code %d, message: %s
There was a problem hiding this comment.
This would be reasonable for a new error contract, but it changes the externally visible error text that existing downstream code already receives.
The current implementation always records the HTTP status in the structured internal log, returns the sanitized provider message when one is available, and falls back to a status-code error otherwise. We prefer to preserve that behavior in this compatibility-focused PR. A structured provider error can be designed together with retry semantics in a separate change.
There was a problem hiding this comment.
who is "existing downstream code"?
There was a problem hiding this comment.
By "existing downstream code" I mean the deployed EMBED_TEXT() integration from tidbcloud/tidb-cse#1686 and its follow-up tidbcloud/tidb-cse#2015.
The concrete call path is pkg/expression/builtin_inference.go -> domainadaptor.GetEmbedFn(sctx).EmbedWithContext(...) -> the batcher -> the OpenAI embedder. EvalEmbedTextArgsToDatum returns the provider error directly, so this text is surfaced to the TiDB user as the SQL error from EMBED_TEXT(). The provider initialization in pkg/inference/sqlembed.go also supplies the user-facing 401 error.
Therefore changing all failures to OpenAI: status code %d, message: %s would change SQL-visible error text already used by that deployment. For this compatibility-focused split PR, I prefer preserving the current text. A later typed provider error can carry status/retry metadata without requiring a user-visible text change.
I should have identified this downstream explicitly in the previous reply.
There was a problem hiding this comment.
there is no need to compatible with tidb-cse, I think we can simplify it directly
| return nil, err | ||
| } | ||
|
|
||
| jsonData, err := json.Marshal(base.JSONFieldsWithOptions(map[string]any{ |
There was a problem hiding this comment.
does open AI have SDK for accessing its resource? so we can avoid write raw http req/resp
There was a problem hiding this comment.
OpenAI Go SDKs are available, but this adapter deliberately targets OpenAI-compatible endpoints rather than only the hosted OpenAI service. It needs configurable base URLs and transparent pass-through of provider-specific options.
The raw HTTP surface here is small, has explicit timeout/body-size/error handling, and avoids introducing SDK-specific request types, retry behavior, or version coupling. For this foundation layer, we prefer to keep the minimal HTTP adapter.
| } | ||
| } | ||
|
|
||
| func readResponseBody(reader io.Reader, maxBytes int64) ([]byte, error) { |
There was a problem hiding this comment.
Could simplify this by letting a single bounded io.ReadAll include the overflow probe:
func readResponseBody(reader io.Reader, maxBytes int64) ([]byte, error) {
body, err := io.ReadAll(io.LimitReader(reader, maxBytes+1))
if err != nil {
return nil, err
}
if int64(len(body)) > maxBytes {
return nil, fmt.Errorf("response body exceeds maximum size of %d bytes", maxBytes)
}
return body, nil
}For an arbitrary io.Reader, consuming one extra byte is still necessary to distinguish an exactly-at-limit body from an oversized one, but this avoids the separate io.ReadFull step and states the intent more directly. Content-Length could provide an early rejection, but cannot replace the bounded read because it may be unknown or inaccurate.
There was a problem hiding this comment.
Thanks, the proposed implementation is valid and is slightly more compact. The current implementation has the same bound and semantics: it reads at most maxBytes, then probes exactly one additional byte to distinguish an exactly-at-limit response from an oversized response.
Since the existing implementation is covered by exact-limit and overflow tests and there is no correctness difference, we prefer to avoid a non-functional rewrite in this compatibility-focused PR.
D3Hunter
left a comment
D3Hunter
left a comment
There was a problem hiding this comment.
Summary
- Total findings: 21
- Inline comments: 21
- Summary-only findings (no inline anchor): 0
Findings (highest risk first)
⚠️ [Major] (11)
- Embedder contract omits the output-to-input ordering invariant (
pkg/inference/embedding/base/base.go:42; pkg/inference/embedding/batcher/batcher.go:368; pkg/inference/embedding/openai/openai.go:196) - Register and MustRegister do not expose distinct registration semantics (
pkg/inference/embedding/batcher/batcher.go:105) - SanitizeErrorText overstates a best-effort credential redactor (
pkg/inference/embedding/base/base.go:34; pkg/inference/embedding/base/base.go:72; pkg/inference/embedding/openai/openai.go:170) - Batch CreateEmbeddings hides its required provider-qualified model syntax (
pkg/inference/embedding/batcher/batcher.go:224; pkg/inference/embedding/base/base.go:44; pkg/inference/embedding/openai/openai.go:110) - Canceled batch owner can mutate options used by another caller (
pkg/inference/embedding/batcher/batcher.go:257 (consumed at line 364)) - Batched callers receive aliased result slices (
pkg/inference/embedding/batcher/batcher.go:392) - Full batches can fan out an unbounded number of provider requests (
pkg/inference/embedding/batcher/batcher.go:287) - A panic in asynchronous batch processing can terminate the TiDB process (
pkg/inference/embedding/batcher/batcher.go:259) - OpenAI failures do not preserve a retryable error classification (
pkg/inference/embedding/openai/openai.go:166) - Caller ownership is encoded in parallel slices during batch fan-out (
pkg/inference/embedding/batcher/batcher.go:317 and pkg/inference/embedding/batcher/batcher.go:389) - Cancellation isolation test can pass before the watcher observes cancellation (
pkg/inference/embedding/batcher/batcher_test.go:506)
🟡 [Minor] (8)
- OpenAI protocol types are exported without consumers or canonical use (
pkg/inference/embedding/openai/protocol.go:17; pkg/inference/embedding/openai/openai.go:139) - Constructor names and comments refer to nonexistent type names (
pkg/inference/embedding/openai/openai.go:47; pkg/inference/embedding/openai/openai.go:60; pkg/inference/embedding/mock/mock.go:33) - DecodeFloat32ArrayBytes does not signal byte order in its name (
pkg/inference/embedding/base/base.go:48; pkg/inference/embedding/base/base_test.go:25) - Unknown-provider errors enumerate providers nondeterministically (
pkg/inference/embedding/batcher/batcher.go:141) - A later chunk failure discards successful results for earlier callers (
pkg/inference/embedding/batcher/batcher.go:361) - Provider-specific implementation helpers are exposed from the base contract package (
pkg/inference/embedding/base/base.go:28 through pkg/inference/embedding/base/base.go:88) - The public mock provider has no consumer and duplicates local test scaffolding (
pkg/inference/embedding/mock/BUILD.bazel:3, pkg/inference/embedding/mock/mock.go:26, and pkg/inference/embedding/batcher/batcher_test.go:31) - Bindinfo test sharding is outside the embedding foundation scope (
pkg/bindinfo/tests/BUILD.bazel:14)
🧹 [Nit] (2)
- Numeric cancellation test names hide the scenarios under test (
pkg/inference/embedding/batcher/batcher_test.go:749; pkg/inference/embedding/batcher/batcher_test.go:773) - Narration-only comments repeat the surrounding function names (
pkg/inference/embedding/batcher/batcher.go:265; pkg/inference/embedding/batcher/batcher.go:301; pkg/inference/embedding/batcher/batcher.go:332; pkg/inference/embedding/batcher/batcher.go:379)
| ) | ||
|
|
||
| // Embedder is an interface for embedding providers. | ||
| type Embedder interface { |
There was a problem hiding this comment.
⚠️ [Major] Embedder contract omits the output-to-input ordering invariant
Why
Embedder is the shared provider contract, but its only method comment does not say that it must return exactly one embedding per input in the same logical order. The batcher and the OpenAI adapter both rely on that invariant to associate vectors with the original texts.
Scope
pkg/inference/embedding/base/base.go:42; pkg/inference/embedding/batcher/batcher.go:368; pkg/inference/embedding/openai/openai.go:196
Risk if unchanged
A new provider can satisfy the interface while returning missing, extra, or provider-ordered vectors; callers may then receive an error late in batching or, worse, associate a valid vector with the wrong text.
Evidence
The interface comment only says CreateEmbeddings generates embeddings. processBatch requires len(chunkEmbeddings) == len(texts) and slices the aggregate by caller boundaries, while the OpenAI implementation explicitly restores request order from response indices.
Change request
Please add a precise contract stating that result length must equal len(texts) and result element i must correspond to texts[i], including the empty-input behavior expected from implementations.
There was a problem hiding this comment.
Thanks, this is the semantic invariant used by the current implementations. The OpenAI adapter validates cardinality, rejects duplicate/out-of-range indices, and restores request order; the batcher also rejects a provider result whose length does not match the submitted chunk before fan-out.
We agree that a stronger interface-level description could improve discoverability. For this PR, however, we are preserving the already-consumed base interface and relying on the existing runtime validation rather than broadening the public contract. No behavior change is needed for the current providers or batcher.
| } | ||
| } | ||
|
|
||
| // Register registers an embedder with the given provider name as the prefix. |
There was a problem hiding this comment.
⚠️ [Major] Register and MustRegister do not expose distinct registration semantics
Why
The non-Must method still panics for nil embedders, accepts invalid provider names that MustRegister rejects, and silently replaces an existing provider. The method names and comments therefore do not tell callers whether registration validates, overwrites, returns an error, or panics.
Scope
pkg/inference/embedding/batcher/batcher.go:105
Risk if unchanged
Configuration mistakes can either crash the process or silently reroute embedding requests to a replacement provider; a provider containing / can also be accepted even though the provider/model parser can never select it.
Evidence
Register calls panicIfNilEmbedder and assigns directly to b.embedders[provider]. Only MustRegister checks empty names, /, and duplicates before delegating back to Register, and neither comment documents Register's panic or replacement behavior.
Change request
Can we make Register the checked, non-panicking operation that returns an error for invalid, nil, and duplicate registrations, and make MustRegister only the panic-on-error wrapper? If replacement is intentional, expose it as a separately named operation such as Replace and document the side effect.
There was a problem hiding this comment.
We agree that a clean-slate API could make Register return an error and implement MustRegister as a panic-on-error wrapper.
In this case, however, changing Register to return an error and changing its replacement behavior would be a source- and behavior-breaking change for the established downstream implementation. Registration is performed during initialization, before concurrent requests, and production initialization uses the stricter MustRegister path. We therefore prefer to preserve the existing registration API in this compatibility-focused PR rather than introduce a second registration contract.
There was a problem hiding this comment.
please comment why not changed
and who is downstream?
There was a problem hiding this comment.
Thanks for pushing on this. I rechecked the actual downstream and my previous reply was inaccurate: production initialization uses the corresponding RegisterEmbedder path, not MustRegisterEmbedder.
The downstream is the deployed EMBED_TEXT() implementation from tidbcloud/tidb-cse#1686 / tidbcloud/tidb-cse#2015, specifically pkg/inference/sqlembed.go. Its NewEmbedFn initialization registers a fixed set of valid, unique providers such as openai, jina_ai, and cohere; it does not rely on invalid provider names or silent replacement.
Based on that check, I changed this in commit 6372fc9592: Register now returns an error for invalid names, nil/typed-nil embedders, and duplicate normalized providers, and it never silently replaces an existing provider. MustRegister is now only the panic-on-error wrapper. I did not add Replace because there is no current replacement use case.
This changes only the Go registration helper; the TiDB SQL/config behavior and provider wire protocols remain unchanged.
|
|
||
| maxSanitizedErrorTextBytes = 4096 | ||
| ) | ||
|
|
There was a problem hiding this comment.
⚠️ [Major] SanitizeErrorText overstates a best-effort credential redactor
Why
The exported name reads as a general guarantee that arbitrary provider error text is safe to log or return, while the implementation only recognizes a small set of field names and token shapes plus exact secrets supplied by the caller.
Scope
pkg/inference/embedding/base/base.go:34; pkg/inference/embedding/base/base.go:72; pkg/inference/embedding/openai/openai.go:170
Risk if unchanged
Future providers can call this broad helper and log text containing credentials or private input under the mistaken assumption that it has been fully sanitized.
Evidence
The patterns cover authorization, API-key/token variants, credentials, bearer tokens, and sk- keys, but not other common sensitive fields such as passwords, cookies, client secrets, or arbitrary user content. The OpenAI path treats the returned value as loggable provider text.
Change request
Prefer a name that states the bounded behavior, such as RedactKnownCredentialPatternsAndTruncate, and document that it is best effort and that callers must pass every exact secret. Better yet, keep provider-specific redaction internal and avoid logging untrusted provider messages when that guarantee cannot be made.
There was a problem hiding this comment.
The concern is valid, but the current comment intentionally says that the helper redacts “common credential patterns” and exact secrets supplied by the caller; it is not intended to claim arbitrary untrusted text is universally safe.
The OpenAI path also does not log the raw response body. It parses only the provider's error message, passes the exact API key for redaction, truncates the result, and records the HTTP status separately. Renaming or moving this exported helper would change the established API, so we prefer to keep it here and retain the bounded, best-effort semantics.
| } | ||
| } | ||
|
|
||
| // CreateEmbeddings batches requests with the same model/opts combination within the batch window. |
There was a problem hiding this comment.
⚠️ [Major] Batch CreateEmbeddings hides its required provider-qualified model syntax
Why
Batch advertises the same CreateEmbeddings interface as a provider embedder, but its model argument has a different routing contract: it must be provider/model. The public method comment mentions batching only and does not state that grammar or the provider-name normalization rules.
Scope
pkg/inference/embedding/batcher/batcher.go:224; pkg/inference/embedding/base/base.go:44; pkg/inference/embedding/openai/openai.go:110
Risk if unchanged
Code written against base.Embedder can substitute the batcher and pass an ordinary model name, then fail at runtime with a format error even though the method name and interface contract appear identical.
Evidence
The OpenAI implementation accepts a bare model such as text-embedding-3-small, whereas Batch.parseModelWithProvider rejects any value without / and interprets the first segment as a registered provider.
Change request
Please make the routing contract explicit at the public boundary. Prefer a named provider-qualified model concept or a router-specific method; at minimum, document the exact provider/model grammar, normalization, and examples on Batch.CreateEmbeddings so the same parameter does not carry two undisclosed meanings.
There was a problem hiding this comment.
Batch is intentionally a routing implementation of base.Embedder, so it must use the same method signature with one model string. For this implementation, the first path segment is the registered provider selector and the remaining value is passed as the provider-local model.
The downstream SQL integration already uses the established provider/model representation, and invalid bare model names fail with an explicit format error. Introducing separate parameters or a router-specific public method would change the shared interface and existing call sites, so we are keeping the current routing contract in this PR.
|
Thanks for the thorough review. We addressed the concrete isolation, panic-safety, determinism, and test-synchronization issues in
The earlier provider terminology, protocol-source, configurable-error documentation, and malformed-error-response logging comments were addressed in For the remaining API and policy suggestions, we have replied in the individual threads. The implementation is already consumed downstream, so this split keeps the established public interfaces and call semantics stable. Broader topics such as provider concurrency limits, typed retryable errors, and partial-success semantics require explicit owner-level policies and are better handled separately rather than introduced implicitly in this foundation PR. |
|
/retest |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| value := reflect.ValueOf(embedder) | ||
| switch value.Kind() { | ||
| case reflect.Chan, reflect.Func, reflect.Interface, reflect.Map, reflect.Ptr, reflect.Slice, reflect.UnsafePointer: | ||
| return value.IsNil() | ||
| } |
There was a problem hiding this comment.
when embedder == nil is not enough? can you give some example?
2 participants
What problem does this PR solve?
Issue Number: ref #67765
Problem Summary:
The auto-embedding feature needs a reusable, provider-independent foundation for calling external embedding services. Without a common abstraction, each provider would need to duplicate request batching, cancellation, timeout handling, response validation, and credential-safe error logging.
What changed and how does it work?
This PR introduces the embedding provider foundation used by follow-up auto-embedding changes:
Embedderinterface and helpers for JSON options, float32 vector decoding, HTTP timeouts, and credential-safe error logging.The new packages are not registered with the TiDB runtime and do not introduce any SQL, system variable, configuration, or Domain behavior changes in this PR.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit