Resolve LLM roles through an SPI so a role is not tied to one provider - #1894
Resolve LLM roles through an SPI so a role is not tied to one provider#1894jasperblues wants to merge 10 commits into
Conversation
`embabel.models.llms` maps a role to a single model name, and a model belongs to one provider. A role therefore pins the provider it was configured against: give a deployment only an Anthropic key and every OpenAI-named role becomes unsatisfiable, which used to fail the Spring context at startup. There was also nowhere to fix it from outside. `ByRoleModelSelectionCriteria` was a static map lookup consulting no strategy, `ModelSelectionCriteria` is sealed, and `AutoLlmSelectionCriteriaResolver` takes no arguments so it cannot know the caller or the active key. The only escape was `withLlmService(...)`, which skips `ModelProvider` entirely - which is what BYOK applications reach for today, reimplementing role resolution in application code. Route `ByRole` through a `RoleResolver` chain instead. Resolvers are consulted in Ordered order and return null to delegate; the platform's own `ConfigurableRoleResolver` runs last, so existing configuration is unchanged. A resolver answers with `RoleResolution.Options` (a model plus tuning), `Credential` (a user's key - the platform looks the role up under that provider, builds the service and caches it), or `Service` (one it built itself). `ModelSelectionContext` carries who the call is for and which key is active; this is the argument `AutoLlmSelectionCriteriaResolver` lacks. Add `embabel.models.roles`, a role -> provider -> options map, for deployments whose provider is not fixed. The flat `llms` map still works and remains the right shape for a single-provider deployment. With no user key active, the provider is whichever one supplies the default LLM, so those deployments need no context at all. A role can now carry hyperparameters, not just a model name: `ModelProvider.resolveLlmOptions` merges them under whatever the caller set explicitly, and `AbstractLlmOperations` applies it once per operation, only when a role is actually named. An unsatisfiable role throws at the point of use rather than falling back to the default LLM - otherwise a role like "cheapest" silently becomes the most capable and most expensive model in the deployment. Startup is separate: it now warns instead of failing the context, so a partially keyed deployment boots and serves every role that does work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…he Java constructor Three problems with role resolution as it stood. ModelSelectionContext is a ThreadLocal, and the platform moves work off the calling thread itself — AgentPlatform.start, parallel actions, OperationContext.parallelMap. Losing the context there does not fail: ConfigurableRoleResolver falls back to the deployment's own provider and serves a model the deployment is billed for, on a call the user brought their own key for. That is exactly the invariant this feature claims. ExecutorAsyncer now propagates it alongside AgentProcess, restoring the previous value so nothing leaks between tasks on a pooled thread. ConfigurableModelProvider gained two Kotlin default parameters, which Java callers cannot skip. @jvmoverloads keeps the three-argument constructor — without it the BYOK starter's boot test (#1890) fails to compile the moment these two branches meet. The startup warning promised a fallback to the default LLM that no longer happens: an unsatisfiable role throws at the point of use, deliberately. Say that instead. Tests: context propagation through async and parallelMap including pooled-thread isolation; role resolution end to end through ChatClientLlmOperations, so the wiring that makes a role carry hyperparameters is pinned, not just the provider that computes them; LlmOptions.withDefaultsFrom and modelName in their own module; the credential paths that fail rather than fall back. Signed-off-by: jasper blues <jasper@liberation-data.com>
… keys in the clear Streaming went straight to chooseLlm, so a role picked the right model and then streamed with none of the tuning configured against it. Streaming is the chat path, which is where that tuning matters most. Resolve once and stream with the same options; the capability queries resolve too, so they answer about the model the role actually means. Resolution dropped the role name, because withDefaultsFrom takes model selection from the defaults wholesale and the configured options name no role. Selection no longer needs it, but events, logs and cost attribution all want to know a call was made "as cheapest" rather than as a bare model name. Put it back explicitly. The credential cache keyed on ProviderCredential, so every cached service also parked a plaintext key in a map living as long as the platform. Key on a SHA-256 digest instead. The service itself still holds the key it was built from, so this narrows the exposure rather than removing it - but it removes the copy that existed only for lookup. Tests cover each: streaming carries the role's temperature and timeout and never touches the default model; resolved options report the role; and the digest change does not alter what the cache means - same key reuses, different key rebuilds. Signed-off-by: jasper blues <jasper@liberation-data.com>
The startup check only walked embabel.models.llms, so the shape this feature adds got no validation at all — while the docs it ships say an unsatisfiable role warns at startup. That gap matters most precisely here. The nested shape is the one place configuration can take a role AWAY: with an entry present for the active provider, resolution uses it and throws when its model is not registered, instead of falling back to the flat map. So a typo under roles was silent until something asked for the role, and then failed a request. Only entries for the deployment's own provider are checked. An entry for a provider it is not keyed for is the entire point of the provider dimension — its model is not expected to be registered, and warning about it would teach people to ignore the warning. Signed-off-by: jasper blues <jasper@liberation-data.com>
…rties Two things this feature was missing for the people who have to consume it. It added a config shape with no supported way to read it. listRoles returns role NAMES, and the resolver that can answer "what model does this role mean" is constructed privately inside ConfigurableModelProvider — so an application with a settings screen has to read the raw properties and reimplement the precedence between the flat and nested shapes. That is exactly how a settings screen ends up disagreeing with what actually runs. ModelProvider gains configuredOptionsForRole, sharing one lookup with the resolver so the two cannot drift, and deliberately narrower: deployment provider only, no user key, no application resolvers, so the answer does not depend on who asks or on which thread. And `roles` went in as the second constructor parameter of a data class, renumbering the three after it for anyone constructing it positionally. It reads better next to `llms`, but that is not worth a source break; moved to the end with a comment saying why it sits away from its sibling. Signed-off-by: jasper blues <jasper@liberation-data.com>
supportsStreaming and supportsThinking now resolve the role before choosing a model, but nothing asserted it. The two models in this test disagree, so answering from the default would be visible rather than merely uncovered. Signed-off-by: jasper blues <jasper@liberation-data.com>
java.util.Collections inline where an import was needed, and a warning built by concatenation where this repo uses raw strings everywhere else. The startup hunk for the flat map is now byte-identical to the one on #1888, so whichever lands first the other merges clean. Signed-off-by: jasper blues <jasper@liberation-data.com>
igordayen
left a comment
There was a problem hiding this comment.
@jasperblues - Jasper , this PR is super-critical, requires regression testing, especially in the area of context propagation. Suggesting to handle this post-release 2.0.0
| action: Action?, | ||
| ): O { | ||
| @Suppress("NAME_SHADOWING") | ||
| val interaction = withRoleResolved(interaction) |
There was a problem hiding this comment.
why not to intro diff name
There was a problem hiding this comment.
The shadowing is deliberate, and I would argue for keeping it — but the @Suppress is doing a poor job of saying so, which is fair.
The point is that after this line the resolved interaction is the interaction for the rest of the method. Introducing resolvedInteraction leaves the unresolved interaction parameter in scope alongside it, and the two differ only in whether a role has been turned into a concrete model plus its hyperparameters. Anyone adding code to these methods later can then reach for the wrong one, and the result is not a compile error — it is a call that silently skips role resolution and runs on the default model. That is precisely the class of bug this PR exists to prevent.
Shadowing makes the wrong one unreachable. The @Suppress is the marker that says so.
What I can improve is that the reasoning is currently invisible. Happy to replace the bare @Suppress with a comment stating it, on all four sites.
If you would still rather have distinct names, I will do it — it is mechanical and the compiler catches every reference — but I think it trades a real safety property for a lint warning.
There was a problem hiding this comment.
Done — the reasoning is now in the code at all four sites rather than left for the reader to infer:
// Shadowed deliberately. After this line the resolved interaction IS the interaction for
// the rest of the method, and shadowing makes the unresolved one unreachable. A distinct
// name would leave both in scope, differing only in whether a role has become a concrete
// model plus its hyperparameters - and picking the wrong one is not a compile error, it is
// a call that silently skips role resolution and runs on the default model.
@Suppress("NAME_SHADOWING")
val interaction = withRoleResolved(interaction)Offer stands: if you would still rather have distinct names, say so and I will do it.
…gation Two review points. The cache bound was a magic 500 with a hand-wave for a comment. It is an operational trade — memory against how often a deployment rebuilds a service for a key it has seen — and the right number depends on how many distinct keys are concurrently active, which only the deployment knows. Now embabel.models.credential-service-cache-size, with a test that pins LRU eviction at a configured bound of 2 so "exceeding it costs construction, never correctness" is asserted rather than claimed. Propagation coverage, as asked: the AgentProcess and model-selection ThreadLocals now have a test that they propagate TOGETHER, since one wrapper restores both and a badly nested try/finally would drop the outer one silently. Plus nested parallelMap, because the parallel tool loop fans out from work already on a worker thread and one-hop propagation would look fine in a single-level test. Writing that second one turned up a trap worth recording: parallelMap submits to its executor and then blocks on join(), so nesting it on a BOUNDED pool deadlocks - the outer tasks hold every thread waiting for inner tasks that can never be scheduled. The test now uses an unbounded executor and says why, and carries a @timeout so a regression fails in seconds rather than hanging a build. Signed-off-by: jasper blues <jasper@liberation-data.com>
|
Flagging two hunks on this branch as not-final, before anyone reviews them as settled. The intended rule for unresolvable model names in configuration is: always fail at startup, except when the deployment is BYOK, where resolution falls back to the placeholder LLM. A deployment that holds a key and has a typo should die on startup; a deployment that holds no key yet legitimately names models nothing has registered. Two things here do not follow that rule.
So the rule wants applying at both sites, plus the embedding-services check, which is currently fatal with no placeholder to fall back to and needs its own decision. Both resolved, and the rule now lives in one place rather than two: Embedding services get the same gate but no fallback. There is no embedding placeholder, and there should not be one: an embedding model is a schema commitment and nothing can safely stand in for one. So the gate decides only whether the deployment starts; asking for the service still throws. The shared hunk is byte-identical with #1888, verified with |
…contract Two review points, both fair: the code was relying on the reader inferring intent. The bare @Suppress("NAME_SHADOWING") looked like silencing a warning. It is a safety property. After that line the resolved interaction IS the interaction for the rest of the method, and shadowing makes the unresolved one unreachable — a distinct name would leave both in scope, differing only in whether a role has become a concrete model plus its hyperparameters, and reaching for the wrong one is not a compile error but a call that silently skips role resolution. Said so, at all four sites. The ordering contract was only discoverable by reading AgentPlatformConfiguration. It is now on RoleResolver, where someone writing one will find it: application resolvers always precede the platform's own, Ordered applies between them, and ties fall back to registration order and should not be relied on. Signed-off-by: jasper blues <jasper@liberation-data.com>
|
Some earlier comments on this PR have been edited in place rather than corrected by follow-ups, so the thread reads as one current account rather than a trail of revisions. GitHub keeps the original text under the "edited" control on each comment. The substantive change: unresolvable model names in @igordayen — flagging directly, since one of the edited comments is a reply to you and reverses a claim in it. |
|
t startup -- Time elapsed: 0.001 s <<< ERROR! [ERROR] com.embabel.common.ai.model.RoleResolutionTest$UnsatisfiableRoles.a nested role naming no model at all warns -- Time elapsed: 0.001 s <<< ERROR! [ERROR] com.embabel.common.ai.model.RoleResolutionTest$UnsatisfiableRoles.a configured role whose model is not registered throws -- Time elapsed: 0.001 s <<< ERROR! [ERROR] com.embabel.common.ai.model.RoleResolutionTest$UnsatisfiableRoles.an unsatisfiable role never silently resolves to the default LLM -- Time elapsed: 0.001 s <<< ERROR! [INFO] Tests run: 0, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.006 s -- in com.embabel.common.ai.model.RoleResolutionTest |
…le tests doTransform and its variants are entry points on LlmOperations in their own right, not only reachable through createObject, so a role named there silently ran on the default model. Resolution is idempotent, so the createObject path pays nothing. The four RoleResolutionTest failures were left by the commit that made checkNestedRoles fatal: they still asserted the older warn-at-startup behaviour and so died in the constructor. They now assert the rule the code enforces, and one is repurposed to cover the awaiting-a-key side of the gate. Also: the embedding-services check gets the same gate; Asyncer states the context it must propagate; CredentialLlmServiceFactory no longer claims provider modules ship an implementation, since none does yet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|



Closes #1889.
Why
embabel.models.llmsmaps a role to one model name, and a model belongs to one provider. So a role pins its provider: give a deployment only an Anthropic key and every OpenAI-named role becomes unsatisfiable — which used to fail the Spring context at startup.There was nowhere to fix it from outside either.
ByRolewas a static map lookup consulting no strategy,ModelSelectionCriteriais sealed, andAutoLlmSelectionCriteriaResolvertakes no arguments so it cannot know the caller or the active key. The only way out waswithLlmService(...), which skipsModelProviderentirely.What
ByRolenow routes through a resolver chain, consulted inOrderedorder, each returningnullto delegate. The platform'sConfigurableRoleResolverruns last, so existing configuration is unchanged.RoleResolutionisOptions(model plus tuning),Credential(a user's key — the platform looks the role up under that provider, builds the service and caches it), orService(one you built).ModelSelectionContextcarries who the call is for and which key is active, set at the application's request boundary.Optional config for deployments whose provider is not fixed:
The flat
llmsmap still works. With no user key active, the provider is whichever supplies thedefault-llm, so single-provider deployments need no context. Call sites are unchanged:ai.withLlmByRole("cheapest").Roles can now carry hyperparameters, not just a model name — merged underneath anything the caller set explicitly, applied once per operation and only when a role is named.
Guide today
Three classes, all in application code:
Plus
UserModelFactory, which builds and caches a service per (provider, model, key).Every provider and model name is compiled in, so retuning
chatis a redeploy. A role yields aString, so no temperature or timeout travels with it. BYOK and server-default take different code paths. And only code that injectsUserLlmResolverparticipates — RAG enhancement and ranking still use the static map and never see the user's key.Guide after this
LlmProvider,LlmRoleandUserLlmResolverall go, andUserModelFactoryshrinks to a builder — the platform now owns the caching, keyed on (provider, key digest, model). The role table moves to the yaml above. Two beans remain, for the only things that are genuinely Guide's business:Plus a
CredentialLlmServiceFactory, which is what is left ofUserModelFactoryonce the caching moves to the platform:This one is required, and no implementation ships with the platform yet. An earlier revision of this description implied otherwise. Without it a
RoleResolution.Credentialresolves to nothing and the role fails withNoSuitableModelExceptionplus a log line naming the provider nothing handled. Provider-supplied factories are follow-up work: they belong with the provider modules, and a pure BYOK deployment deliberately has none of those on the classpath.One path for BYOK and server-keyed alike.
Being precise about the reach, since an earlier revision of this description overstated it: a call participates by naming a role.
ai.withLlmByRole("chat")gets the user's key without opting in, wherever it is called from, including inside the framework. A call that names a model or asks forautodoes not — it resolves against deployment configuration exactly as before.LlmRankeris the one to know about: it usesauto, so ranking still runs on the deployment's own key after this PR. Pointing it at a role is a separate change.What did need fixing for that to be true:
doTransformand its variants are entry points onLlmOperationsin their own right, not only reachable throughcreateObject, and they were not resolving roles. They are now. Resolution is idempotent — it replaces the role criteria with a pre-resolved one — so thecreateObjectpath that already resolved pays nothing.Behaviour changes
cheapestsilently becomes the most expensive model in the deployment.llmsmap names models the deployment is keyed for, so using them would bill the wrong party.Tests
32 new across
RoleResolutionTestandRoleConfigurationBindingTest: provider dimension, unsatisfiable roles, role-carried options and caller precedence, resolver ordering and context, and BYOK including per-key isolation and reuse.The binding test exists because
LlmOptions.withModel()records selection criteria while configuration binding sets themodelfield — the first run failed on exactly that, henceLlmOptions.modelNamereading both.For review
ModelSelectionContextHolderis aThreadLocal, so it does not cross threads the application spawns. Documented. The alternative was changingModelProvider.getLlm's signature and every caller.ProviderInitializationis still discarded inAgentPlatformConfiguration, so nothing can enumerate providers with live credentials. Only matters for a failover resolver — follow-up issue.SetupRequiredLlm(booting with no keys at all) is Add an embabel-agent-starter-byok, so a BYOK app can boot without keys #1888, deliberately not duplicated here.