Skip to content
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: constrain custom conversation handler event version #2958

Merged

Conversation

sobolk
Copy link
Member

@sobolk sobolk commented Oct 15, 2024

Description of changes

Change how @conversation directive accepts custom conversation handler.

From:

@conversation(
  aiModel: "anthropic.claude-3-haiku-20240307-v1:0", 
  systemPrompt: "Hello, world!",
  functionName: "FnCustomHandlerChatBot",
  eventVersion: '1.0'
)

To:

@conversation(
  aiModel: "anthropic.claude-3-haiku-20240307-v1:0", 
  systemPrompt: "Hello, world!",
  handler: {
    functionName: "FnCustomHandlerChatBot",
    eventVersion: '1.0'
  }
)

While still accepting older version. Older version will be removed upon GA.

Motivation

This is a companion PR to aws-amplify/amplify-backend#2089 .
And a pre-requisite for https:/aws-amplify/amplify-api-next/pull/369 .

These PRs setup a versioning for the event protocol between conversation route resolvers and lambda handler.

The protocol versioning works as follows:

  1. We plumb event version through custom conversation handler -> schema -> transformer at runtime.
  2. We're using subset of semver, i.e. <major>.<minor>. Patch is meaningless for protocol versioning.
  3. We assert that protocol version must match on <major> digit. (mismatches within minor version should result in disabled functionality, not runtime errors).

This prepares us for future handling of 2.x and higher versions of event. Where we can:

  1. Either create a payload compatible to provided custom handler.
  2. Or fail with validation and ask customer to provide matching version of custom handler.

(This decision will be made later, when break actually happens, in this PR we establish necessary primitives to handle it in the future, and assert correctness of current state where we only have single major version).

CDK / CloudFormation Parameters Changed

NONE.

Description of how you validated changes

Added unit tests.

Checklist

  • PR description included
  • yarn test passes
  • E2E test run linked
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)
  • New AWS SDK calls or CloudFormation actions have been added to relevant test and service IAM policies
  • Any CDK or CloudFormation parameter changes are called out explicitly

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@@ -9,6 +9,8 @@ if [ -z "$PR_NUM" ]; then
exit
fi

export NODE_OPTIONS=--max-old-space-size=4096
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

@sobolk sobolk changed the title feat: constrain custom conversation handler version feat: constrain custom conversation handler event version Oct 15, 2024
@sobolk sobolk marked this pull request as ready for review October 15, 2024 22:53
@sobolk sobolk requested review from a team as code owners October 15, 2024 22:53
p5quared
p5quared previously approved these changes Oct 16, 2024
atierian
atierian previously approved these changes Oct 16, 2024
Copy link
Member

@atierian atierian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I requested a review from @palpatim since this changes the public API of the @conversation directive.

Comment on lines 113 to 122
if (directive.handler && directive.functionName) {
throw new Error("'functionName' and 'handler' are mutually exclusive");
}
if (directive.handler) {
const eventVersion = semver.coerce(directive.handler.eventVersion);
if (eventVersion?.major !== 1) {
throw new Error(
`Unsupported custom conversation handler. Expected eventVersion to match 1.x, received ${directive.handler.eventVersion}`,
);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems more appropriate to house in the transformer's validate method.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a feeling this is too deep in the implementation details, but couldn't find better place.
I'll take a stab at validate method, thanks for the pointer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@sobolk sobolk dismissed stale reviews from atierian and p5quared via b173a92 October 16, 2024 16:13
palpatim
palpatim previously approved these changes Oct 16, 2024
atierian
atierian previously approved these changes Oct 16, 2024
@sobolk sobolk dismissed stale reviews from atierian and palpatim via 7ff0496 October 16, 2024 16:37
@atierian atierian enabled auto-merge (squash) October 16, 2024 17:24
@atierian atierian merged commit 083fe17 into aws-amplify:main Oct 16, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants