-
Notifications
You must be signed in to change notification settings - Fork 184
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
Add JsonSchemaRule #1165
Add JsonSchemaRule #1165
Conversation
Question: should this be the mechanism by which we enable features like OpenAI Structured Outputs? If so, we'll need to make the prompt rendering happen at the Driver level rather than the Task level. |
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
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.
Nice!
Approving because I think there is some immediate value to this. Also, I like the choice of using a rule to enforce json schema, since that's the same as what you'd do now. It's intuitive imo.
Even if we need to change some things to make it work with say OpenAI Structure Outputs, I like this as a "Rule" from a API perspective.
@@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
### Added | |||
- Parameter `meta: dict` on `BaseEvent`. | |||
- `AzureOpenAiTextToSpeechDriver`. | |||
- `JsonSchemaRule` for instructing the LLM to output a JSON object that conforms to a 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.
Responding with an inline comment, so it's easier to reply.
Question: should this be the mechanism by which we enable features like OpenAI Structured Outputs? If so, we'll need to make the prompt rendering happen at the Driver level rather than the Task level.
If we didn't use this mechanism for OpenAI Structured Outputs, then what would the mechanism for that look like? Would it require passing a json schema? (I would expect so) If it requires passing a json schema, then we'd have two choices, which doesn't seem like the right approach (imagine that we had taken that approach for tools 🤮 ).
Seems like making prompt rendering at the Driver level is the way forward because I don't really see duplicate ways of passing in a json schema as a viable option. This could technically wait until introducing native structured outputs, right?
(Also, if we do use this rule for native structure outputs, what mechanism will we use to opt-in/out of structure outputs when a driver supports it? Should we that the same approach as a use_native_tools
flag?)
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.
Would it require passing a json schema?
Yes, it'd likely require passing a schema to the Prompt Driver 🤢.
Rendering at the Prompt Driver level allows us to lean on more provider-specific nuances so I think it's a worthwhile change. But as you point out it's not required for this PR -- this can still serve as a good intermediate solution.
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.
Yay!
Optional suggestions on the sample
docs/griptape-framework/structures/src/json_schema_rule_pydantic.py
Outdated
Show resolved
Hide resolved
13498e5
to
297d864
Compare
297d864
to
aed5f6c
Compare
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.
x2
aed5f6c
to
2805494
Compare
Describe your changes
Added
JsonSchemaRule
for instructing the LLM to output a JSON object that conforms to a schema.Issue ticket number and link
NA
📚 Documentation preview 📚: https://griptape--1165.org.readthedocs.build//1165/