fix(sdk): Fix SDK model redeployment #466

deadlycoconuts · 2023-09-21T09:40:51Z

What this PR does / why we need it:
This PR introduces a fix to PR #455, which itself introduced a new feature to allow the redeployment of existing Merlin model versions using the deploy method.

While this new feature assumes that a user would be able to only specify fields he/she intends to update in the model version configuration when calling the deploy method, the existing implementation however unexpectedly overwrites some of the existing configuration with certain default configuration values if they are not explicitly specified. These configuration values include:

protocol
deployment mode
resources
autoscaling policy

This happens because the 'deploy' method indirectly sets default values for the aforementioned configuration if they aren't specified (this was primarily intended to make the initial deployment process simpler so users wouldn't have to specify too many config values), and does not distinguish between a user calling deploy on an existing model version or on a new model version.

This PR thus makes changes to the SDK to set these defaults selectively.

A separate change is made to the override method used in the Merlin API server update model version endpoint to override the existing model protocol ONLY when the new model version explicitly specifies a protocol. This change is a partial reversion of PR #300 (change 2) which means that from now on users need to explicitly specify the HTTP protocol (as opposed to leaving it as null) when updating a UPI model to use HTTP instead. It's ultimately a minor change since the main fix that had been introduced in that PR involves change 3 (in that PR).

Which issue(s) this PR fixes:
Fixes #455

Does this PR introduce a user-facing change?:

Users need to explicitly specify the HTTP protocol (as opposed to leaving it as `null`) when updating a UPI model to use HTTP instead

Checklist

Added unit test, integration, and/or e2e tests
Tested locally
Updated documentation
Update Swagger spec if the PR introduce API changes
Regenerated Golang and Python client if the PR introduce API changes

ghost · 2023-09-21T09:42:08Z

👇 Click on the image for a new way to code review

Legend

api/service/version_endpoint_service.go

deadlycoconuts · 2023-09-22T04:07:25Z

python/sdk/merlin/model.py

