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

Do not bundle AWS SDK if default implementation is used. #1969

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

sobolk
Copy link
Member

@sobolk sobolk commented Sep 6, 2024

Problem

Bundling Bedrock SDK is problematic in default implementation case. I.e. when data construct uses conversation handler construct under the hood.

That behavior demands that data construct bundles Bedrock SDK with it's dependencies as well. Which is not optimal.

Changes

We started conversation handler with bundling of Bedrock SDK because it was not available in Lambda runtime yet.

Since then however. Lambda has updated SDKs and they now include Bedrock. Which is also compatible with our default implementation.
Therefore, we disable bundling if default handler is used to simplify situation on data construct side.

Note

This is another solution like this one that aims to simplify caller (upstream) situation and smoothen DX for preview purposes. We do have an action item to revisit bundling on data side.

Validation

This change is not unit-testable as there's no good way to assert bundling behavior in unit tests.

However, the logical branch introduced here is already covered by e2e tests which deploy both default and custom implementation of conversation handler.

Hence no new tests are added in this PR.

Checklist

  • If this PR includes a functional change to the runtime behavior of the code, I have added or updated automated test coverage for this change.
  • If this PR requires a change to the Project Architecture README, I have included that update in this PR.
  • If this PR requires a docs update, I have linked to that docs PR above.
  • If this PR modifies E2E tests, makes changes to resource provisioning, or makes SDK calls, I have run the PR checks with the run-e2e label set.

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

Copy link

changeset-bot bot commented Sep 6, 2024

🦋 Changeset detected

Latest commit: 8466e4a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@aws-amplify/ai-constructs Patch

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 sobolk added the run-e2e Label that will include e2e tests in PR checks workflow label Sep 6, 2024
@sobolk sobolk marked this pull request as ready for review September 10, 2024 14:30
@sobolk sobolk requested a review from a team as a code owner September 10, 2024 14:30
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.

Can confirm that we're able to drop @aws-sdk/client-bedrock-runtime in the graphql-api-construct E2E tests for the conversation transformer (ref).

Also manually confirmed by successfully deploying a sandbox with a tagged release of the Data / API construct targeting this change.

🚢

@sobolk sobolk merged commit 041d041 into main Sep 10, 2024
61 checks passed
@sobolk sobolk deleted the not-bundle-bedrock-sdk branch September 10, 2024 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-e2e Label that will include e2e tests in PR checks workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants