-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add output_json_schema method to Agent class #3454
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?
Conversation
|
@DouweM is there a source of truth I can reference for what the schema output should be for tests? I think I will get stuck figuring this out so any guidance will be appreciated. Edit: I added snapshots of expected output to the tests. Please review them carefully as I'm guessing what they are supposed to be. |
| return self.text_processor is not None | ||
|
|
||
| @abstractmethod | ||
| def dump(self) -> JsonSchema: |
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.
Can we call the method json_schema()?
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.
If allows_deferred_tools is True, we should add the schema of DeferredToolRequests to the output union
Same for allows_image and BinaryImage.
So we may have to implement that here, and then have the subclass methods call super() and combine that with the union somehow.
| return 'image' | ||
|
|
||
| def dump(self) -> JsonSchema: | ||
| raise NotImplementedError() |
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.
You can use the schema of BinaryImage
| processors.append(processor) | ||
| return UnionOutputProcessor(processors).object_def.json_schema | ||
|
|
||
| return self.processor.object_def.json_schema |
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.
We don't need the toolset logic; this one should always be present
| processor.object_def.name = tool_def.name | ||
| processor.object_def.description = tool_def.description | ||
| processors.append(processor) | ||
| return UnionOutputProcessor(processors).object_def.json_schema |
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.
Hmm this is a bit ugly :) Can we add a new json_schema property to OutputToolset, and then generate this inside OutputToolset.build, where we already have the processors etc?
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.
If self.allows_text is enabled, we should add {type: string} to the union. (similar to what I said above). We may need some logic to take a UnionOutputProcessor and basically add a new branch to it, (instead of ending up with nested unions that could be flattened)
| if isinstance(output, ObjectOutputProcessor): | ||
| processor = output | ||
| else: | ||
| processor = ObjectOutputProcessor(output=output, strict=strict) |
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 did we need to change this?
Closes #3225
Note: there is still some discussion whetheroutput_json_schemashould be a property or method: #3225 (comment)Need help with:
AutoOutputSchemaexpected output schema?ImageOutputSchemaexpected output schema?ToolOutputSchema,toolsetcan beNone... how to handle?