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: bedrock agent custom resources and constructs #207

Merged
merged 24 commits into from
Jan 26, 2024

Conversation

jstrunk
Copy link
Contributor

@jstrunk jstrunk commented Jan 19, 2024

Fixes #169

This PR implements custom resources and constructs for Bedrock Agents and Knowledge Bases and OpenSearch Serverless vector indices.

Test it with https://github.com/jstrunk/generative-ai-cdk-constructs-samples/tree/bedrock-agent/samples/bedrock-agent

New constructs:
OpenSearch Serverless

  • OpenSearchVectorCollection - OpenSearch Serverless Collection to be used as a vector store.
  • OpenSearchVectorIndex - vector index on the collection.

Bedrock

  • Knowledge Base - Only supports OpenSearch Serverless. The construct creates the OpenSearch Vector Index.
  • S3 Data Source - Configures a data source for the KB.
  • Agent - Creates an Agent, associates Knowledge Bases, prepares the Agent, and creates an Alias. The Alias is updated on any change to the agent or knowledge bases.

Missing features

  • Agent Action Groups
  • Other Vector Stores for Knowledge Bases

It includes a new function to generate physical names and deprecates the old one to avoid breaking stacks.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.

Copy link

codecov bot commented Jan 19, 2024

Codecov Report

Attention: 36 lines in your changes are missing coverage. Please review.

Comparison is base (99ddf75) 93.82% compared to head (b5c8c39) 95.07%.

❗ Current head b5c8c39 differs from pull request most recent head fa82023. Consider uploading reports for the commit fa82023 to get more accurate results

Files Patch % Lines
src/cdk-lib/bedrock/agent.ts 95.68% 26 Missing ⚠️
src/cdk-lib/bedrock/knowledge-base.ts 98.32% 5 Missing ⚠️
src/cdk-lib/bedrock/s3-data-source.ts 98.08% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #207      +/-   ##
==========================================
+ Coverage   93.82%   95.07%   +1.25%     
==========================================
  Files          19       30      +11     
  Lines        5766     7679    +1913     
  Branches      152      201      +49     
==========================================
+ Hits         5410     7301    +1891     
- Misses        356      378      +22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@oconpa
Copy link
Member

oconpa commented Jan 19, 2024

Might be best to divide this into smaller PRs, isolating 1 construct at a time. PR is showing +7k changes with other changes suggested on top of constructs, e.g. a migration from a v1 physical name funct to v2

@jstrunk
Copy link
Contributor Author

jstrunk commented Jan 19, 2024

Might be best to divide this into smaller PRs, isolating 1 construct at a time. PR is showing +7k changes with other changes suggested on top of constructs, e.g. a migration from a v1 physical name funct to v2

Would that really be much better than reviewing everything in this PR? It's almost all new code in new files. The constructs could be evaluated in the following order to understand dependencies:

  • src/common/helpers/custom-resource-provider-helper.ts
  • src/common/helpers/aoss-vector.ts
  • lambda/opensearch-serverless-custom-resources/
  • src/cdk-lib/bedrock/models.ts
  • src/cdk-lib/bedrock/custom-resources.ts
  • lambda/bedrock-custom-resources/
  • src/cdk-lib/bedrock/knowledge-base.ts
  • src/cdk-lib/bedrock/s3-data-source.ts
  • src/cdk-lib/bedrock/agent.ts

The only changed file is utils. I tried adapting generatePhysicalName in #196, but I found more incompatibilities. After talking to @krokoko yesterday, we determined that it would be best to create an alternative version. This change reverts #196, adds some exceptions to highlight the bug, and creates generatePhysicalNameV2.

@krokoko
Copy link
Collaborator

krokoko commented Jan 22, 2024

Thank you @jstrunk, awesome contribution ! I will review today

Copy link
Collaborator

@scottschreckengaust scottschreckengaust left a comment

Choose a reason for hiding this comment

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

Please describe why the *.ts.snap files are committed or remove.

@jstrunk
Copy link
Contributor Author

jstrunk commented Jan 22, 2024

Please describe why the *.ts.snap files are committed or remove.

They're included as a simple form of integration test to confirm that the different constructs work together. If we don't see value in using snapshot tests, then I don't have a problem removing them. We should also add that guidance to CONTRIBUTING to advise not to use snapshot testing.

