Skip to content
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

add sm custom kernel module #106

Closed
wants to merge 6 commits into from

Conversation

kukushking
Copy link
Contributor

Issue #, if available:
N/A

Description of changes:
Added SageMaker custom kernel module to IDF.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Signed-off-by: Anton Kukushkin <[email protected]>
Copy link
Contributor

@dgraeber dgraeber left a comment

Choose a reason for hiding this comment

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

Minor comments...I need to dig further into the logic...more to come, but see my comments?

- "sagemaker:DescribeDomain"
- "sagemaker:AddTags"
- "iam:PassRole"
Resource: "*"
Copy link
Contributor

Choose a reason for hiding this comment

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

too wide open....lets discuss

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks good catch, especially pass role, will scope down

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Scoped down iam:PassRole.

One interesting caveat regarding ECR deployment IAM permissions - ECRDeployment construct always adds a policy that contains wildcard permissions, even if you provide it with a custom role. Filed an issue for that.

modules/ml/sagemaker-custom-kernel/app.py Show resolved Hide resolved
Signed-off-by: Anton Kukushkin <[email protected]>
cache_image=$IMAGE_URI
docker pull ${cache_image} 2>&1 > /dev/null || true
cd docker/$SEEDFARMER_PARAMETER_SAGEMAKER_IMAGE_NAME && docker build --progress plain --cache-from=${cache_image} -t $IMAGE_URI .
docker push $IMAGE_URI
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of going with the traditional way of building docker images via CLI, you could use CDK way of building docker images. Ref: https://github.com/srinivasreddych/demo-with-mps3-reinvent23/blob/main/modules/pre-processing/image-extraction/stack.py#L68

This will look for a dockerfile, build and push it to ECR and you can avoid the build script and build commands in the deployspec.yaml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, nice, didn't know about this. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

oooohhhh....this is nice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored to use this. Thanks Srini!

return sm_client.describe_image_version(ImageName=image_name)


def check_image_version_exists():
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not an SM expert, but is there a CDK way of doing this, like using an L3 construct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is an L1 construct for image version, but I will need to check whether it does everything we need it to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored to use CDK constructs where possible - Image, ImageVersion, and AppImageConfig - check the latest commit please. Dealing with L1 is a bit annoying but arguably still easier than maintaining a custom script.

@kukushking kukushking marked this pull request as draft January 4, 2024 19:09
@malachi-constant
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: IDFModuleTestsInfrastructur-59JoFoVH64CI
  • Commit ID: c29a15c
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@malachi-constant
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: IDFModuleTestsInfrastructur-59JoFoVH64CI
  • Commit ID: 5e01a7f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@malachi-constant
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: IDFModuleTestsInfrastructur-59JoFoVH64CI
  • Commit ID: 9221e90
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@kukushking kukushking marked this pull request as ready for review February 19, 2024 17:50
@kukushking
Copy link
Contributor Author

@dgraeber @srinivasreddych should I reopen this against https://github.com/awslabs/mlops-modules?

@kukushking
Copy link
Contributor Author

Closed as this is moving to https://github.com/awslabs/mlops-modules

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants