-
Notifications
You must be signed in to change notification settings - Fork 401
Add embabel-agent-starter-byok so a BYOK app can boot without keys #1890
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 10 commits
94cb2c9
2f2e407
822eb9c
4acd0d9
ed9ee77
7d71805
db5da46
251832a
9d4c658
f4b3750
22f290c
4b28a12
eb16bc4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| /* | ||
| * Copyright 2024-2026 Embabel Pty Ltd. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package com.embabel.agent.spi | ||
|
|
||
| /** | ||
| * Marks an [LlmService] that stands in for a model this deployment does not yet have a key for. | ||
| * | ||
| * A placeholder completes nothing. Its only job is to give the platform something to resolve | ||
| * before any key is supplied, so that "no key yet" surfaces as an actionable error at the call | ||
| * that needed an LLM rather than as a failure to start. | ||
| * | ||
| * The platform treats the presence of a placeholder as the deployment's own statement that keys | ||
| * arrive at runtime, and relaxes exactly one thing on the strength of it: model names in | ||
| * configuration that nothing has registered are expected rather than fatal. Every other | ||
| * deployment keeps failing fast on a name it cannot resolve, because there a name that resolves | ||
| * to nothing is a typo. | ||
| * | ||
| * Implemented by `SetupRequiredLlm` in `embabel-agent-byok-autoconfigure`. It lives here, next to | ||
| * [LlmService], because `com.embabel.common.ai.model.ConfigurableModelProvider` has to recognise | ||
| * a placeholder without depending on the BYOK module - carrying the placeholder without dragging | ||
| * in that module is the reason the module exists. | ||
| */ | ||
| interface PlaceholderLlmService |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ | |
| package com.embabel.common.ai.model | ||
|
|
||
| import com.embabel.agent.spi.LlmService | ||
| import com.embabel.agent.spi.PlaceholderLlmService | ||
| import com.embabel.common.util.indent | ||
| import com.embabel.common.util.loggerFor | ||
| import org.springframework.boot.context.properties.ConfigurationProperties | ||
|
|
@@ -68,11 +69,44 @@ class ConfigurableModelProvider( | |
| private val defaultLlm = | ||
| if (llms.isNotEmpty()) | ||
| llms.firstOrNull { it.name == properties.defaultLlm } | ||
| ?: placeholderLlm() | ||
| ?: throw IllegalArgumentException( | ||
| "Default LLM '${properties.defaultLlm}' not found. Set the 'embabel.models.default-llm' property to one of the available models: ${llms.map { it.name }}.") | ||
| else | ||
| throw IllegalArgumentException("No models detected. Ensure that at least one Embabel Agent Starter (e.g. embabel-agent-starter-openai) is on the classpath and models are loaded into it.") | ||
|
|
||
| /** | ||
| * Whether this deployment is waiting for a key rather than misconfigured. | ||
| * | ||
| * True exactly when `default-llm` resolved to a [PlaceholderLlmService] - either because it | ||
| * names one, or because the model it names is not registered and a placeholder stands in. That | ||
| * is the deployment stating that keys arrive at runtime, and it is the only thing that makes an | ||
| * unresolvable model name in configuration expected rather than a typo. | ||
| * | ||
| * A deployment that has a key resolves `default-llm` to a real model and so is never in this | ||
| * mode, even with a placeholder registered alongside - which is what a BYOK starter next to a | ||
| * provider starter looks like. | ||
| */ | ||
| private val setupRequired: Boolean = defaultLlm is PlaceholderLlmService | ||
|
|
||
| /** | ||
| * The registered placeholder, if this deployment carries one. | ||
| * | ||
| * Deliberately structural rather than by name: `com.embabel.agent.spi` owns the marker, and | ||
| * this class must not depend on the BYOK module that implements it. | ||
| */ | ||
| private fun placeholderLlm(): LlmService<*>? = | ||
| llms.firstOrNull { it is PlaceholderLlmService } | ||
| ?.also { | ||
| // Named, because degrading a real model to the placeholder would otherwise hide the | ||
| // case where the key IS set and the model simply failed to register. | ||
| logger.warn( | ||
| "Default LLM '{}' is not registered; falling back to the '{}' placeholder. " + | ||
| "Calls will fail with an actionable 'no LLM configured' error until a key is supplied. Available: {}", | ||
| properties.defaultLlm, it.name, llms.map { it.name }, | ||
| ) | ||
| } | ||
|
|
||
| // Compute this lazily as embedding services may not be available | ||
| private fun defaultEmbeddingService() = | ||
| embeddingServices.firstOrNull { it.name == properties.defaultEmbeddingModel } | ||
|
|
@@ -81,14 +115,41 @@ class ConfigurableModelProvider( | |
| init { | ||
| properties.llms.forEach { (role, model) -> | ||
| if (llms.none { it.name == model }) { | ||
| error("LLM '$model' for role $role is not available: Choices are ${llms.map { it.name }}") | ||
| // Fatal, unless this deployment is waiting for a key. A name that resolves to | ||
| // nothing is a typo in a deployment that has one, and letting it start would move | ||
| // the failure to whichever unrelated call first asks for that role. A deployment in | ||
| // setup-required mode has no models registered yet by definition, so the same name | ||
| // is expected there and only worth reporting. | ||
| if (setupRequired) { | ||
| logger.warn( | ||
| "LLM '{}' for role '{}' is not registered. This deployment is awaiting a key, so that is expected; " + | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above — flagged the 68:12 count in favour of stacked |
||
| "the role will report 'no LLM configured' until one is supplied. Available: {}", | ||
| model, role, llms.map { it.name }, | ||
| ) | ||
| } else { | ||
| error("LLM '$model' for role $role is not available: Choices are ${llms.map { it.name }}") | ||
| } | ||
| } | ||
| } | ||
| logger.info(infoString(verbose = true)) | ||
|
|
||
| properties.embeddingServices.forEach { (role, model) -> | ||
| if (embeddingServices.none { it.name == model }) { | ||
| error("Embedding model '$model' for role $role is not available: Choices are ${embeddingServices.map { it.name }}") | ||
| // The same gate as the LLM roles above, and for the same reason: an unresolvable | ||
| // name is a typo in a deployment that holds a key, and expected in one still | ||
| // waiting for it. There is no fallback here, though, and there should not be - | ||
| // an embedding model is a schema commitment and nothing can stand in for one. | ||
| // The gate decides only whether the deployment STARTS; asking for the service | ||
| // still throws. | ||
| if (setupRequired) { | ||
| logger.warn( | ||
| "Embedding model '{}' for role '{}' is not registered. This deployment is awaiting a key, " + | ||
|
jasperblues marked this conversation as resolved.
|
||
| "so that is expected; asking for that role will still fail. Available: {}", | ||
| model, role, embeddingServices.map { it.name }, | ||
| ) | ||
| } else { | ||
| error("Embedding model '$model' for role $role is not available: Choices are ${embeddingServices.map { it.name }}") | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd"> | ||
| <modelVersion>4.0.0</modelVersion> | ||
| <parent> | ||
| <groupId>com.embabel.agent</groupId> | ||
| <artifactId>embabel-agent-autoconfigure</artifactId> | ||
| <version>1.5.0-SNAPSHOT</version> | ||
| <relativePath>../../pom.xml</relativePath> | ||
| </parent> | ||
| <artifactId>embabel-agent-byok-autoconfigure</artifactId> | ||
| <packaging>jar</packaging> | ||
| <name>Embabel Agent Autoconfiguration BYOK</name> | ||
| <description>Placeholder LLM service allowing a pure BYOK deployment to start with no keys configured</description> | ||
| <url>https://github.com/embabel/embabel-agent</url> | ||
|
|
||
| <scm> | ||
| <url>https://github.com/embabel/embabel-agent</url> | ||
| <connection>scm:git:https://github.com/embabel/embabel-agent.git</connection> | ||
| <developerConnection>scm:git:https://github.com/embabel/embabel-agent.git</developerConnection> | ||
| <tag>HEAD</tag> | ||
| </scm> | ||
|
|
||
| <dependencies> | ||
| <dependency> | ||
| <groupId>com.embabel.agent</groupId> | ||
| <artifactId>embabel-agent-api</artifactId> | ||
| </dependency> | ||
|
|
||
| <dependency> | ||
| <groupId>com.embabel.agent</groupId> | ||
| <artifactId>embabel-agent-byok</artifactId> | ||
| </dependency> | ||
|
|
||
| <dependency> | ||
| <groupId>com.embabel.agent</groupId> | ||
| <artifactId>embabel-agent-test-internal</artifactId> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| </dependencies> | ||
|
|
||
| </project> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| /* | ||
| * Copyright 2024-2026 Embabel Pty Ltd. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package com.embabel.agent.autoconfigure.models.byok; | ||
|
|
||
| import com.embabel.agent.config.models.byok.SetupRequiredLlmConfig; | ||
| import org.springframework.boot.autoconfigure.AutoConfiguration; | ||
| import org.springframework.boot.autoconfigure.AutoConfigureBefore; | ||
| import org.springframework.context.annotation.Import; | ||
|
|
||
| /** | ||
| * Autoconfiguration for Bring Your Own Key deployments. | ||
| * <p> | ||
| * Unlike the provider autoconfigurations, this registers no real models and requires no API key. | ||
| * It contributes only the {@code setup-required} placeholder LLM, which lets a deployment holding | ||
| * no provider key start up and resolve {@code embabel.models.default-llm}. Keys arrive at runtime | ||
| * and reach a call through {@code PromptRunner.withLlmService(...)}. | ||
| * <p> | ||
| * Runs before the platform autoconfiguration so the placeholder bean exists by the time the model | ||
| * provider is built. | ||
| */ | ||
| @AutoConfiguration | ||
| @AutoConfigureBefore(name = {"com.embabel.agent.autoconfigure.platform.AgentPlatformAutoConfiguration"}) | ||
|
jasperblues marked this conversation as resolved.
|
||
| @Import(SetupRequiredLlmConfig.class) | ||
| public class AgentByokAutoConfiguration { | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
multiline comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to change these, but the repo currently goes the other way, so flagging before I do.
Counting indented multi-line comments in
embabel-agent-api/src/main/kotlin://runs of 3+ lines: 68/* ... */non-KDoc blocks: 12So stacked
//is roughly 6:1 the house style —AgentProcessChatbot.kt,TokenBudgetConversationFormatter.ktandHybridUtilityPlanner.ktare three at random. KDoc/** */is of course used everywhere for declarations; this is only about explanatory comments inside function bodies, which is what these are.I have no strong preference and will switch all three if you want them as block comments — just say and it is one edit. If the intent is that the repo should move to
/* */generally, that is worth doing as its own sweep rather than only on the lines this PR happens to touch, otherwise the file ends up mixed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point, RJ reviewed the code and requested to change all occurrences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That settles it — done, and my count was measuring the wrong thing.
68 stacked-
//against 12 block comments describes a codebase that has not caught up with what Rod asked for, not the convention it is meant to follow. Arguing from that count was the error, not the conclusion I drew from it.Converted the multi-line explanations this PR introduces, in
ConfigurableModelProvider,ConfigurableModelProviderTest,SetupRequiredLlmTestandPureByokWithRolesTest. 48 tests green.Deliberately scoped to blocks this PR adds — I did convert the pre-existing ones in files it merely touches at first, then backed that out: it widened the diff into
InstrumentedChatModel,SpringAiLlmMessageSenderandconverters.ktfor no reviewable reason. The sweep Rod asked for is better done as a sweep than smuggled through whichever PRs happen to touch a file. Happy to do that as its own change if you want it.