@scottschreckengaust
Copy link
Collaborator

scottschreckengaust commented Jan 22, 2024

Please describe why the *.ts.snap files are committed or remove.

They're included as a simple form of integration test to confirm that the different constructs work together. If we don't see value in using snapshot tests, then I don't have a problem removing them. We should also add that guidance to CONTRIBUTING to advise not to use snapshot testing.

https://jestjs.io/docs/snapshot-testing

@krokoko
Copy link
Collaborator

krokoko commented Jan 23, 2024

Overall looks great, awesome contribution, thank you again @jstrunk !
Main item missing for merging: add documentation for the new exposed constructs. We can help with that if needed

Questions/thoughts:

  • would it make sense to expose the OpenSearch Serverless constructs ? They could be used in other constructs as well, or directly by users in their apps, it's useful !
  • I would move the publicly exposed constructs (Bedrock) to src/patterns/gen-ai to follow the existing folder structure. If the OpenSearch one is exposed as well, we can add it under src/patterns/general for instance as it is not gen ai specific
    Then, we will make it clearer in the catalog and main README about that the library offers in terms of constructs.

Copy link
Collaborator

@krokoko krokoko left a comment

Choose a reason for hiding this comment

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

  • move files as discussed in previous comment to match existing folder structure
  • add documentation

src/cdk-lib/bedrock/index.ts Show resolved Hide resolved
src/cdk-lib/bedrock/knowledge-base.ts Outdated Show resolved Hide resolved
@jstrunk
Copy link
Contributor Author

jstrunk commented Jan 23, 2024

  • I would move the publicly exposed constructs (Bedrock) to src/patterns/gen-ai to follow the existing folder structure. If the OpenSearch one is exposed as well, we can add it under src/patterns/general for instance as it is not gen ai specific

These aren't patterns. These are L2 constructs for Bedrock Agents and OpenSearch as a Vector store. Per @dineshSajwan in #169 (comment) , it makes since to create a new directory called cdk-lib. I could see these becoming a foundation for official L2 constructs once they no longer require custom resources.

I think it makes sense to put the OpenSearch ones under cdk-lib as well.

@jstrunk
Copy link
Contributor Author

jstrunk commented Jan 23, 2024

Per our call

  • Generate API markdown docs with JSDoc
  • Write overview documentation for Bedrock and AOSS Vectors
  • Move AOSS Vectors to src/cdk-lib
  • Reexport in top-level index.ts

Copy link

@massi-ang massi-ang left a comment

Choose a reason for hiding this comment

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

Great contribution.
Summary of my comments:

  • you have created few utilities functions that might have broader use than your constructs. Consider contributing them to the CDK
  • add the possibility to pass existing OS Index resource to the KnowledgeBase construct
  • us knowledgeBaseId instead of kbId. Same for ...Arn

src/cdk-lib/bedrock/custom-resources.ts Outdated Show resolved Hide resolved
src/cdk-lib/bedrock/knowledge-base.ts Show resolved Hide resolved
src/cdk-lib/bedrock/knowledge-base.ts Outdated Show resolved Hide resolved
src/cdk-lib/bedrock/knowledge-base.ts Outdated Show resolved Hide resolved
src/cdk-lib/bedrock/knowledge-base.ts Outdated Show resolved Hide resolved
src/common/helpers/custom-resource-provider-helper.ts Outdated Show resolved Hide resolved
src/common/helpers/utils.ts Outdated Show resolved Hide resolved
src/cdk-lib/bedrock/agent.ts Outdated Show resolved Hide resolved
src/cdk-lib/bedrock/agent.ts Outdated Show resolved Hide resolved
krokoko
krokoko previously approved these changes Jan 25, 2024
@krokoko krokoko enabled auto-merge (squash) January 26, 2024 17:10
auto-merge was automatically disabled January 26, 2024 19:56

Head branch was pushed to by a user without write access

@krokoko krokoko self-requested a review January 26, 2024 20:06
Copy link
Collaborator

@scottschreckengaust scottschreckengaust left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution. Small changes are easier to test, review, and approve. Please consider breaking up future PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(new construct): Implement Amazon Bedrock features as CDK constructs
6 participants