-
Notifications
You must be signed in to change notification settings - Fork 403
Resolve LLM roles through an SPI so a role is not tied to one provider #1894
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 all commits
fb1ecff
35bb4be
3166ace
d2d47f6
453c9e8
e8f2214
88dca44
b04dff3
badb5c6
75707dc
a9e0ab0
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 |
|---|---|---|
|
|
@@ -34,6 +34,7 @@ import com.embabel.agent.spi.validation.ValidationPromptGenerator | |
| import com.embabel.chat.Message | ||
| import com.embabel.chat.UserMessage | ||
| import com.embabel.common.ai.model.AutoModelSelectionCriteria | ||
| import com.embabel.common.ai.model.ByRoleModelSelectionCriteria | ||
| import com.embabel.common.ai.model.LlmOptions | ||
| import com.embabel.common.ai.model.ModelProvider | ||
| import com.embabel.common.ai.model.ModelSelectionCriteria | ||
|
|
@@ -144,6 +145,14 @@ abstract class AbstractLlmOperations( | |
| agentProcess: AgentProcess, | ||
| action: Action?, | ||
| ): O { | ||
| // 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) | ||
|
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. why not to intro diff name
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. The shadowing is deliberate, and I would argue for keeping it — but the The point is that after this line the resolved interaction is the interaction for the rest of the method. Introducing Shadowing makes the wrong one unreachable. The What I can improve is that the reasoning is currently invisible. Happy to replace the bare 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.
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. 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. |
||
|
|
||
| val (allTools, llmRequestEvent) = getToolsAndEvent( | ||
| agentProcess = agentProcess, | ||
| interaction = interaction, | ||
|
|
@@ -260,6 +269,14 @@ abstract class AbstractLlmOperations( | |
| agentProcess: AgentProcess, | ||
| action: Action?, | ||
| ): Result<O> { | ||
| // 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) | ||
|
|
||
| val (allTools, llmRequestEvent) = getToolsAndEvent( | ||
| agentProcess = agentProcess, | ||
| interaction = interaction, | ||
|
|
@@ -312,6 +329,14 @@ abstract class AbstractLlmOperations( | |
| agentProcess: AgentProcess, | ||
| action: Action?, | ||
| ): ThinkingResponse<O> { | ||
| // 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) | ||
|
|
||
| val (allTools, llmRequestEvent) = getToolsAndEvent( | ||
| agentProcess = agentProcess, | ||
| interaction = interaction, | ||
|
|
@@ -364,6 +389,14 @@ abstract class AbstractLlmOperations( | |
| agentProcess: AgentProcess, | ||
| action: Action?, | ||
| ): Result<ThinkingResponse<O>> { | ||
| // 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) | ||
|
|
||
| val (allTools, llmRequestEvent) = getToolsAndEvent( | ||
| agentProcess = agentProcess, | ||
| interaction = interaction, | ||
|
|
@@ -409,6 +442,33 @@ abstract class AbstractLlmOperations( | |
| return response | ||
| } | ||
|
|
||
| /** | ||
| * Resolve any role named by this interaction before anything reads its options: a role can | ||
| * carry hyperparameters, and which model it means depends on the provider active for this call. | ||
| * | ||
| * Interactions naming no role are returned untouched, so the common path costs nothing. | ||
| * | ||
| * Idempotent, so a subclass may call it on a path this class has already resolved: resolution | ||
| * replaces the role criteria with a pre-resolved one, and a second call sees no role and does | ||
| * nothing. That is what lets the low-level `doTransform` entry points resolve for themselves | ||
| * without double-resolving the `createObject` path that reaches them. | ||
| */ | ||
| protected fun withRoleResolved(interaction: LlmInteraction): LlmInteraction { | ||
| val resolved = withRoleResolved(interaction.llm) | ||
| return if (resolved === interaction.llm) interaction else interaction.copy(llm = resolved) | ||
| } | ||
|
|
||
| /** | ||
| * As above, for the paths that carry options rather than a whole interaction - streaming, | ||
| * and the capability queries that pick a model without running a prompt. | ||
| */ | ||
| private fun withRoleResolved(options: LlmOptions): LlmOptions = | ||
| if (options.criteria is ByRoleModelSelectionCriteria) { | ||
| modelProvider.resolveLlmOptions(options) | ||
| } else { | ||
| options | ||
| } | ||
|
|
||
| protected fun chooseLlm( | ||
| llmOptions: LlmOptions, | ||
| ): LlmService<*> { | ||
|
|
@@ -426,18 +486,22 @@ abstract class AbstractLlmOperations( | |
| } | ||
|
|
||
| override fun supportsStreaming(options: LlmOptions): Boolean { | ||
| val llmService = chooseLlm(options) | ||
| val llmService = chooseLlm(withRoleResolved(options)) | ||
| return llmService.supportsStreaming() | ||
| } | ||
|
|
||
| override fun supportsThinking(options: LlmOptions): Boolean { | ||
| val llmService = chooseLlm(options) | ||
| val llmService = chooseLlm(withRoleResolved(options)) | ||
| return llmService.supportsThinking() | ||
| } | ||
|
|
||
| override fun createStreamingOperations(options: LlmOptions): StreamingLlmOperations { | ||
| val llmService = chooseLlm(options) | ||
| val messageStreamer = llmService.createMessageStreamer(options) | ||
| // Resolve once and stream with the SAME options. The streamer reads hyperparameters, so | ||
| // resolving only far enough to pick a model would silently drop the tuning a role carries - | ||
| // and streaming is the chat path, where that tuning matters most. | ||
| val resolved = withRoleResolved(options) | ||
| val llmService = chooseLlm(resolved) | ||
| val messageStreamer = llmService.createMessageStreamer(resolved) | ||
| return StreamingLlmOperationsImpl( | ||
| messageStreamer = messageStreamer, | ||
| objectMapper = objectMapper, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.