Use message history instead of event payload for conversation handler #2047
Conversation
🦋 Changeset detectedLatest commit: a220cee The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
sobolk
changed the title
sobolk
force-pushed
the
conversation-history
branch
from
bb4b697
to
767e73b
Compare
| return messages.reduce((acc, current) => { | ||
| // Bedrock expects that message history is user->assistant->user->assistant->... and so on. | ||
| // The chronological order doesn't assure this ordering if there were any concurrent messages sent. | ||
| // Therefore, conversation is ordered by user's messages only and corresponding assistant messages are inserted | ||
| // into right place regardless of their createdAt value. | ||
| // This algorithm assumes that GQL query returns messages sorted by createdAt. |
There was a problem hiding this comment.
Would this better be suited in the AppSync query?
There was a problem hiding this comment.
It's apparently not easy with NoSQL like DDB. It would require custom resolver to pretty much encapsulate same logic. @atierian is that correct?
Some other considerations:
- Number of messages per conversatonId is expected to be rather small set (which we cap at 1000 items, might be configurable in the future).
- We have to have all these messages in memory anyways to send it to Bedrock.
- The DDB and query provide linear view if history audit by createdAt. The special ordering and filtering is for Bedrock API requirements. Appsync doesn't know about Bedrock calls, Lambda knows, so this adaptation of data seem to belong to Lambda.
- This isn't one-way door. I.e. we can move ordering back to special list query and it's resolver later and take this out. Without breaking (multiple application of this algorithm is idempotent).
There was a problem hiding this comment.
It's possible to do this logic in the AppSync resolver function, however the downsides outweigh the benefits.
For context, there's a single GraphQL query per conversation route for listing messages (listConversationMessage<RouteName>). The list query is used by clients and now the Lambda function.
The logic for ordering and omitting in-flight user messages added here is specific to Bedrock's API requirements, not intrinsic to querying message history. We don't want to omit in-flight / currently unanswered user messages from responses from that list query because it could lead to confusing behavior in client applications.
I see a couple options:
- Bedrock specific history logic lives in lambda (these changes).
- We introduce a flag in the list query for bimodal behavior.
- We introduce a separate GraphQL query for outputting messages in this structure.
Of those, 1. feels like the right choice for separation of responsibilities and maintenance.
| /** | ||
| * @deprecated This field is going to be removed in upcoming releases. | ||
| */ | ||
| messages?: Array<ConversationMessage>; |
There was a problem hiding this comment.
why not remove it right now since this is in preview anyways?
There was a problem hiding this comment.
I was going back and forth on this one and ultimately decided to keep backwards compatibility.
Factors that influenced this direction:
- This is a change in protocol that requires upstream sender to adapt as well. It would disturb "custom handler scenario" in preview until upstream ships.
- The alternative was to keep these changes on feature branch. But this seems to be the only change for GA that disturbs the protocol, and maintaining compatibility seems to be less effort than long running branch.
- It's a drill of how we'd do this post GA.
There was a problem hiding this comment.
But. thanks to preview we'll be able to take this out the moment upstream ships.
| messageHistoryQuery: { | ||
| getQueryName: string; | ||
| getQueryInputTypeName: string; | ||
| listQueryName: string; | ||
| listQueryInputTypeName: string; | ||
| listQueryLimit?: number; | ||
| }; |
There was a problem hiding this comment.
Just for my understanding, these values are populated by AppSync, and are these constants or dynamic?
There was a problem hiding this comment.
these values are populated by AppSync
Yes.
are these constants or dynamic?
These are dynamic. There might be multiple "conversational routes" and associated models in AppSync. AppSync provides these names so that we do query correct route.
Problem
Conversation handler receives messages in the event. The problem is that event size is constrained to 256 KB for asynchronous invocations. https://docs.aws.amazon.com/lambda/latest/dg/gettingstarted-limits.html .
When conversation is long enough and/or contains images this limit is hit and lambda can't be called.
Previous logic as defined in upstream resolver can be seen here. Note that it's using outdated gql schema.
Changes
event.messagesfor now to be backwards compatible until upstream components absorb this change. This will be removed later.GraphqlRequestExecutorin theai-constructssince we've hit the rule of 3 and refactor other components that execute GQL queries to use it.Validation
Added tests, including e2e tests.
Manual tests via snapshot release with upstream components.
Checklist
run-e2elabel set.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.