Skip to content

Conversation

@rantoniuk
Copy link
Contributor

@rantoniuk rantoniuk commented Jul 27, 2025

Follow-up of #34618 as it did not fully fix the warnings.

Change

Stop passing the deprecated property to Cfn object.

Tests

Tested on minimal stack and an actual production stack.

Fixes #34895

Copilot AI review requested due to automatic review settings July 27, 2025 19:18
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes lazy loading for the deprecated inferenceAccelerators property in ECS task definitions to eliminate warnings. The change simplifies the code by directly evaluating the inference accelerators condition instead of wrapping it in a lazy loader.

  • Replaces lazy-loaded inferenceAccelerators with direct evaluation
  • Maintains the same conditional logic for Fargate compatibility
  • Addresses deprecation warnings from the lazy loading mechanism

@github-actions github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Jul 27, 2025
@aws-cdk-automation aws-cdk-automation requested a review from a team July 27, 2025 19:18
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 fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.

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.

✅ A exemption request has been requested. Please wait for a maintainer's review.

@rantoniuk
Copy link
Contributor Author

Exemption Request - as previously, I don't think this needs tests.

@aws-cdk-automation aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Jul 27, 2025
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 594ce5e
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@rantoniuk rantoniuk marked this pull request as draft July 28, 2025 08:23
@rantoniuk rantoniuk force-pushed the fix/inferenceAccellerators-warning-fix branch from 594ce5e to bf6ef59 Compare August 14, 2025 09:06
@github-actions github-actions bot added p1 and removed p2 labels Aug 14, 2025
@rantoniuk rantoniuk changed the title fix: remove lazy loading for deprecated inferenceAccelerators property fix: stop passing the deprecated property to Cfn construct Aug 14, 2025
@rantoniuk rantoniuk marked this pull request as ready for review August 14, 2025 09:22
@rantoniuk
Copy link
Contributor Author

@pahud any chance you can help finding a reviewer?

@rantoniuk rantoniuk force-pushed the fix/inferenceAccellerators-warning-fix branch from bf6ef59 to baaeea7 Compare August 20, 2025 10:04
@Abogical Abogical self-assigned this Aug 20, 2025
Copy link
Member

@Abogical Abogical left a comment

Choose a reason for hiding this comment

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

I believe a unit test (at packages/aws-cdk-lib/aws-ecs/test/task-definition.test.ts) is needed here just to check that the inferenceAccelerator is not added by default and that it is only added iff an inferenceAccelerator is passed.

@rantoniuk
Copy link
Contributor Author

I believe a unit test (at packages/aws-cdk-lib/aws-ecs/test/task-definition.test.ts) is needed here just to check that the inferenceAccelerator is not added by default and that it is only added iff an inferenceAccelerator is passed.

This is already covered by unit tests.

@rantoniuk rantoniuk requested a review from Abogical August 21, 2025 09:14
@Abogical
Copy link
Member

Abogical commented Aug 21, 2025

I believe a unit test (at packages/aws-cdk-lib/aws-ecs/test/task-definition.test.ts) is needed here just to check that the inferenceAccelerator is not added by default and that it is only added iff an inferenceAccelerator is passed.

This is already covered by unit tests.

Is there a unit test to test that inferenceAccelerator is not passed if not provided? (by using Match.absent for example)

Copy link
Member

@Abogical Abogical left a comment

Choose a reason for hiding this comment

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

Per my comment above. A unit test to test that inferenceAccelerator is not passed is needed here. You can use Match.absent to achieve this.

@rantoniuk rantoniuk force-pushed the fix/inferenceAccellerators-warning-fix branch from c96a66f to 20b682f Compare August 21, 2025 12:05
@rantoniuk rantoniuk force-pushed the fix/inferenceAccellerators-warning-fix branch from 20b682f to edc8633 Compare August 21, 2025 12:06
@rantoniuk rantoniuk requested a review from Abogical August 21, 2025 12:22
Copy link
Member

