flytekit icon indicating copy to clipboard operation
flytekit copied to clipboard

add support for overriding sagemaker model deployment

Open samhita-alla opened this issue 1 year ago • 3 comments

Tracking issue

Why are the changes needed?

This can be useful for deleting an existing deployment before creating a new one if there's already one in place.

What changes were proposed in this pull request?

Add an override parameter to the create_sagemaker_deployment function to delete an existing deployment before creating a new one. It'd be more useful to check if the endpoint, endpoint configuration, and model exist before deletion to avoid Could not find xxx errors. However, implementing conditional checks in the workflow isn't possible with imperative workflows.

Currently, this option can be used to delete all three entities before creating a deployment, given that they exist.

How was this patch tested?

Setup process

Screenshots

Check all the applicable boxes

  • [ ] I updated the documentation accordingly.
  • [ ] All new and existing tests passed.
  • [ ] All commits are signed-off.

Related PRs

Docs link

samhita-alla avatar May 09 '24 09:05 samhita-alla

I think a complementary approach is to create a task SagemakerEndpointExists so that users can compose a workflow themselves with the conditional:

@workflow
def wf():
    (
        if_(SagemakerEndpointExists(name="my-model")
        .then_(create_endpoint(..., override=True))  # delete then create
        .else(create_endpoint(...))  # create
    )

I guess once conditionals in imperative workflows is implemented this pattern wouldn't be necessary

cosmicBboy avatar May 09 '24 14:05 cosmicBboy

I think a complementary approach is to create a task SagemakerEndpointExists so that users can compose a workflow themselves with the conditional:

@cosmicBboy i think we can create a workflow to handle this case as we need to use the list API to check if the endpoint's available. i'll work on it.

samhita-alla avatar May 09 '24 15:05 samhita-alla

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 76.26%. Comparing base (a6a8651) to head (376906a). Report is 15 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2400      +/-   ##
==========================================
- Coverage   76.27%   76.26%   -0.02%     
==========================================
  Files         183      183              
  Lines       18716    18728      +12     
  Branches     3694     3695       +1     
==========================================
+ Hits        14275    14282       +7     
- Misses       3804     3811       +7     
+ Partials      637      635       -2     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar May 09 '24 18:05 codecov[bot]