feat(codebuild): add support for GPU build images #8879

skinny85 · 2020-07-02T20:56:07Z

CodeBuild has added support for running builds on machines with GPU drivers.
This required some changes to the protocol between IBuildImage and Project,
as those images are hosted in a public ECR repository that the image must grant the Project's Role access to.
Introduced a bind() method to IBuildImage -
to maintain backwards comaptibility,
made it optional.

Fixes #8408

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

rix0rrr

Conditionally approved -- one rename, and one reorganization that I'm leaving to you whether you want to carry it out (but I dearly request you do)

packages/@aws-cdk/aws-codebuild/lib/linux-gpu-build-image.ts

packages/@aws-cdk/aws-codebuild/lib/project.ts

packages/@aws-cdk/aws-codebuild/lib/linux-gpu-build-image.ts

rix0rrr · 2020-08-07T08:13:44Z

packages/@aws-cdk/aws-codebuild/lib/project.ts

@@ -1138,6 +1139,24 @@ export interface BuildEnvironment {
  readonly environmentVariables?: { [name: string]: BuildEnvironmentVariable };
 }

+/** Optional arguments to {@link IBuildImage.binder} - currently empty. */
+export interface BuildImageBindOptions {}


I think this is fine to not put in. Adding optional arguments is not a breaking change.

You cannot have optional arguments on interface declarations in TypeScript.

Are ya sure?

Is that a jsii restriction? Seems to work fine:

https://www.typescriptlang.org/play?#code/JYOwLgpgTgZghgYwgAgEJxBuyDeAoZQ5COMACjDgGcxgAbATwH4AuZAIwHtO6SQBKNgDdOwACYBuPAF8gA

You're right, apologies. I was thinking about default arguments, not optional arguments.

But is adding an optional argument later really a backwards-compatible change? If I have:

export interface IBindableBuildImage extends IBuildImage { bind(scope: Construct, project: IProject): BuildImageConfig; }

and I later change it to:

export interface IBindableBuildImage extends IBuildImage { bind(scope: Construct, project: IProject, options? BuildImageBindOptions): BuildImageConfig; }

Won't that break, for example, Java implementations of IBindableBuildImage ?

Hm. You might be right. Ask Romain, I guess :)

So this won't break because:

In Java we generate overload signatures by shaving the last optional argument until the last argument is no longer optional

In C# we'll generate a signature with a default value (of null) for this argument, which reproduces the JS behavior

In Python we'll generate a signature with a default value (of None) for this argument, which reproduces the JS behavior

Oh wait this is an interface... I'm not sure we generate default overrides on those (we could totally do it if we don't already).

EDIT: No we cannot make this non-breaking, because even if we introduce default overrides, existing code that implements only the override with fewer arguments won't implement the "new" form with one more parameter... #ouch

That needs to be a jsii-diff issue then :(

rix0rrr · 2020-08-07T08:15:21Z

packages/@aws-cdk/aws-codebuild/lib/project.ts

@@ -796,6 +794,9 @@ export class Project extends ProjectBase {
    if (props.encryptionKey) {
      this.encryptionKey = props.encryptionKey;
    }
+
+    // bind
+    this.buildImage.binder?.bind(this, this, {});


How about getting as close as possible to the final desired state?

If there is a bind, call it, and the bind should return all the relevant image properties. Otherwise use the properties from the current class.

That, instead of mixing the two as is happening right now.

I'm not sure I want to do that. That would mean a lot more complicated migration from 1.x to 2.x for users who have their own implementation of IBuildImage.

rix0rrr · 2020-08-07T08:19:41Z

...ages/@aws-cdk/aws-codebuild/test/integ.aws-deep-learning-container-build-image.expected.json

+    }
+  },
+  "Mappings": {
+    "AwsDeepLearningContainersRepositoriesAccounts": {


I think the better structure for this mapping is better to look something more like this:

Mappings: { RegionInfo: { us-east-1: { dlcRepositoryAccount: 763104351884, elbAccount: 12345768012, cloudFrontHostedZoneId: AIJ35819Z } }, }

etc. I'm not looking forward to having a different mapping for every key, especially when mappings are already 2-level by default.

Means that there should be some RegionInfo helper that progressively adds facts to a global map if a new fact is required in an agnostic region. I will be frank that I'm not 100% sure where that helper should live. Does @aws-cdk/core already depend on region-info? If so, that might be a good place to put it.

I'm actually not sure about this. It would mean that everyone who uses just Deep Learning Containers will also get all information about ELB accounts, CloudFront hosted zone IDs, and a lot of different stuff they probably have zero interest in. This will:

a) make their template a lot bigger, and
b) as they upgrade their version of the CDK, every change to ELB accounts, CloudFront hosted zone IDs, etc. will result in a needless change to their CodeBuild template.

Obviously I did not mean "just stuff all facts into a single static map that gets included into every template". Only the facts needed by the template should be in the map. To quote myself:

Means that there should be some RegionInfo helper that progressively adds facts to a global map if a new fact is required in an agnostic region.

On second thought, nevermind I guess. We will kick the can down the road and address this when we have 5 mappings.

When we changed the merging behavior in aws#8251, we forgot to account for the 'Rules' section. To prevent that error from happening again, let's default to merging objects without duplicates. Fixes aws#9485

skinny85 · 2020-08-08T02:53:05Z

@rix0rrr I've incorporated your comments, this is ready for another round of reviews.

rix0rrr

Double check the optional argument please.

mergify · 2020-08-12T17:43:20Z

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

mergify · 2020-08-12T18:05:30Z

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

aws-cdk-automation · 2020-08-12T18:31:59Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: a20cfd4
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-08-12T18:32:04Z

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

skinny85 requested a review from rix0rrr July 2, 2020 20:56

mergify bot added the contribution/core This is a PR that came from AWS. label Jul 2, 2020

skinny85 mentioned this pull request Jul 2, 2020

feat(codebuild): add support for images of type LINUX_GPU_CONTAINER #8441

Closed

rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Jul 7, 2020

rix0rrr approved these changes Jul 7, 2020

View reviewed changes

packages/@aws-cdk/aws-codebuild/lib/linux-gpu-build-image.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-codebuild/lib/project.ts Outdated Show resolved Hide resolved

SomayaB assigned skinny85 Jul 7, 2020

skinny85 force-pushed the feat/codebuild-gpu-images branch from 6dab0bc to ff864ee Compare August 7, 2020 01:11

skinny85 requested a review from rix0rrr August 7, 2020 01:12

rix0rrr requested changes Aug 7, 2020

View reviewed changes

skinny85 added 9 commits August 7, 2020 18:11

LinuxGpuBuildImage

68bc6e8

Implement an optional bind() in IBuildImage.

c1603ad

Add a CFN Mapping for DLC repository accounts in the different regions.

b262337

Extract a helper in ECR's Repository.

94b17a5

fix(core): don't fail for multiple CfnRules in your Stack

110021e

When we changed the merging behavior in aws#8251, we forgot to account for the 'Rules' section. To prevent that error from happening again, let's default to merging objects without duplicates. Fixes aws#9485

bind -> binder

e810d0b

Move the accounts to region-info.

bd8e728

Change to IBindableBuildImage.

5ed76f4

Introduce regionMap to RegionInfo.

0350376

skinny85 force-pushed the feat/codebuild-gpu-images branch from ff864ee to 0350376 Compare August 8, 2020 01:12

skinny85 requested a review from rix0rrr August 8, 2020 01:12

rix0rrr requested changes Aug 10, 2020

View reviewed changes

rix0rrr approved these changes Aug 12, 2020

View reviewed changes

skinny85 removed the pr/do-not-merge This PR should not be merged at this time. label Aug 12, 2020

skinny85 added the pr/do-not-merge This PR should not be merged at this time. label Aug 12, 2020

Remove inaccurate comment

39cefb5

skinny85 removed the pr/do-not-merge This PR should not be merged at this time. label Aug 12, 2020

Merge branch 'master' into feat/codebuild-gpu-images

a20cfd4

mergify bot merged commit b1b4cee into aws:master Aug 12, 2020

skinny85 deleted the feat/codebuild-gpu-images branch August 12, 2020 22:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(codebuild): add support for GPU build images #8879

feat(codebuild): add support for GPU build images #8879

skinny85 commented Jul 2, 2020

rix0rrr left a comment

rix0rrr Aug 7, 2020

skinny85 Aug 7, 2020

rix0rrr Aug 10, 2020 •

edited

Loading

rix0rrr Aug 10, 2020

skinny85 Aug 10, 2020 •

edited

Loading

rix0rrr Aug 11, 2020

RomainMuller Aug 12, 2020

RomainMuller Aug 12, 2020 •

edited

Loading

rix0rrr Aug 12, 2020

rix0rrr Aug 7, 2020

skinny85 Aug 7, 2020

rix0rrr Aug 7, 2020

skinny85 Aug 8, 2020

rix0rrr Aug 10, 2020

skinny85 commented Aug 8, 2020

rix0rrr left a comment

mergify bot commented Aug 12, 2020

mergify bot commented Aug 12, 2020

aws-cdk-automation commented Aug 12, 2020

mergify bot commented Aug 12, 2020

feat(codebuild): add support for GPU build images #8879

feat(codebuild): add support for GPU build images #8879

Conversation

skinny85 commented Jul 2, 2020

rix0rrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RomainMuller Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 commented Aug 8, 2020

rix0rrr left a comment

Choose a reason for hiding this comment

mergify bot commented Aug 12, 2020

mergify bot commented Aug 12, 2020

aws-cdk-automation commented Aug 12, 2020

AWS CodeBuild CI Report

mergify bot commented Aug 12, 2020

rix0rrr Aug 10, 2020 •

edited

Loading

skinny85 Aug 10, 2020 •

edited

Loading

RomainMuller Aug 12, 2020 •

edited

Loading