fix(stepfunctions-tasks): fix bedrock input/output path in step-funct… by shikha372 · Pull Request #31305 · aws/aws-cdk

shikha372 · 2024-09-03T19:45:08Z

Issue # (if applicable)

Reason for this change

PR#30298 introduced a breaking change in the stepfunctions-tasks which was encoding the input/output path from the TaskStateBaseProps to be used to input a S3 URI, which is not always the case and customer might be using the JSON string as it is.

Description of changes

To keep the functionality from the original ask of issue #29229 ,added another props to specify S3 URI as an input to the stepfunctions-tasks.

Introduced a feature flag to keep the existing behaviour for the customers and not introduce a breaking change.

Description of how you validated changes

Updated integ test and deployed in account

Integ Test Results:
aws stepfunctions start-execution --state-machine-arn <deployed state machine arn> : should return execution arn

A5-zEnXPmmPWTJx
{
"executionArn": "arn:aws:states:us-east-1:XXXX:execution:StateMachine2E01A3A5-zEnXPmmPWTJx:516f2e60-9507-46cb-95e6-4d9453429b08",
"startDate": "2024-09-04T15:43:33.200000-07:00"
}

aws stepfunctions describe-execution --execution-arn <exection-arn generated before> : should return status as SUCCEEDED

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

…ions tasks

godwingrs22

Overall looks good. Just left few minor comments

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/bedrock/invoke-model.ts

…ions tasks

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/bedrock/invoke-model.ts

clareliguori · 2024-09-03T20:46:28Z

packages/aws-cdk-lib/aws-stepfunctions-tasks/test/bedrock/invoke-model.test.ts

-      InputPath: '$.prompt',
-      OutputPath: '$.prompt',


Are there other unit tests that test this functionality?

There are two unit tests, one to check the expected template and one to check the permissions populated in policy

The title of this unit test is "invoke model allows input and output json path". I think this test was originally meant to test the original behavior of inputPath and outputPath. So we need an additional new test for these new s3InputUri and s3OutputUri params, AND fix this unit test to go back to testing the original behavior of inputPath and outputPath.

Update: I see in the history now that you had actually added this test in the last change. I do still think we should have both tests (inputPath and s3InputUri)

Thanks @clareliguori , this leads to one question can body be skipped in case of an inputPath ? since the earlier validation seems to added only for body and input and didn't point to a definition where we might be using these base props.

Oh I see, yes you still need body. With inputPath, you would do something like body=sfn.JsonPath.object_at("$")

Yeah seems like I totally missed adding unit tests for inputPath and outputPath

godwingrs22 · 2024-09-03T20:52:48Z

...-cdk-testing/framework-integ/test/aws-stepfunctions-tasks/test/bedrock/integ.invoke-model.ts

+  body: sfn.TaskInput.fromObject(
+    {
+      inputText: sfn.JsonPath.format(
+        'Alphabetize this list of first names:\n{}',
+        sfn.JsonPath.stringAt('$.names'),
+      ),
+      textGenerationConfig: {
+        maxTokenCount: 100,
+        temperature: 1,
+      },
+    },
+  ),
  outputPath: sfn.JsonPath.stringAt('$.names'),


Can help me to understand why body is required instead of inputPath?

just added this integ test is to make sure, we are not breaking any existing use case for outputPath or inputPath from the base class, looking back at why it might have been marked as required could be a decision taken while designing this s3 input parameter, i don't see it something as required in API doc

In order to test the inputPath field, you can do a Pass state to transform the previous output into the format expected for the InvokeModel body.

clareliguori · 2024-09-03T22:00:17Z

...-cdk-testing/framework-integ/test/aws-stepfunctions-tasks/test/bedrock/integ.invoke-model.ts

+/** Test for Bedrock s3 URI Path */
+const prompt4 = new BedrockInvokeModel(stack, 'Prompt4', {
+  model,
+  s3InputUri: sfn.JsonPath.stringAt('$.names'),
+  s3OutputUri: sfn.JsonPath.stringAt('$.names'),
+});
+


Can you run the verification steps listed at the top of this class?
Stack verification steps

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/bedrock/invoke-model.ts

packages/aws-cdk-lib/aws-stepfunctions-tasks/README.md

moelasmar · 2024-09-05T16:24:30Z

...-cdk-testing/framework-integ/test/aws-stepfunctions-tasks/test/bedrock/integ.invoke-model.ts

