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

handle existing sagemaker deployments gracefully #2400

Merged
merged 20 commits into from
Jul 15, 2024

Conversation

samhita-alla
Copy link
Contributor

@samhita-alla samhita-alla commented May 9, 2024

Tracking issue

Why are the changes needed?

This can be useful for generating deterministic names for sagemaker entities with an "idempotence token", and to avoid raising an error if the deployment already exists, instead returning the existing deployment.

An idempotence token is useful for generating different tokens given different configurations. It avoids name collisions when updating the configuration.

What changes were proposed in this pull request?

  • Add try-catch to avoid raising exceptions when the deployment already exists.
  • Update the endpoint agent's output.
  • Add "idempotence token" as the mixin's output.
  • Update the workflow's output.

UX:

  • idempotence_token corresponds to the config hash
  • inputs.idempotence_token corresponds to the idempotence token of the previous task. Since create_sagemaker_deployment is a workflow, it injects the previous task's idempotence token into the current task as an input.
from flytekit import kwtypes, workflow
from flytekit.types.file import FlyteFile
from flytekitplugins.awssagemaker_inference import create_sagemaker_deployment

sd_deployment = create_sagemaker_deployment(
    name="stable-diffusion",
    model_input_types=kwtypes(deployment_name=str, ...),
    model_config={
        "ModelName": "{inputs.deployment_name}-{idempotence_token}",
        ...
    },
    endpoint_config_input_types=kwtypes(deployment_name=str, ...),
    endpoint_config_config={
        "EndpointConfigName": "{inputs.deployment_name}-{idempotence_token}",
        "ProductionVariants": [
            {
                "ModelName": "{inputs.deployment_name}-{inputs.idempotence_token}",
                ...
            },
        ],
    },
    endpoint_input_types=kwtypes(deployment_name=str),
    endpoint_config={
        "EndpointName": "{inputs.deployment_name}-{idempotence_token}",
        "EndpointConfigName": "{inputs.deployment_name}-{inputs.idempotence_token}",
    },
)


@workflow
def wf(
    execution_role_arn: str = "...",
    deployment_name: str = "stable-diffusion-model",
    instance_type: str = "ml.g5.2xlarge",
    initial_instance_count: int = 1,
    region: str = "us-east-2",
) -> list[dict]:
    return sd_deployment(...)

If the deployment already exists, here's how the result might look: [{'result': 'Entity already exists: arn:aws:sagemaker:us-east-2:123456789:model/stable-diffusion-model-0b2634d258f999f6'}, {'result': 'Entity already exists: arn:aws:sagemaker:us-east-2:123456789:endpoint-config/stable-diffusion-model-bc3afab2046ae0df'}, {'EndpointArn': 'arn:aws:sagemaker:us-east-2:123456789:endpoint/stable-diffusion-model-3cfe9fbf5941f3c2'}]

If only some entities exist, such as the model and endpoint configuration, the plugin detects that these two entities exist and only creates an endpoint to complete the deployment.

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

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>
Signed-off-by: Samhita Alla <aallasamhita@gmail.com>
Signed-off-by: Samhita Alla <aallasamhita@gmail.com>
Signed-off-by: Samhita Alla <aallasamhita@gmail.com>
@cosmicBboy
Copy link
Contributor

cosmicBboy commented May 9, 2024

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

@samhita-alla
Copy link
Contributor Author

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.

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>
Signed-off-by: Samhita Alla <aallasamhita@gmail.com>
Signed-off-by: Samhita Alla <aallasamhita@gmail.com>
Copy link

codecov bot commented May 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>
Signed-off-by: Samhita Alla <aallasamhita@gmail.com>
Signed-off-by: Samhita Alla <aallasamhita@gmail.com>
Signed-off-by: Samhita Alla <aallasamhita@gmail.com>
Signed-off-by: Samhita Alla <aallasamhita@gmail.com>
Signed-off-by: Samhita Alla <aallasamhita@gmail.com>
Signed-off-by: Samhita Alla <aallasamhita@gmail.com>
Signed-off-by: Samhita Alla <aallasamhita@gmail.com>
Signed-off-by: Samhita Alla <aallasamhita@gmail.com>
Signed-off-by: Samhita Alla <aallasamhita@gmail.com>
Signed-off-by: Samhita Alla <aallasamhita@gmail.com>
Signed-off-by: Samhita Alla <aallasamhita@gmail.com>
@samhita-alla samhita-alla changed the title add support for overriding sagemaker model deployment handle existing sagemaker deployments gracefully Jul 11, 2024
@samhita-alla samhita-alla marked this pull request as ready for review July 11, 2024 17:08
@samhita-alla samhita-alla merged commit 097e9e8 into master Jul 15, 2024
46 checks passed
fiedlerNr9 pushed a commit that referenced this pull request Jul 25, 2024
* add support to override model deployment

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* fix task chaining

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* fix task chaining

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* nit

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* add is deployment exists function

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* add override

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* mutable fix

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* nit

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* update workflow

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* add idempotence token and deployment exists check

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* fix init

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* fix workflow output

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* fix wf output

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* idempotence token as task output

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* move try catch to mixin

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* fix wf code

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* move try catch to agents

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* fixed all bugs

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* fix tests

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

---------

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>
Signed-off-by: Jan Fiedler <jan@union.ai>
mao3267 pushed a commit to mao3267/flytekit that referenced this pull request Jul 29, 2024
* add support to override model deployment

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* fix task chaining

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* fix task chaining

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* nit

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* add is deployment exists function

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* add override

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* mutable fix

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* nit

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* update workflow

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* add idempotence token and deployment exists check

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* fix init

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* fix workflow output

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* fix wf output

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* idempotence token as task output

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* move try catch to mixin

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* fix wf code

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* move try catch to agents

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* fixed all bugs

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

* fix tests

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>

---------

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>
Signed-off-by: mao3267 <chenvincent610@gmail.com>
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.

3 participants