-
Notifications
You must be signed in to change notification settings - Fork 193
Refactor structured output API to make it more flexible, support native structured output for OpenAI and Google. #443
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
Refactor structured output API to make it more flexible, support native structured output for OpenAI and Google. #443
Conversation
99b8819
to
703baa3
Compare
Qodana for JVM419 new problems were found
@@ Code coverage @@
+ 67% total lines covered
9121 lines analyzed, 6127 lines covered
# Calculated according to the filters of your coverage tool ☁️ View the detailed Qodana report Contact Qodana teamContact us at [email protected]
|
12b52ee
to
3d2962f
Compare
I've added simple method overloads to the structured output API, allowing users to avoid always configuring all parameters manually and to rely on the framework to figure out the best approach instead. It is possible now to write very little code to get the structure in common cases where no fine-grained control is required: // Prompt executor
promptExecutor.executeStructured<MyStruct>(prompt, model)
// Agent LLM contenxt
llm.writeSession {
requestLLMStructured<MyStruct>()
}
// Node
val getMyStruct by nodeLLMRequestStructured<MyStruct>() Full example is in the @devcrocod can you please check, is it better now? |
This is perfect, I just have a small suggestion - In my experience, most of the times I'd want to also give a specific system message when generating structured output (instead of relying solely on the history) |
@OfekTeken sorry, updated the original example, |
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.
I have some questions, mostly related to the design. I’d appreciate it if you could clarify them for me
*/ | ||
public data class StructuredOutputConfig<T>( | ||
public val default: StructuredOutput<T>? = null, | ||
public val byProvider: Map<LLMProvider, StructuredOutput<T>> = emptyMap(), |
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.
Why is it LLMProvider here instead of LLModel?
As I see, in PromptExecutor
, where the config will be used, all providers are already present
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.
Hm, by using the model as a key we can specify the behavior more granularly, e.g. that Gemini Flash 2.5 Lite should use Native
output, but Gemini Flash 2.5 Pro should use Manual
. But I'm not sure if this is a realistic case though. The main idea is that since providers might have differences in JSON schema formats and Natve/No Native support, specifiying the behavior per provider only would be enough. I would say we can leave it like this for now and intrduce per model mapping if there will be a real need for it, wdyt?
agents/agents-core/src/commonMain/kotlin/ai/koog/agents/core/agent/entity/AIAgentSubgraph.kt
Show resolved
Hide resolved
config = StructuredOutputConfig( | ||
default = StructuredOutput.Manual( | ||
JsonStructuredData.createJsonStructure<SelectedTools>( | ||
schemaGenerator = FullJsonSchemaGenerator, |
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.
As I see it, there’s only one generator, so why are we choosing it here?
We could just use FullJsonSchemaGenerator
by default, and if users want to use a custom one, they can specify it themselves
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.
There are multiple types of generators, and I'm again (as I explained above) chose to be a bit more explicit here for the sake of clarity, to make it easier to understand what is going on here exactly, since it's part of the core implementation. But now that I made Standard
(ex Full
) JSON schema generator a default value, this can be removed if you want.
agents/agents-core/src/commonMain/kotlin/ai/koog/agents/core/agent/entity/AIAgentSubgraph.kt
Show resolved
Hide resolved
agents/agents-core/src/commonMain/kotlin/ai/koog/agents/core/agent/session/AIAgentLLMSession.kt
Show resolved
Hide resolved
* Used to attempt to get a proper generator in the simple version of [executeStructured] (that does not accept [StructuredOutput] explicitly) | ||
* to attempt to generate an appropriate schema for the passed [KType]. | ||
*/ | ||
public val DefaultSimpleSchemaGenerators: Map<LLMProvider, JsonSchemaGenerator> = mapOf( |
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.
In my opinion, it’s not a good idea to use such global variables.
Can we put jsonSchemaGenerator into the companion object for OpenAI and Google respectively?
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.
After some consideration I decided to use "register pattern" (similar to ServiceLoader) to be able to move LLM provider specific JSON schema generators to their clients' respective modules. So now these LLM clients register these generators (formats) in global maps. This is not an ideal solution, but seems like it achieves nice balance in terms of stability/convenience. I marked these global maps with internal API annotations.
prompt/prompt-structure/src/commonMain/kotlin/ai/koog/prompt/structure/StructureFixingParser.kt
Show resolved
Hide resolved
* Note: it does not handle nullability because these might be different in different schema specs. | ||
* Implementations must handle these themselves. | ||
*/ | ||
public abstract class GenericJsonSchemaGenerator : JsonSchemaGenerator() { |
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.
Will this work if I have a custom serializer and the descriptor is overridden?
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.
generate
method accepts an instance of Json
that is used to (de)serialize objects, so yes. Everything this json instance knows about (registered subtypes, custom serializers, etc.) should be used properly by the generator.
...re/src/commonMain/kotlin/ai/koog/prompt/structure/json/generator/core/JsonSchemaGenerator.kt
Outdated
Show resolved
Hide resolved
* It is recommended to use [SimpleJsonSchemaGenerator.Default] companion object, which provides a default instance, | ||
* instead of creating new instances manually. | ||
*/ | ||
public open class SimpleJsonSchemaGenerator : GenericJsonSchemaGenerator() { |
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.
The use of Simple/Full in the naming raises questions, since it’s hard for the user to understand what Simple
means and why it’s considered simple without additional context
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.
After some considertation, I decided to rename these as follows:
Simple
->Basic
. "Basic" implementation of JSON schema, without additional capabilities such as special functions, nested objects only.Full
->Standard
. "Standard" implementation of JSON schema, with support for object definitions, refs, and certain special functions likeoneOf
/anyOf
.
Is it better now?
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.
Yes, but that’s just my personal preference :)
I’m curious how we can fill the entire JSON schema for a class? I mean all fields of json schema
3d531c9
to
a749563
Compare
a749563
to
abacafc
Compare
…ve structured output for OpenAI and Google * Add helper Gradle tasks to verify compilation on all targets * Adjust envs loading in integration tests, fix tests * Load envs in integration tests only for `jvmIntegrationTest` tasks, not for all Test tasks (we don't need credentials here) * Simple structured output API that tries to find the best approach to get the structured response on its own, without requiring the users to specify structures and modes manually.
abacafc
to
5e7ea7e
Compare
Thanks! Can't wait to use this :) |
Can't wait to use this as well - without being dramatic it makes the difference between koog.ai being usable/unusable for my use cases! Thanks @EugeneTheDev let's goo..! 🚀 (KSlack post for context) |
Fixes #377 Changes are the same as #378, it's just the updated version after the refactor of #443 Proposed changes: Add an `excludedProperties` parameter like this: ```kotlin simpleSchemaGenerator.generate( "TestClass", serializer<TestClass>(), descriptionOverrides = emptyMap(), excludedProperties = setOf("TestClass.someProperty") ) ``` --- #### Type of the change - [x] New feature - [ ] Bug fix - [ ] Documentation fix #### Checklist for all pull requests - [x] The pull request has a description of the proposed change - [x] I read the [Contributing Guidelines](https://github.com/JetBrains/koog/blob/main/CONTRIBUTING.md) before opening the pull request - [x] The pull request uses **`develop`** as the base branch - [x] Tests for the changes have been added - [x] All new and existing tests passed ##### Additional steps for pull requests adding a new feature - [x] An issue describing the proposed change exists - [x] The pull request includes a link to the issue - [ ] The change was discussed and approved in the issue - [ ] Docs have been added / updated
…ains#638) Fixes JetBrains#377 Changes are the same as JetBrains#378, it's just the updated version after the refactor of JetBrains#443 Proposed changes: Add an `excludedProperties` parameter like this: ```kotlin simpleSchemaGenerator.generate( "TestClass", serializer<TestClass>(), descriptionOverrides = emptyMap(), excludedProperties = setOf("TestClass.someProperty") ) ``` --- #### Type of the change - [x] New feature - [ ] Bug fix - [ ] Documentation fix #### Checklist for all pull requests - [x] The pull request has a description of the proposed change - [x] I read the [Contributing Guidelines](https://github.com/JetBrains/koog/blob/main/CONTRIBUTING.md) before opening the pull request - [x] The pull request uses **`develop`** as the base branch - [x] Tests for the changes have been added - [x] All new and existing tests passed ##### Additional steps for pull requests adding a new feature - [x] An issue describing the proposed change exists - [x] The pull request includes a link to the issue - [ ] The change was discussed and approved in the issue - [ ] Docs have been added / updated
PrompExecutor.executeStructured
,AIAgentLLMSession.requestLLMStructured
,nodeLLMRequestStructured
- to make it easier to use with different modes (manual and native)Fixes #366