@Abogical Abogical left a comment

Choose a reason for hiding this comment

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

Integ test snapshots would need to be updated:

@aws-cdk-testing/framework-integ:   CHANGED    aws-dynamodb/test/integ.global-replicas-provisioned 2.532s
@aws-cdk-testing/framework-integ:       Resources
@aws-cdk-testing/framework-integ:       [~] AWS::CloudFormation::Stack awscdkawsdynamodbReplicaProviderNestedStackawscdkawsdynamodbReplicaProviderNestedStackResource18E3F12D
@aws-cdk-testing/framework-integ:        └─ [~] TemplateURL
@aws-cdk-testing/framework-integ:            └─ [~] .Fn::Join:
@aws-cdk-testing/framework-integ:                └─ @@ -13,6 +13,6 @@
@aws-cdk-testing/framework-integ:                   [ ]     {
@aws-cdk-testing/framework-integ:                   [ ]       "Fn::Sub": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}"
@aws-cdk-testing/framework-integ:                   [ ]     },
@aws-cdk-testing/framework-integ:                   [-]     "/be82eb3f30fbf19ad6227c31590fdcc98fdc3f1968a085e41f52ff22aaa2fff6.json"
@aws-cdk-testing/framework-integ:                   [+]     "/d80a42298fecb9cef8ab0548bbddde09f369cc7d09c1c6fabbae147532f77556.json"
@aws-cdk-testing/framework-integ:                   [ ]   ]
@aws-cdk-testing/framework-integ:                   [ ] ]
@aws-cdk-testing/framework-integ:       
@aws-cdk-testing/framework-integ:       
@aws-cdk-testing/framework-integ:   CHANGED    aws-dynamodb/test/integ.global-replicas-provisioned 2.532s
@aws-cdk-testing/framework-integ:       Resources
@aws-cdk-testing/framework-integ:       [~] AWS::Lambda::Function OnEventHandler42BEBAE0
@aws-cdk-testing/framework-integ:        └─ [~] Code
@aws-cdk-testing/framework-integ:            └─ [~] .S3Key:
@aws-cdk-testing/framework-integ:                ├─ [-] 7e256f5e4a92713cc232cd23819cb666756ee6023ce380130abac3b6a3fcea6a.zip
@aws-cdk-testing/framework-integ:                └─ [+] 7ecc77636deefb31954ea2e6aadf6d2c361a2943535a678d65f46a8de5e1f503.zip
@aws-cdk-testing/framework-integ:       [~] AWS::Lambda::Function IsCompleteHandler7073F4DA
@aws-cdk-testing/framework-integ:        └─ [~] Code
@aws-cdk-testing/framework-integ:            └─ [~] .S3Key:
@aws-cdk-testing/framework-integ:                ├─ [-] 7e256f5e4a92713cc232cd23819cb666756ee6023ce380130abac3b6a3fcea6a.zip
@aws-cdk-testing/framework-integ:                └─ [+] 7ecc77636deefb31954ea2e6aadf6d2c361a2943535a678d65f46a8de5e1f503.zip
@aws-cdk-testing/framework-integ:       
@aws-cdk-testing/framework-integ:       
@aws-cdk-testing/framework-integ:   CHANGED    aws-dynamodb/test/integ.global 2.561s
@aws-cdk-testing/framework-integ:       Resources
@aws-cdk-testing/framework-integ:       [~] AWS::CloudFormation::Stack awscdkawsdynamodbReplicaProviderNestedStackawscdkawsdynamodbReplicaProviderNestedStackResource18E3F12D
@aws-cdk-testing/framework-integ:        └─ [~] TemplateURL
@aws-cdk-testing/framework-integ:            └─ [~] .Fn::Join:
@aws-cdk-testing/framework-integ:                └─ @@ -9,6 +9,6 @@
@aws-cdk-testing/framework-integ:                   [ ]     {
@aws-cdk-testing/framework-integ:                   [ ]       "Fn::Sub": "cdk-hnb659fds-assets-${AWS::AccountId}-eu-west-1"
@aws-cdk-testing/framework-integ:                   [ ]     },
@aws-cdk-testing/framework-integ:                   [-]     "/2d2187472236ea907da93f0398d9237e06c266540258f4ed7ac24cd4ef9dcabb.json"
@aws-cdk-testing/framework-integ:                   [+]     "/85cd6b466d0c362b83cedb76074823dd89cccb47cbf551109abb90e8406bcd3d.json"
@aws-cdk-testing/framework-integ:                   [ ]   ]
@aws-cdk-testing/framework-integ:                   [ ] ]
@aws-cdk-testing/framework-integ:       
@aws-cdk-testing/framework-integ:       
@aws-cdk-testing/framework-integ:   CHANGED    aws-dynamodb/test/integ.global 2.561s
@aws-cdk-testing/framework-integ:       Resources
@aws-cdk-testing/framework-integ:       [~] AWS::Lambda::Function OnEventHandler42BEBAE0
@aws-cdk-testing/framework-integ:        └─ [~] Code
@aws-cdk-testing/framework-integ:            └─ [~] .S3Key:
@aws-cdk-testing/framework-integ:                ├─ [-] 7e256f5e4a92713cc232cd23819cb666756ee6023ce380130abac3b6a3fcea6a.zip
@aws-cdk-testing/framework-integ:                └─ [+] 7ecc77636deefb31954ea2e6aadf6d2c361a2943535a678d65f46a8de5e1f503.zip
@aws-cdk-testing/framework-integ:       [~] AWS::Lambda::Function IsCompleteHandler7073F4DA
@aws-cdk-testing/framework-integ:        └─ [~] Code
@aws-cdk-testing/framework-integ:            └─ [~] .S3Key:
@aws-cdk-testing/framework-integ:                ├─ [-] 7e256f5e4a92713cc232cd23819cb666756ee6023ce380130abac3b6a3fcea6a.zip
@aws-cdk-testing/framework-integ:                └─ [+] 7ecc77636deefb31954ea2e6aadf6d2c361a2943535a678d65f46a8de5e1f503.zip

@rantoniuk
Copy link
Contributor Author

I don't think that's anyhow related to my change.

@rantoniuk rantoniuk requested a review from Abogical August 21, 2025 12:42
Copy link
Member

@Abogical Abogical 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!

@Abogical Abogical dismissed aws-cdk-automation’s stale review August 21, 2025 13:49

This comes from an old build workflow.

@mergify
Copy link
Contributor

mergify bot commented Aug 21, 2025

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

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.

(This review is outdated)

@mergify
Copy link
Contributor

mergify bot commented Aug 21, 2025

This pull request has been removed from the queue for the following reason: pull request dequeued.

Pull request #35080 has been dequeued. The pull request rule doesn't match anymore. The following conditions don't match anymore:

  • #changes-requested-reviews-by=0
  • check-success=build
  • check-success=validate-pr
  • any of: [🔀 queue conditions]
    • all of: [📌 queue conditions of queue default-squash]
      • #changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
      • any of: [🛡 GitHub branch protection]
        • check-neutral = validate-pr
        • check-skipped = validate-pr
        • check-success = validate-pr.

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.
If you do update this pull request, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

@Abogical Abogical added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Aug 21, 2025
@aws-cdk-automation aws-cdk-automation dismissed their stale review August 21, 2025 14:01

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

@mergify
Copy link
Contributor

mergify bot commented Aug 21, 2025

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 a20afcd into aws:main Aug 21, 2025
27 checks passed
@github-actions
Copy link
Contributor

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK bug This issue is a bug. effort/medium Medium work item – several days of effort p1 pr-linter/exempt-integ-test The PR linter will not require integ test changes pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

aws-ecs: aws-cdk-lib.aws_ecs.CfnTaskDefinitionProps#inferenceAccelerators deprecation warning

3 participants