-
Notifications
You must be signed in to change notification settings - Fork 79
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
feat(conversation): update directive input for tool definition for model list queries #3013
feat(conversation): update directive input for tool definition for model list queries #3013
Conversation
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.
Does the validation allow for mixed-mode tool maps? In other words, the map contains both a queryName
style and a modelOperation style?
(I can’t think of why this would be necessary, but if it’s supported, we should test it)
Approved assuming the answer is "no", or that we'll add tests for mixed-mode support later.
Also approved as API BR since this is a change to the conversation directive API.
A single tool definition cannot have both But a single
If I need to make any other changes to this branch before merging, I'll include a test case for mixed tool definitions. Otherwise, I'll include it in a follow up. |
packages/amplify-graphql-conversation-transformer/src/tools/process-tools.ts
Outdated
Show resolved
Hide resolved
The failing test is |
Related PR
Problem
Description of changes
Updates the
ToolMap
input in the@conversation
directive definitionAs referenced in the inline documentation, this is essentially a fake union type.
New DX
Why only support
list
queries?list
queries becauseget
queries require the LLM to have the primary key of the relevant model. It's possible to make this work today by including the primary key in theaiContext
of a message, however this approach hasn't proven dependable or an acceptable DX. We're planning on exploring alternative ways to supportget
queries; until then, we're constraining model generated query tools tolist
.Why not just have
data-schema
generate thequeryName
rather than changing the directive API?This was the initial route I planned, but it's the wrong one as it would force
data-schema
to take a dependency on the implementation details of the model transformer.What about custom named model generated list queries?
Supported 😄
CDK / CloudFormation Parameters Changed
N/A
Issue #, if available
N/A
Description of how you validated changes
Checklist
yarn test
passesE2E test run linkedRelevant documentation is changed or added (and PR referenced)New AWS SDK calls or CloudFormation actions have been added to relevant test and service IAM policiesAny CDK or CloudFormation parameter changes are called out explicitlyBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.