-
Notifications
You must be signed in to change notification settings - Fork 4k
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
(stepfunctions-tasks): BedrockInvokeModel cannot use JsonPath to specify input/output S3 URIs #29229
(stepfunctions-tasks): BedrockInvokeModel cannot use JsonPath to specify input/output S3 URIs #29229
Comments
Thank you for the detailed feedback. Making it a p1 and we welcome any pull requests. |
Thanks @pahud . Unfortunately I'm unlikely to be in a position to do this myself, because we're not going to end up using this integration. |
Hi @gm-al, Thank you for your interest to work on this issue. Please feel free to submit a PR. |
Hello, is there any update on this ? Thank you very much |
Hey @Celine999999 , |
Comments on closed issues and PRs are hard for our team to see. |
1 similar comment
Comments on closed issues and PRs are hard for our team to see. |
…ws#30298) ### Issue # (if applicable) Closes aws#29229. ### Reason for this change When trying to use JsonPath to specify the S3 URIs that BedrockInvokeModel will read from and write from, you get an error. Example of the Error message: `jsii.errors.JavaScriptError: Error: Field references must be the entire string, cannot concatenate them (found 's3://${Token[prompt_bucket.348]}/${Token[prompt_key.349]}')` ### Description of changes Extended the inputPath property to be allowed as an input value for the task state. Instead of adding a new S3Uri props in current `BedrockInvokeModelProps` as proposed in the original issue, leveraged the `inputPath` property that is already defined in `sfn.TaskStateBaseProps` and being extended by `BedrockInvokeModelInputProps` and `BedrockInvokeModelOutputProps` **Limitation:** We cannot limit the resource policy to specific input token for which the value might be coming from the prompt, so had to keep it as [*] here. ### Description of how you validated changes Added unit tests. Successful deployment of integration tests in the account. ### Checklist - [x] Unit Tests - [x] Integration Tests - [x] Updated ReadMe - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
#31305) ### Issue # (if applicable) Closes #31302. ### 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 - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
aws#31305) ### Issue # (if applicable) Closes aws#31302. ### 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 aws#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 - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
aws#31305) ### Issue # (if applicable) Closes aws#31302. ### 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 aws#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 - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
#31305) ### Issue # (if applicable) Closes #31302. ### 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 - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Describe the bug
If you try and use JsonPath to specify the S3 URIs that BedrockInvokeModel will read from and write from, you get an error.
Expected Behavior
I expect to be able to provide S3 URIs to BedrockInvokeModel using JsonPath, so I can have dynamic paths for different state machine executions.
Current Behavior
I get the error
Reproduction Steps
Possible Solution
The issue here is that the S3 locations are specified as an
aws_s3.Location
. This object expects a separatebucket_name
andobject_key
. However, BedrockInvokeModel expects an S3 URI. This mismatch is handled in the code by building an S3 URI from the S3 Locationaws-cdk/packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/bedrock/invoke-model.ts
Lines 208 to 213 in 83aa395
The problem is that field references cannot be concatenated in this way, hence the error I see.
This makes the S3 integration basically unusable from the CDK, because the S3 locations must be hardcoded, which cannot work for any real world workload.
My suggestion is to extend
BedrockInvokeModelInputProps
andBedrockInvokeModelOutputProps
to allow either the existings3Location
field to be used (for backwards compatibility), or astring
field calleds3Uri
. Ifs3Uri
were provided, it would be used directly, allowing for the use of JsonPaths for the URI.Additional Information/Context
No response
CDK CLI Version
2.129.0 (build d5ab0df)
Framework Version
2.129.0
Node.js Version
v18.17.1
OS
Ubuntu (Windows Subsystem for Linux)
Language
TypeScript, Python, .NET, Java, Go
Language Version
Python 3.11.6
Other information
No response
The text was updated successfully, but these errors were encountered: