feat(stepfunctions-tasks): task constructs for creating and transforming SageMaker jobs #8391
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
packages/@aws-cdk/aws-stepfunctions-tasks/lib/sagemaker/base-types.ts
Outdated
Show resolved
Hide resolved
| private securityGroup?: ec2.ISecurityGroup; | ||
| private readonly securityGroups: ec2.ISecurityGroup[] = []; | ||
| private readonly subnets?: string[]; | ||
| private readonly integrationPattern: sfn.ServiceIntegrationPattern; | ||
| private readonly integrationPattern: sfn.IntegrationPattern; | ||
| private _role?: iam.IRole; | ||
| private _grantPrincipal?: iam.IPrincipal; | ||
|
|
||
| constructor(private readonly props: SagemakerTrainTaskProps) { | ||
| this.integrationPattern = props.integrationPattern || sfn.ServiceIntegrationPattern.FIRE_AND_FORGET; | ||
| constructor(scope: Construct, id: string, private readonly props: SageMakerCreateTrainingJobProps) { | ||
| super(scope, id, props); |
There was a problem hiding this comment.
Minor: I'd prefer that we either store props as a private member or a bunch of separate values like securityGroup, subnets, vpc, etc.
It's weird that we evaluate an intermediate value based on props in the constructor, store them as instance variables state while still storing the original props as an instance variable.
There was a problem hiding this comment.
I agree with your objection here - my preference is to store props as a private member. I'll work this feedback on a second pass. It does look strange to use both.
packages/@aws-cdk/aws-stepfunctions-tasks/lib/sagemaker/create-training-job.ts
Outdated
Show resolved
Hide resolved
| ...(spec.trainingImage ? { TrainingImage: spec.trainingImage.bind(this).imageUri } : {}), | ||
| ...(spec.algorithmName ? { AlgorithmName: spec.algorithmName } : {}), |
There was a problem hiding this comment.
In general I find this style hard to read. I'd rather create a local variable called trainingImage and then return it in this object.
Of course, this is everywhere in this file and that's not the intent of this PR, so I'll let you decide.
There was a problem hiding this comment.
I'd like to restructure this entire class. Noting your feedback and will take a second pass to do some cleanup after this PR.
| * Subscribe to demo Algorithm vended by Amazon (free): | ||
| * https://aws.amazon.com/marketplace/ai/procurement?productId=cc5186a0-b8d6-4750-a9bb-1dcdf10e787a | ||
| * FIXME - create Input data pertinent for the training model and insert into S3 location specified in inputDataConfig. |
There was a problem hiding this comment.
Is there a input data available somewhere that one can just use, like a sagemaker demo data or something?
Would be nice to provide a couple of CLI commands to quickly do this.
There was a problem hiding this comment.
spent a little bit of time, there's no turn-key usage available for training data... I'll iterate on this to improve on it. I think a simple set of training data is something we could do with a few CLI calls
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
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). |
|
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
replacement for the current implementation of
SageMakerserviceintegration and state level properties are merged and represented
as a construct.
The previous implementation that implemented
IStepFunctionsTaskhasbeen removed. The previously existing classes were directly converted to constructs
as these were marked experimental and still require further iterations
as they are not backed by a SageMaker L2 (does not exist yet).
In the interest of pragmatism, I decided to move them to leverage the newer pattern
so we can deprecate the
Taskconstruct.Note that I have left the unit and integration tests verbatim. The integration test
requires some additional steps as there are pre-requisites to running a training job
such as creating and configuring input data that are not currently included.
BREAKING CHANGE: constructs for
SageMakerCreateTrainingJobandSageMakerCreateTransformJobreplace previous implementation thatimplemented
IStepFunctionsTask.volumeSizeInGBproperty inResourceConfigforSageMaker tasks are now type
core.SizemaxPayloadproperty inSagemakerTransformPropsis now type
core.SizevolumeKmsKeyIdproperty inSageMakerCreateTrainingJobis nowvolumeEncryptionKeyBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license