@@ -1029,6 +1029,21 @@ def deploy(self, environment_name: str = None,
        :param protocol: protocol to be used for deploying the model (default: HTTP_JSON)
        :return: VersionEndpoint object
        """
+        current_endpoint = self.endpoint


This line checks for the existence of an existing endpoint that has been deployed.

deadlycoconuts · 2023-09-22T04:40:00Z

python/sdk/test/integration_test.py

+    # Check that the deployment configs of v2 have remained the same as those used in v1
+    assert endpoint.resource_request.cpu_request == "123m"
+    assert endpoint.resource_request.memory_request == "234Mi"
+    assert endpoint.env_vars == {"green": "TRUE"}
+    assert endpoint.deployment_mode == DeploymentMode.RAW_DEPLOYMENT


Additional checks to ensure the original configs do not get overwritten by the default configs.

I think we should add another test case for redeployment where the first version is deployed with transformer and the second version without transformer. The expected result will be no transformer in the second version, right?

Ahh, that's actually not how the API server works right now though 👀 It seems like the intended behaviour (from the implementation in the API server) for the update endpoint workflow is to only 'override' or 'add on' additional configs to the existing endpoint configs. In the case of the transformer, if there's a transformer in the original 'configs' but no transformer specified in the new 'configs', the existing transformer will simply not get overwritten and remain in the newly deployed endpoint. So there's actually no way to 'remove' the transformer config by using this UpdateEndpoint endpoint.

Actually this behaviour exists for all the other endpoint configs, even the 'null-able' ones, like env vars and the logger. I suppose this is a design choice for the update endpoint workflow and not a bug? In order to remove the transformer, env vars or the logger, I think the user has to create an entirely new model version right? 🤔

Oh, I see. I have forgotten this. Thanks for checking it.

-> I suppose this is a design choice for the update endpoint workflow and not a bug?
Yes, this is a design choice.

-> In order to remove the transformer, env vars or the logger, I think the user has to create an entirely new model version right?
It's not necessarily to create an entirely new model version. You can still using redeploy but with empty or disabled value. For example:

merlin.deploy(v, transformer=StandardTransformer(enabled=False))

api/service/version_endpoint_service.go

python/sdk/merlin/model.py

ariefrahmansyah · 2023-09-25T11:07:53Z

python/sdk/merlin/model.py

+        target_deployment_mode = None
+        if deployment_mode is None:
+            if current_endpoint is None:
+                target_deployment_mode = DeploymentMode.SERVERLESS.value
+        else:
+            target_deployment_mode = deployment_mode.value
+
+        target_protocol = None
+        if protocol is None:
+            if current_endpoint is None:
+                target_protocol = Protocol.HTTP_JSON.value
+        else:
+            target_protocol = protocol.value


Instead of ordering them by each parameter, what do you think of rearranging the order by grouping the activity. For example, something like this:

# Initialize target parameters target_deployment_mode = None target_protocol = None ... # Get the currently deployed endpoint and if there's no deployed endpoint yet, use the default value current_endpoint = self.endpoint if current_endpoint is None: target_deployment_mode = DeploymentMode.SERVERLESS.value target_protocol = Protocol.HTTP_JSON.value ... # Update target values from given user inputs if deployment_mode is not None: target_deployment_mode = deployment_mode.value if protocol is not None: target_protocol = protocol.value ...

Oh yes, that's an awesome suggestion 😁 I was also thinking the deploy method was starting to look a little messy with the setting of each parameter manually but yeah I think your suggestion looks really clean; let me try refactoring this a little, thanks!

Okay I refactored the deploy method quite a bit so now it looks like what you had suggested, and also by creating some helper functions here and there to make the method easier to understand.

ariefrahmansyah · 2023-09-25T11:08:44Z

python/sdk/merlin/model.py

-            resource_request.cpu_request, resource_request.memory_request)
+            if resource_request.gpu_request is not None and resource_request.gpu_name is not None:
+                env_api = EnvironmentApi(self._api_client)
+                env_list = env_api.environments_get()


note: Currently, the deploy() function could call List Environments API up to three times. We can optimize it in different PR. This comment is just me thinking out loud.

That's a good point, while refactoring the deploy method, I actually went to simplify it also so it does not repeat the ListEnvironments API call multiple times. It now calls that API once and reuses the response of that call throughout the entire deploy method.

ariefrahmansyah · 2023-09-25T11:10:32Z

python/sdk/test/integration_test.py

+    # Check that the deployment configs of v2 have remained the same as those used in v1
+    assert endpoint.resource_request.cpu_request == "123m"
+    assert endpoint.resource_request.memory_request == "234Mi"
+    assert endpoint.env_vars == {"green": "TRUE"}
+    assert endpoint.deployment_mode == DeploymentMode.RAW_DEPLOYMENT


I think we should add another test case for redeployment where the first version is deployed with transformer and the second version without transformer. The expected result will be no transformer in the second version, right?

python/sdk/merlin/model.py

deadlycoconuts · 2023-09-26T08:46:32Z

python/sdk/merlin/model.py

@@ -1029,82 +1029,67 @@ def deploy(self, environment_name: str = None,
        :param protocol: protocol to be used for deploying the model (default: HTTP_JSON)
        :return: VersionEndpoint object
        """
+        current_endpoint = self.endpoint
+        env_list = self._get_env_list()


This call reduces all the redundant calls to the ListEnvironments API to just 1 call at the start of the entire method.

deadlycoconuts · 2023-09-26T08:47:35Z

python/sdk/merlin/model.py

+        return target_env_vars
+
+    @staticmethod
+    def _create_transformer_spec(transformer: Transformer, target_env_name: str, env_list: List[client.models.Environment]) -> client.Transformer:


This is actually almost the same as the original create_transformer_spec method - the only changes I made was make this method static and reused some other helper methods.

…vided

… and protocol

…n not specified

deadlycoconuts · 2023-10-12T04:57:32Z

python/sdk/test/conftest.py

@@ -136,7 +136,7 @@ def requests():
    retry_strategy = Retry(
        total=5,
        status_forcelist=[429, 500, 502, 503, 504, 404],
-        method_whitelist=["POST"],
+        allowed_methods=["POST"],


The change in this argument is done because the keyword argument method_whitelist has been deprecated as of urllib3>=2. allowed_methods was introduced as a replacement from urllib3>=1.26 onwards (the arguments for method_whitelist get passed to allowed_methods under the hood anyway).

This was changed because the e2e tests were breaking: https://github.com/caraml-dev/merlin/actions/runs/6490922979/job/17627543254#step:14:222.

Thanks for fixing this!

deadlycoconuts · 2023-10-12T04:58:27Z

python/sdk/setup.py

-    "urllib3>=1.23",
+    "urllib3>=1.26",


Bumped up min version of urllib3. Context: https://github.com/caraml-dev/merlin/pull/466/files#r1356056883

Thanks Zi Yi!

ariefrahmansyah

LGTM! Thanks for improving the codebase as well, Zi Yi!

ariefrahmansyah · 2023-10-12T10:28:09Z

python/sdk/setup.py

-    "urllib3>=1.23",
+    "urllib3>=1.26",


Thanks Zi Yi!

ariefrahmansyah · 2023-10-12T10:28:20Z

python/sdk/test/conftest.py

@@ -136,7 +136,7 @@ def requests():
    retry_strategy = Retry(
        total=5,
        status_forcelist=[429, 500, 502, 503, 504, 404],
-        method_whitelist=["POST"],
+        allowed_methods=["POST"],


Thanks for fixing this!

tkpd-hafizhan

LGTM

deadlycoconuts · 2023-10-13T02:44:37Z

Thanks a lot for the detailed review and the suggestions @ariefrahmansyah and @tkpd-hafizhan ! Merging this now! 🚀

commit 637050b Author: Ewe Zi Yi <36802364+deadlycoconuts@users.noreply.github.com> Date: Fri Oct 13 10:45:16 2023 +0800 fix(sdk): Fix SDK model redeployment (#466) **What this PR does / why we need it**: This PR introduces a fix to PR #455, which itself introduced a new feature to allow the redeployment of existing Merlin model versions using the `deploy` method. While this new feature assumes that a user would be able to only **_specify fields he/she intends to update_** in the model version configuration when calling the `deploy` method, the existing implementation however unexpectedly overwrites some of the existing configuration with certain default configuration values if they are not explicitly specified. These configuration values include: - protocol - deployment mode - resources - autoscaling policy This happens because the 'deploy' method indirectly sets default values for the aforementioned configuration if they aren't specified (this was primarily intended to make the **initial** deployment process simpler so users wouldn't have to specify too many config values), and does not distinguish between a user calling `deploy` on an existing model version or on a new model version. This PR thus makes changes to the SDK to set these defaults selectively. A separate change is made to the `override` method used in the Merlin API server update model version endpoint to override the existing model protocol ONLY when the new model version explicitly specifies a protocol. This change is a **_partial reversion_** of PR #300 (change 2) which means that from now on users need to explicitly specify the HTTP protocol (as opposed to leaving it as `null`) when updating a UPI model to use HTTP instead. It's ultimately a minor change since the main fix that had been introduced in that PR involves change 3 (in that PR). **Which issue(s) this PR fixes**: Fixes #455 **Does this PR introduce a user-facing change?**: ```release-note Users need to explicitly specify the HTTP protocol (as opposed to leaving it as `null`) when updating a UPI model to use HTTP instead ``` **Checklist** - [x] Added unit test, integration, and/or e2e tests - [x] Tested locally - [ ] Updated documentation - [ ] Update Swagger spec if the PR introduce API changes - [ ] Regenerated Golang and Python client if the PR introduce API changes

…out deployment type specified (#474) **What this PR does / why we need it**: As a follow up to #455 and #466, this tiny change to the update endpoint _endpoint_ allows requests that do not specify a deployment type to avoid the check that checks for different deployment types between a deployed endpoint and the new endpoint configuration. What will instead happen if a deployment type isn't specified is that the existing deployed endpoint's deployment type will be used. The main change is just the addition of another boolean condition, but there's a unit test that I added which is just an almost exact copy of another unit test case. **Which issue(s) this PR fixes**: Fixes # **Does this PR introduce a user-facing change?**: ```release-note NONE ``` **Checklist** - [x] Added unit test, integration, and/or e2e tests - [x] Tested locally - [ ] Updated documentation - [ ] Update Swagger spec if the PR introduce API changes - [ ] Regenerated Golang and Python client if the PR introduce API changes

deadlycoconuts added the bug Something isn't working label Sep 21, 2023

deadlycoconuts self-assigned this Sep 21, 2023

deadlycoconuts force-pushed the fix_sdk_model_redeployment branch from c729a13 to 654b4cd Compare September 21, 2023 10:05

deadlycoconuts commented Sep 22, 2023

View reviewed changes

api/service/version_endpoint_service.go Outdated Show resolved Hide resolved

deadlycoconuts commented Sep 22, 2023

View reviewed changes

deadlycoconuts requested review from ariefrahmansyah, krithika369 and tkpd-hafizhan September 22, 2023 04:26

deadlycoconuts marked this pull request as ready for review September 22, 2023 04:27

deadlycoconuts commented Sep 22, 2023

View reviewed changes

deadlycoconuts force-pushed the fix_sdk_model_redeployment branch from a0e7469 to c70ab65 Compare September 22, 2023 05:41

ariefrahmansyah reviewed Sep 25, 2023

View reviewed changes

deadlycoconuts force-pushed the fix_sdk_model_redeployment branch 2 times, most recently from 5400bf2 to 9f4c510 Compare September 26, 2023 07:58

deadlycoconuts commented Sep 26, 2023

View reviewed changes

python/sdk/merlin/model.py Outdated Show resolved Hide resolved

deadlycoconuts commented Sep 26, 2023

View reviewed changes

deadlycoconuts added 11 commits October 6, 2023 15:14

Fix SDK deploy method

27e27e5

Revert change so that protocol is overridden only when explicitly pro…

d626e75

…vided

Update client deploy method to expect none values for deployment mode…

761215f

… and protocol

Fix bug in packing deployment configs in api request

e81b7f8

Fix broken unit tests by adding additional mocked endpoint

dadc394

Update integration tests

3c9eefd

Remove redundant call to retrieve the current endpoint of a model

0bf04a8

Refactor deploy method

c042d64

Fix deploy method assignment bugs

67f655d

Make target env non-nullable

b6d8a50

Fix helper method

90685e7

deadlycoconuts force-pushed the fix_sdk_model_redeployment branch from 09db525 to 90685e7 Compare October 6, 2023 07:44

deadlycoconuts added 3 commits October 6, 2023 15:45

Set default protocol if it is not specified explicitly

984015d

Set env name as either the default or the existing endpoint's env whe…

ab50d4a

…n not specified

Update deploy method to support redeployment in non-default environments

69f1870

deadlycoconuts force-pushed the fix_sdk_model_redeployment branch from 995321f to 69f1870 Compare October 12, 2023 03:33

deadlycoconuts added 4 commits October 12, 2023 11:54

Refactor default env getter into a package level helper metohd

6c97326

Refactor deploy method to look for endpoints in specified environment

7ae0aaa

Fix type hinting

e4ba52f

Bump up urllib3 dependencies

137a914

deadlycoconuts commented Oct 12, 2023

View reviewed changes

deadlycoconuts requested a review from ariefrahmansyah October 12, 2023 05:03

Fix bug in argument passed to helper method

a9048f3

ariefrahmansyah approved these changes Oct 12, 2023

View reviewed changes

ariefrahmansyah mentioned this pull request Oct 12, 2023

Add standard transformer Simulate Feast Test #470

Merged

5 tasks

tkpd-hafizhan approved these changes Oct 13, 2023

View reviewed changes

deadlycoconuts merged commit 637050b into main Oct 13, 2023
42 checks passed

deadlycoconuts deleted the fix_sdk_model_redeployment branch October 13, 2023 02:45

deadlycoconuts mentioned this pull request Oct 19, 2023

fix(api): Add additional check to allow update endpoint requests without deployment type specified #474

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sdk): Fix SDK model redeployment #466

fix(sdk): Fix SDK model redeployment #466

deadlycoconuts commented Sep 21, 2023 •

edited

ghost commented Sep 21, 2023 •

edited by ghost

Legend

deadlycoconuts Sep 22, 2023 •

edited

deadlycoconuts Sep 22, 2023

ariefrahmansyah Sep 25, 2023

deadlycoconuts Sep 26, 2023 •

edited

ariefrahmansyah Oct 2, 2023

ariefrahmansyah Sep 25, 2023

deadlycoconuts Sep 26, 2023

deadlycoconuts Sep 26, 2023

ariefrahmansyah Sep 25, 2023

deadlycoconuts Sep 26, 2023

ariefrahmansyah Sep 25, 2023

deadlycoconuts Sep 26, 2023

deadlycoconuts Sep 26, 2023

deadlycoconuts Oct 12, 2023 •

edited

ariefrahmansyah Oct 12, 2023

deadlycoconuts Oct 12, 2023

ariefrahmansyah Oct 12, 2023

ariefrahmansyah left a comment

ariefrahmansyah Oct 12, 2023

ariefrahmansyah Oct 12, 2023

tkpd-hafizhan left a comment

deadlycoconuts commented Oct 13, 2023

fix(sdk): Fix SDK model redeployment #466

fix(sdk): Fix SDK model redeployment #466

Conversation

deadlycoconuts commented Sep 21, 2023 • edited

ghost commented Sep 21, 2023 • edited by ghost

Legend

deadlycoconuts Sep 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deadlycoconuts Sep 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deadlycoconuts Oct 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariefrahmansyah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkpd-hafizhan left a comment

Choose a reason for hiding this comment

deadlycoconuts commented Oct 13, 2023

deadlycoconuts commented Sep 21, 2023 •

edited

ghost commented Sep 21, 2023 •

edited by ghost

deadlycoconuts Sep 22, 2023 •

edited

deadlycoconuts Sep 26, 2023 •

edited

deadlycoconuts Oct 12, 2023 •

edited