+  postCliContext: {
+    '@aws-cdk/aws-cdk.aws-stepfunctions-tasks:useNewS3UriParametersForBedrockInvokeModelTask': true,
+  },


if the default value of the flag is True, so we do not need to set this flag, and instead you need to update the old integration test cases to set the flag to false to test the current behaviour (using Input path, and output path instead of the new S3 uris)

shikha372 · 2024-09-05T21:19:15Z

@Mergifyio update

mergify · 2024-09-05T21:19:23Z

update

✅ Branch has been successfully updated

aws-cdk-automation · 2024-09-05T21:50:42Z

AWS CodeBuild CI Report

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

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

mergify · 2024-09-05T21:51:09Z

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).

github-actions · 2024-09-05T21:51:34Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

fix(stepfunctions-tasks): fix bedrock input/output path in step-funct…

d504493

…ions tasks

github-actions bot added bug This issue is a bug. p0 labels Sep 3, 2024

aws-cdk-automation requested a review from a team September 3, 2024 19:45

mergify bot added the contribution/core This is a PR that came from AWS. label Sep 3, 2024

godwingrs22 reviewed Sep 3, 2024

View reviewed changes

fix(stepfunctions-tasks): fix bedrock input/output path in step-funct…

8f0d77f

…ions tasks

shikha372 marked this pull request as ready for review September 3, 2024 20:28

shikha372 added the pr/do-not-merge This PR should not be merged at this time. label Sep 3, 2024

godwingrs22 reviewed Sep 3, 2024

View reviewed changes

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/bedrock/invoke-model.ts Outdated Show resolved Hide resolved

update readme

7682fa2

clareliguori reviewed Sep 3, 2024

View reviewed changes

godwingrs22 reviewed Sep 3, 2024

View reviewed changes

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 3, 2024

clareliguori reviewed Sep 3, 2024

View reviewed changes

move input and output s3 uri to bedrock invoke model props

6c6ac5f

shikha372 force-pushed the bedrock_input_path branch 13 times, most recently from df27bb4 to 9a4b791 Compare September 4, 2024 20:20

adding feature flag

070af9e

moelasmar reviewed Sep 5, 2024

View reviewed changes

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/bedrock/invoke-model.ts Outdated Show resolved Hide resolved

update feature flag

47d60bd

shikha372 force-pushed the bedrock_input_path branch from 79dec88 to 47d60bd Compare September 5, 2024 10:07

moelasmar reviewed Sep 5, 2024

View reviewed changes

packages/aws-cdk-lib/aws-stepfunctions-tasks/README.md Outdated Show resolved Hide resolved

fix small nits

7589bd4

moelasmar reviewed Sep 5, 2024

View reviewed changes

packages/aws-cdk-lib/aws-stepfunctions-tasks/README.md Show resolved Hide resolved

fix small nits

38b80bd

moelasmar reviewed Sep 5, 2024

View reviewed changes

shikha372 added 3 commits September 5, 2024 09:26

fix README

211e23f

set feature flag default to true

af747c8

fix integ

a52bdcf

shikha372 force-pushed the bedrock_input_path branch from aa8897c to a52bdcf Compare September 5, 2024 17:03

shikha372 added 2 commits September 5, 2024 10:24

fix nit

451b442

fix last nit hopefully

0b7d4e8

moelasmar approved these changes Sep 5, 2024

View reviewed changes

shikha372 added 4 commits September 5, 2024 11:01

Update features.ts

4415b89

set flag type to api default

2ce70ff

fix nit

dfa2739

final fix for successful build

0a7c25b

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 5, 2024

Merge branch 'main' into bedrock_input_path

523c05d

shikha372 removed the pr/do-not-merge This PR should not be merged at this time. label Sep 5, 2024

mergify bot merged commit a190935 into main Sep 5, 2024

mergify bot deleted the bedrock_input_path branch September 5, 2024 21:51

github-actions bot locked as resolved and limited conversation to collaborators Sep 5, 2024

Conversation

shikha372 commented Sep 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

Uh oh!

godwingrs22 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shikha372 Sep 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shikha372 commented Sep 5, 2024

Uh oh!

mergify bot commented Sep 5, 2024

✅ Branch has been successfully updated

Uh oh!

aws-cdk-automation commented Sep 5, 2024

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Sep 5, 2024

Uh oh!

github-actions bot commented Sep 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

shikha372 commented Sep 3, 2024 •

edited

Loading

shikha372 Sep 3, 2024 •

edited

Loading