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(stepfunctions-tasks): support for the Step Functions optimized integration for Bedrock InvokeModel API #28276

Merged
merged 8 commits into from
Dec 11, 2023

Conversation

clareliguori
Copy link
Member

Step Functions recently released an optimized integration for Bedrock InvokeModel API, and these changes add support for adding the Bedrock InvokeModel task to Step Functions state machines.

Closes #28268.

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

@github-actions github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 admired-contributor [Pilot] contributed between 13-24 PRs to the CDK labels Dec 6, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team December 6, 2023 20:05
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@clareliguori
Copy link
Member Author

My account does not have Bedrock model access in us-east-1, so the integ test does not validate that the Step Functions state machine can successfully execute. I deployed the stack in my account in us-west-2 where I have model access, and manually executed the state machine successfully and returned an expected result:

{
  "names": "\nHere is the alphabetized list of first names:\n\n1. Elijah\n2. John\n3. Liam\n4. Oliver\n5. Noah"
}

@clareliguori clareliguori changed the title feat(aws-stepfunctions-tasks): support for the Step Functions optimized integration for Bedrock InvokeModel API feat(stepfunctions-tasks): support for the Step Functions optimized integration for Bedrock InvokeModel API Dec 6, 2023
@aws-cdk-automation aws-cdk-automation dismissed their stale review December 6, 2023 20:09

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@clareliguori clareliguori force-pushed the step-functions-bedrock-invoke-model branch from 466a6f2 to a0924a6 Compare December 6, 2023 21:13
@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 6, 2023
@scanlonp
Copy link
Contributor

scanlonp commented Dec 6, 2023

Hey @clareliguori, this looks terrific! Before I dive into a full review, I had a couple of higher level questions. I see you created a aws-cdk-lib/aws-bedrock folder, but that it does not correspond to any cloudformation resources. You explain that in the readme, but I am concerned this could be confusing for users when they see an import from a bedrock module. Are there use cases for the bedrock models outside of stepfunctions? Or is there a reason they should not be contained in the stepfunctions module? Are there other cases in stepfunctions that have had a similar pattern?

I will also ask the team about if we have dealt with patterns similar to this, and how we approach them.

Also a quick note about the bedrock output. Did you notice Noah and Oliver are out of alphabetical order! That is pretty funny 🙃

@clareliguori
Copy link
Member Author

Hey @clareliguori, this looks terrific! Before I dive into a full review, I had a couple of higher level questions. I see you created a aws-cdk-lib/aws-bedrock folder, but that it does not correspond to any cloudformation resources. You explain that in the readme, but I am concerned this could be confusing for users when they see an import from a bedrock module. Are there use cases for the bedrock models outside of stepfunctions? Or is there a reason they should not be contained in the stepfunctions module? Are there other cases in stepfunctions that have had a similar pattern?

I will also ask the team about if we have dealt with patterns similar to this, and how we approach them.

Yeah I went back and forth on that, and I'm happy to change it

Pros of being in the aws-bedrock module:

  • These import-only constructs can be expanded to add IAM permission-type methods, like grantInvokeModel, in order to grant permissions to invoke the models to other constructs like ECS services, Lambda functions, etc.
  • If CloudFormation does eventually support a resource like Bedrock ProvisionedThroughput (I don't have any insight on if they will or not), then it would be easier to expand on the existing class for the L2 construct vs having some duplicate and/or deprecated logic between the aws-stepfunctions-tasks and aws-bedrock modules.

Cons of being in the aws-bedrock module:

  • Since we don't know what any potential future Bedrock resource type would look like, I'm kind of guessing at the right shape/naming of the L2 construct with these initial classes. We could end up deprecating one or more of the classes anyway if I got it wrong.

Note that I don't expect a base foundational model would ever be a CFN resource (since they don't belong to an account), so that would likely always be an import-only construct as it is in this PR.

Also a quick note about the bedrock output. Did you notice Noah and Oliver are out of alphabetical order! That is pretty funny 🙃

LOL I did not notice that

Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

Hi @clareliguori, thanks for the PR! This is an interesting one, and my feelings are that, while unusual, your aws-bedrock pre-CFN implementation provides some benefit in creating the model arn, and is relatively low-risk in predicting what a bedrock L2 would look like in the future.

Know that our normal process for creating new L2s involves an RFC, experimental module first, and much more bureaucracy. But I think common sense should prevail here as this is simple enough.

Sending this off to @rix0rrr to glance at just to make sure there's not something obvious i'm missing about creating the aws-bedrock submodule without cfn. I doubt that will cause any issues, although when bedrock gets CFN resources we'll have to rely on our L1 generation to be resilient enough to play nice with the existing folder.

packages/aws-cdk-lib/aws-bedrock/lib/foundation-model.ts Outdated Show resolved Hide resolved
packages/aws-cdk-lib/aws-bedrock/lib/foundation-model.ts Outdated Show resolved Hide resolved
*
* @see https://docs.aws.amazon.com/bedrock/latest/userguide/api-methods-run.html
*/
readonly model: bedrock.IModel;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. The alternative here is to just allow a modelArn in the absence of bedrock CFN constructs. That seems like the simplest path forward, and we can always add in model: bedrock.IModel later when CFN supports it.

The current benefit of your aws-bedrock implementation is that there are some helper functions to achieving the arn. The question is, how will users of this task feel about that? Would they prefer to just copy/paste the model arn that they have created elsewhere?

The other interesting thing is that per your implementation, it looks like Foundation Models have well known arns based on their model ID. Perhaps that is enough benefit to move forward with what you've got.

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 think the most common case will be using foundation models, so having that ARN constructed for you is pretty useful

@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 7, 2023
Co-authored-by: Kaizen Conroy <[email protected]>
@mergify mergify bot dismissed kaizencc’s stale review December 7, 2023 21:31

Pull request has been modified.

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 7, 2023
kaizencc
kaizencc previously approved these changes Dec 11, 2023
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

Looks good! From my understanding of bedrock, it looks like at least the foundation model will never be a CFN resource since we are referencing something that exists rather than creating something new. So the bedrock pseudo-construct is actually the best we can do and will be useful if there are other places we want to send in a bedrock arn. Thanks @clareliguori

@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 11, 2023
Copy link
Contributor

mergify bot commented Dec 11, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot dismissed kaizencc’s stale review December 11, 2023 16:30

Pull request has been modified.

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 11, 2023
@clareliguori
Copy link
Member Author

@kaizencc I need your approval again - I ran into this issue with Mergify:
Mergifyio/mergify#5055

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: ab1d24b
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Copy link
Contributor

mergify bot commented Dec 11, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit f3dafa4 into aws:main Dec 11, 2023
10 checks passed
@clareliguori clareliguori deleted the step-functions-bedrock-invoke-model branch December 11, 2023 21:03
@linkcd
Copy link

linkcd commented Dec 12, 2023

Thanks for this PR! Sorry for a basic question, but how do i check which CDK version will contain this RP?

@clareliguori
Copy link
Member Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
admired-contributor [Pilot] contributed between 13-24 PRs to the CDK effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws-stepfunctions-tasks: Support for integration with Bedrock InvokeModel
5 participants