[AI-3] Probe models for embedding support automatically - #24886
[AI-3] Probe models for embedding support automatically#24886tangopium wants to merge 8 commits into
Conversation
|
Caution The provided work package version does not match the core version Details:
Please make sure that:
|
|
Caution The Enterprise plan field is not set on the work package Details:
Please make sure that:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e475d8679
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
6e475d8 to
87cc4ee
Compare
tangopium
changed the title
87cc4ee to
c83347e
Compare
c83347e to
82901e2
Compare
Adds the one behavioural probe worth writing: a single embeddings request per candidate model, whose 200 response also carries the vector dimension count. Probing every listed model would be wrong, since a gateway can list hundreds and some providers bill per request, so the background pass after a credential change is capped at ten models whose names suggest they embed. The name is only a hint for where to spend a probe, never a verdict in itself. A definite refusal records unsupported, a vector records supported, and anything else records nothing at all, because servers silently drop unknown parameters and a 200 alone proves nothing. Administrator assertions are never overwritten. Part 8 of the AI-3 stack. https://community.openproject.org/work_packages/66020
82901e2 to
d6182c5
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
thykel
left a comment
thykel
left a comment
There was a problem hiding this comment.
I know we discussed potentially changing a lot of this stuff -- so for now I'm just leaving you with some Mr. Robot comments that look legit.
|
|
||
| # The server understood the request and refused it for this model. Anything | ||
| # else -- 5xx, throttling -- says something about the server, not the model. | ||
| REFUSED_STATUSES = [400, 404, 405, 501].freeze |
There was a problem hiding this comment.
🤖 #Logic 🚨 Do all four statuses really mean the model refused?
A match here records :unsupported, which is the only state that blocks - and record refuses to soften a persisted definite verdict with a later :unknown. So a wrong reading is permanent until an administrator overrides it by hand.
Two of the four look like statements about the server, not the model:
- 501 - this server implements no embeddings at all.
- 405 - about the route, not the model.
- 404 - ambiguous: no
/embeddingsroute, or an unknown model. - 400 - can be about the request. The probe sends
PROBE_INPUTas a bare string; a server that requiresinputas an array, or requiresencoding_format, answers 400 for every model, including genuine embedding models.
Worth noting that the stack already reads three of these the opposite way: LlmServerValidator::MODELS_ENDPOINT_ABSENT = [404, 405, 501] treats them as "the endpoint is not here, conclude nothing bad".
The failure mode: point the connection at a gateway that routes chat completions and nothing else - the deployment manual model entry exists for - and every candidate is permanently marked incapable, so semantic search can never bind to any of them. In the models table that renders as a confident grey "Chat" label, indistinguishable from a correctly determined one.
Could we drop 404/405/501 to :unknown and keep 400 as the only per-model refusal? And if the first probe of a batch returns one of them, stop - the remaining nine billed requests buy nothing, which also serves the cost rationale behind BACKGROUND_LIMIT.
| require "spec_helper" | ||
|
|
||
| RSpec.describe LlmConnections::DetectCapabilitiesService, :llm_server_helpers, :webmock do | ||
| subject(:service) { described_class.new(connection) } |
There was a problem hiding this comment.
🤖 #Testing
All three examples call service.detect("bge-m3"). detect_likely_embedding_models - the entry point DetectCapabilitiesJob invokes - is never exercised, and it holds both decisions the PR description leads with:
- the
EMBEDDING_NAME_HINTfilter, so non-matching models are skipped BACKGROUND_LIMIT = 10
The cap guards real money, since each probe is a billed request on some providers. Could we add a case for each? Nothing would currently catch the cap being removed.
|
|
||
| def record(model_id, result) | ||
| verdicts.transaction do | ||
| verdict = verdicts.lock.find_or_initialize_by(model_id:, capability: "embeddings") |
There was a problem hiding this comment.
🤖 #Logic
FOR UPDATE locks a row that exists. On a first probe there is no row, so nothing is locked and find_or_initialize_by just builds a new record - the comment below promises more than the code delivers.
Two concurrent probes of the same model then both insert and hit the unique index on (llm_connection_id, model_id, capability), raising RecordNotUnique with nothing to catch it. That becomes reachable once the synchronous detect gets its caller in the feature bindings controller and can run alongside the background job.
Would create_or_find_by, or rescuing RecordNotUnique and re-reading, close it?
| @probe ||= Llm::Probes::EmbeddingsProbe.new(connection) | ||
| end | ||
|
|
||
| def candidates |
There was a problem hiding this comment.
🤖 #Design
available_model_ids is models.active, which includes rows an administrator has deactivated. The previous PR introduced selectable_model_ids for exactly this distinction.
Someone who switched a model off has said they do not want it used, so a speculative probe against it costs a request for no benefit. Is selectable_model_ids the right source here?
| # connection does not wait on one request per candidate model. | ||
| class DetectCapabilitiesJob < ApplicationJob | ||
| def perform | ||
| connection = LlmConnection.first |
There was a problem hiding this comment.
🤖 #Design ℹ️ Third place the single-connection assumption is hard-coded
This is copied from SyncModelsJob, including the guard that cannot fire: configured? is base_url.present?, and base_url is null: false with a presence validation, so it is always true for a persisted row.
With Queries::LlmModels::LlmModelQuery#default_scope, that is now three places holding an assumption LlmConnection's own docstring says is a one-line change to lift. Could this iterate with find_each and leave the cardinality to the model's validation?
| REFUSED_STATUSES = [400, 404, 405, 501].freeze | ||
|
|
||
| Result = Data.define(:state, :detail) do | ||
| def supported? = state == :supported |
There was a problem hiding this comment.
🤖 #Style ℹ️ Result#supported? has no caller
No reference in app/, none in the specs, and none later in the stack. Worth removing until something needs it?
| class DetectCapabilitiesService | ||
| # Naming is a hint for which models are worth spending a probe on, never a | ||
| # verdict in itself. | ||
| EMBEDDING_NAME_HINT = /embed|bge|e5|gte|nomic|minilm/i |
There was a problem hiding this comment.
🤖 #Style ℹ️ e5 and gte match anywhere in an identifier
Unanchored, these two match inside build hashes and version suffixes, so a chat model can be pulled into the candidate set and cost a request. The cost is bounded and the comment is clear that naming is only a hint, so this is minor - but anchoring on a separator, for example (^|[-_./])e5, would tighten it at no cost.
Live discovery hands us the server's own card, which for OpenRouter and the gateways following it carries the model name and a context_length rather than the max_model_len that vLLM reports. Both were dropped on the floor, so an OpenRouter connection listed bare ids and an empty context window even though the server had just told us better. The card now names the model when the adapter cannot, its context_length fills the window unless the operator's own limit is reported, and the registry only fills in what neither the card nor the stored metadata already has, so a refresh no longer overwrites a figure that came from the deployment itself.
The UX review with Tom made the model refresh manual, so hanging capability detection off a settings change no longer matches when the list actually changes. Detection now follows the sync itself, whichever caller asked for it: the initial fill, the Refresh button on the AI models page and the environment seeder's background sync all end in a probe pass. The pass itself spends fewer requests. It reads the models an administrator still offers rather than every active one, so a model switched off is not probed, and the e5 and gte name hints only count on a separator, which keeps build hashes out of the candidate set. The job iterates the stored connections instead of taking the first one behind a guard that cannot fire. Before the first probe of a model there is no row for FOR UPDATE to lock, so two probes running at once both inserted one. The row is now claimed through the unique index first and the verdict written under the lock as before.
Tom pointed out in the review that three of the four statuses the probe read as a refusal are statements about the server. A 404, 405 or 501 says the embeddings route is not there, which a gateway routing chat completions and nothing else answers for every model alike. Since only an unsupported verdict blocks, and a definite verdict is never softened again, that marked every candidate permanently incapable and left semantic search with nothing to bind to. Those three are now inconclusive, in the same reading LlmServerValidator already applies to the model list, and a batch that meets one stops: the remaining probes are billed for the same answer. A 400 stays the one per-model refusal. Result#supported? goes with it; nothing ever called it.
Enrichment skipped the published window whenever the model already had one, and an administrator's override counts as one. A refresh against a server that lists bare model ids drops the registry figure from the metadata, so clearing the override afterwards left the window unknown until the next refresh. The guard now looks at what the server reported, not at the effective window. Reported in the review of the follow-up commits (finding 4).
The batch decided whether to stop from the verdict it had just recorded, but an inconclusive probe never softens a definite verdict, so a model with a cached supported or unsupported answer hid the 404 and the batch kept spending requests up to its limit. The decision now reads the probe result itself. Reported in the review of the follow-up commits (finding 6).
The example was annotated with the claim that only registry-backed adapters report a name. Since this part reads the name out of the live card as well, a server speaking the OpenAI API does supply one whenever it lists more than ids.
The capability detection spec still asked the factory for a connection with LLMs switched on, which was a column on the row before that switch became a setting. The probe does not read it either way.
d6182c5 to
861d35f
Compare
|
The branch was rebuilt, so the commits your comments point at no longer exist and those threads now render as outdated. Nothing was dropped. Here is where each point landed.
CI has not started on the new commits: only the CLA check ran, and the workflow runs seem to need maintainer approval on this repository. The checks are waiting on that. |
2 participants
Ticket
AI-3
What are you trying to accomplish?
PR 8 of 11 in the AI-3 stack. The one behavioural probe worth writing: a single embeddings request per candidate model, whose 200 response also carries the vector dimension count. The background pass after a credential change is capped at ten models whose names suggest they embed, because a gateway can list hundreds and some providers bill per request. A definite refusal records unsupported, a vector records supported, and anything else records nothing, because servers silently drop unknown parameters and a 200 alone proves nothing. Administrator assertions are never overwritten.
Merge checklist
llm_connectionfeature flagStacked on #24885.