aws-cdk
aws-cdk copied to clipboard
feat(sagemaker): add model hosting L2 constructs
Based closely on PR #2888, this commit introduces the Endpoint L2 construct and the L2 constructs on which it depends, including Model and EndpointConfig. Departures from PR #2888 include:
- EndpointConfig definition moved into its own L2 construct as one EndpointConfig resource may be shared by multiple Endpoints.
- An Endpoint-specific IEndpointProductionVariant interface was added to support "metric*" and "autoScale*" APIs per endpoint-variant combination.
- The Notebook construct was excluded from this new commit to limit changes to model hosting use-cases only.
- Feedback on the earlier PR has been incorporated into this new commit.
fixes #2809
Co-authored-by: Matt McClean [email protected] Co-authored-by: Long Yao [email protected] Co-authored-by: Drew Jetter [email protected] Co-authored-by: Murali Ganesh [email protected] Co-authored-by: Abilash Rangoju [email protected]
All Submissions:
- [x] Have you followed the guidelines in our Contributing guide?
Adding new Unconventional Dependencies:
- [ ] This PR adds new unconventional dependencies following the process described here
New Features
- [x] Have you added the new feature to an integration test?
- [x] Did you use
yarn integto deploy the infrastructure and generate the snapshot (i.e.yarn integwithout--dry-run)?
- [x] Did you use
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license
Hi @petermeansrock! As you can see from #2888, the cdk team has trouble coming to consensus on the API for new L2s, which usually represent a large surface area. Lots of L2 PRs sit in our PR inbox for a long time, because it's tough to find an entrypoint into the new L2. Internally, when we create new L2s we write an RFC first, which is a design document that helps us envision what an API would look like prior to actually implementing. We're now requiring that process for community-led L2s, with the idea that this will lower the number of stale L2 PRs in our repo. This means less back and forth on the PR itself and community authors will know that their L2 PRs, when written, are in alignment with what the cdk team wants to see. See https://github.com/aws/aws-cdk/pull/20044 for more -- it's not yet in the contributing guide but it soon will be.
We won't look at this PR in it's current state (sorry). Instead, we want to see an RFC first and iterate on that document until we're both satisfied with the surface area. If you're interested in championing that, I suggest you take a look at this RFC for Kinesis Firehose L2 and focus on the README component. It's basically an exercise on stabilizing the README before we get to looking at implementation.
Generally I find this process to be extremely helpful in understanding all use cases, generates a more ergonomic API, and speeds up the review process 10 fold. Please let me know if you have any questions! If you do write an RFC, feel free to link it to this PR and ping me. I'll be sure to take a look at it.
Thanks for the quick response on this new PR, @kaizencc!
... the cdk team has trouble coming to consensus on the API for new L2s, which usually represent a large surface area. Lots of L2 PRs sit in our PR inbox for a long time, because it's tough to find an entrypoint into the new L2. Internally, when we create new L2s we write an RFC first, which is a design document that helps us envision what an API would look like prior to actually implementing.
That makes sense. My co-authors and I spent a fair amount of time finalizing the design for these L2 constructs before producing the original PR, so I can understand that the CDK team would, too, want to take time to vet the design of new L2s.
Instead, we want to see an RFC first and iterate on that document until we're both satisfied with the surface area. If you're interested in championing that, I suggest you take a look at this RFC for Kinesis Firehose L2 and focus on the README component. It's basically an exercise on stabilizing the README before we get to looking at implementation.
Sounds good. As this PR already includes a README, it shouldn't take me long to draft up an RFC mirroring the Kinesis Firehose proposal. I'll aim to get that out for review no later than early next week.
In the meantime, how often should I be merging back into my fork to ensure that this PR isn't prematurely closed (i.e., my last PR was auto-closed without warning)?
@kaizencc Here's a link to the tracking issue for the associated RFC: https://github.com/aws/aws-cdk-rfcs/issues/431
Awesome! Look forward to seeing the RFC. I wouldn't worry about this PR being auto-closed. You can always reopen it (and if that's not true, I can definitely re-open it for you).
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.
I'm going to put this into draft mode until the RFC is finalized and approved. Once that's been done and any relevant changes have been made, please mark this as ready to review.
AWS CodeBuild CI Report
- CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
- Commit ID: e0741ffabc83b97d7fe03c57e65f4dfd921a0f45
- Result: SUCCEEDED
- Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
The RFC associated with this PR has been approved and merged, and the API bar raiser has asked me to break the implementation down into a smaller set of PRs, covering the L2 constructs for Model, EndpointConfig, and Endpoint each in their own PR. Now that the new Model PR has been published, I'm going to close this PR to avoid confusion. Issue #2809 continues to be the best place to track the addition of these model hosting constructs.