feat(stepfunctions-tasks): add support for SageMakerCreateTransformJob to take instance type as a parameter

[PPPW]

Currently to create a SageMaker Batch Transform job, we can't specify the instance type as a parameter (e.g., 'InstanceType.$': '$.InstanceType' in CFN template), because the TransformResources.instanceType is of type ec2.InstanceType, and we will add a "ml." prefix to this EC2 instance type as the final instance type. We'd like to build step functions with SageMaker Batch Transform state and be able to support different instance types from user input.

With this change, user can pass instance type as a parameter, with values such as "ml.p2.xlarge".

---

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

[gitpod-io]

[shivlaks]

Typescript union types will not be compatible with other languages.
This will need to be represented by a single type. The way we do it in the cdk is to create a union like class with APIs like fromInstanceType and fromString that return that type.

[PPPW]

I see, I can define a instanceType class here to use it as the type of this field, something like this:

export class InstanceType {
  public static fromEc2InstanceType(instanceType: ec2.InstanceType) {
    return new InstanceType(`ml.${instanceType}`);
  }

  public static fromString(instanceType: string) {
    return new InstanceType(instanceType);
  }

  constructor(private readonly instanceType: string) {
  }

  public toString(): string {
    return this.instanceType;
  }
}

But it won't be backward compatible, e.g. current users need to change their code: instanceType: ec2.InstanceType.of(ec2.InstanceClass.P3, ec2.InstanceSize.XLARGE2). Should we make it backward compatible somehow, or it's fine to do this?

[PPPW]

Hi @shivlaks, could you take a quick look at the above comments when you get a chance? Thanks!

[PPPW]

Hi @shivlaks, looks like yarn compat is not happy with this change, because it changed the API. So should I make the InstanceType class defined in above to extend ec2. InstanceType?

Pull request has been modified.

[PPPW]

Dear @shivlaks, when you get a chance, could let me know if my fix looks fine? The instance type part is not backward compatible, I appreciate if you could provide some suggestions. Thank you for your time!

[shivlaks]

@PPPW this looks good to me! a couple of minor things to take care of before we can merge. Let me know if you have any questions!

[gitpod-io]

Pull request has been modified.

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
• Commit ID: b3731c4
• Result: FAILED
• Build Logs (available for 30 days)

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

[shivlaks]

pending resolution re: breaking change from modifying type of instanceType from ec2.InstanceType to one that is defined locally within the module.

[shivlaks]

I know we removed the @experimental annotations after you had initially published this PR, but I believe this is now a breaking change.

Let me check with a team member or two to see whether we would need to:

1. support the old parameter but mark it as @deprecated
2. make this change as it is, but call it out as a BREAKING change.

It is a breaking change if we ship it as it is as clients who upgrade will see breakage.

[PPPW]

Right, in earlier comments I was checking whether we should make it backward compatible. Let me know if we have to extend ec2. InstanceType here, thanks

[PPPW]

Hi @shivlaks, any updates on this?

[BenChaimberg]

As Shiv mentions above, we will need to create a new property (maybe mlInstanceType) and mark the old property as deprecated. Unfortunately we cannot extend this type with a type union due to jsii restrictions

[BenChaimberg]

This will probably be superseded by #15726 as a simpler solution

[PPPW]

This will probably be superseded by #15726 as a simpler solution

Thanks @BenChaimberg, that looks good. Feel free to borrow my README and unit tests (or I can contribute if we'd like to).