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

Sagemaker Endpoint Resource #1034

Merged
merged 5 commits into from
Dec 26, 2023
Merged

Conversation

blakeromano
Copy link
Contributor

@blakeromano blakeromano commented Dec 19, 2023

Description of your changes

Adds Sagemaker Endpoint resource.

Fixes #1029

I have:

  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

This requires an image that can be created/deleted only from the AWS CLI. In this case I use a public image from ECR of 683313688378.dkr.ecr.us-east-1.amazonaws.com/sagemaker-scikit-learn:0.23-1-cpu-py3 found here: https://docs.aws.amazon.com/sagemaker/latest/dg-ecr-paths/ecr-us-east-2.html#sklearn-us-east-2.title
I update the Model resource with the full image reference for the key: spec.forProvider.primaryContainer[0]

I see the following output once I apply that and let resources create:

Screenshot 2023-12-26 at 11 40 23 AM

@Upbound-CLA
Copy link

Upbound-CLA commented Dec 19, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@mbbush mbbush left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! It sounds like you need a bit of help getting your local dev environment set up properly. The command to use to run codegen is simply make generate. You should also test your example using make e2e UPTEST_EXAMPLE_LIST=config/examples/sagemaker/endpoint.yaml. You'll need to install kind, kubectl, and a few other tools, and configure aws credentials in an env var as explained in a comment in the Makefile. Yes, you can make an env var with newlines in it.

examples-generated/sagemaker/endpoint.yaml Show resolved Hide resolved
config/externalnamenottested.go Show resolved Hide resolved
Signed-off-by: Blake Romano <[email protected]>
@blakeromano blakeromano marked this pull request as ready for review December 23, 2023 02:29
matchLabels:
testing.upbound.io/example-name: example
primaryContainer:
- image: ${data.aws_account_id}.dkr.ecr.us-east-1.amazonaws.com/sagemaker-scikit-learn:latest
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the only reason for the manual intervention annotation? I'm not very familiar with Sagemaker, but couldn't you just define the endpoint and EndpointConfiguration in here, and not have a model?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You need to have a model for an EndpointConfiguration and need an EndpointConfiguration for an Endpoint hence the need for the label

Copy link
Collaborator

@turkenf turkenf 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 for your efforts in this PR @blakeromano, I left a few comments for you to consider.
Please fill in how you tested the resource after all corrections as in this PR.

config/sagemaker/config.go Outdated Show resolved Hide resolved
examples/sagemaker/endpoint.yaml Outdated Show resolved Hide resolved
examples/sagemaker/endpoint.yaml Outdated Show resolved Hide resolved
examples/sagemaker/endpoint.yaml Show resolved Hide resolved
examples/sagemaker/endpoint.yaml Outdated Show resolved Hide resolved
examples/sagemaker/endpoint.yaml Outdated Show resolved Hide resolved
examples/sagemaker/endpoint.yaml Outdated Show resolved Hide resolved
examples/sagemaker/endpoint.yaml Outdated Show resolved Hide resolved
Signed-off-by: Blake Romano <[email protected]>
@blakeromano
Copy link
Contributor Author

I believe I have fixed all the PR comments and have updated my comment with a way I was able to validate endpoint creation being successful with Crossplane locally. @turkenf

Copy link
Collaborator

@turkenf turkenf left a comment

Choose a reason for hiding this comment

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

Many thanks for your contribution @blakeromano, I just left a small comment that you should do before merging. Since these examples will appear on the marketplace, it would be better to remove your account ID as I mentioned in the comment below.

examples/sagemaker/endpoint.yaml Outdated Show resolved Hide resolved
Co-authored-by: Fatih Türken <[email protected]>
@turkenf turkenf merged commit 2538b54 into crossplane-contrib:main Dec 26, 2023
9 checks passed
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.

Request for aws_sagemaker_endpoint resource
4 participants