feat: support autoscaling metrics when deploying models #1197

munagekar · 2022-05-05T03:21:23Z

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Fixes #1198

munagekar · 2022-05-05T17:59:52Z

@sararob I see that you have added the do-not-merge label to all open PRs in the repo. Let me know if there's anything specific that you want addressed in this PR.

My team needs this feature to be added to the python sdk to use vertex AI.

munagekar · 2022-05-10T04:23:14Z

@sasha-gitg Could you take a look ?

sararob

Thanks for this contribution! Could you add unit tests in tests/unit/aiplatform/test_endpoints.py for these additions? You can add the following tests:

test_deploy_with_autoscaling_target_cpu_utilization
test_deploy_with_autoscaling_target_accelerator_duty_cycle
test_deploy_with_autoscaling_target_accelerator_duty_cycle_and_no_accelerator_type_or_count_raises (to ensure this raises a ValueError)

Let me know if you need any help adding these.

sararob · 2022-05-10T19:43:23Z

google/cloud/aiplatform/models.py

@@ -980,6 +1011,12 @@ def _deploy_call(
                "Both `accelerator_type` and `accelerator_count` should be specified or None."
            )

+        if not accelerator_type or not accelerator_count and autoscaling_target_accelerator_duty_cycle:


I don't think this check is working as expected (I tried deploying a model with only autoscaling_target_cpu_utilization set and it raised this error). You can change it to something like:

if autoscaling_target_accelerator_duty_cycle is not None and (not accelerator_type or not accelerator_count):

Fixed in 9501eb0

sararob · 2022-05-10T19:43:27Z

google/cloud/aiplatform/models.py

@@ -840,6 +853,13 @@ def _deploy(
                be immediately returned and synced when the Future has completed.
            deploy_request_timeout (float):
                Optional. The timeout for the deploy request in seconds.
+            autoscaling_target_cpu_utilization (int):
+                Target CPU Utilization to use for Autoscaling Replicas.


Could you add "Optional" to the beginning of this docstring? Same for autoscaling_target_accelerator_duty_cycle.

Fixed in 7a601c7

munagekar · 2022-05-11T09:57:50Z

Could you add unit tests in tests/unit/aiplatform/test_endpoints.py for these additions?

@sararob Thank you for the review. I have added in tests. PTAL

sararob

Thanks for adding tests. Left a few comments, once these are addressed and you merge the latest from main into your branch, this should be ready to merge after presubmit tests pass.

sararob · 2022-05-12T13:25:12Z

tests/unit/aiplatform/test_endpoints.py

+                deploy_request_timeout=None,
+                autoscaling_target_accelerator_duty_cycle=70
+            )
+


Could you add the following after the deploy call so the test passes with sync=False?

if not sync: test_endpoint.wait()

None of the other tests which raise errors with the deploy operation wait for the operation to complete except for one.

def test_deploy_raise_error_traffic_80

Can you confirm if you need me to the wait call.

Yes, this test needs the wait call. The ones that don't need it are testing invalid parameter values (negative numbers and > 100 for traffic_percentage so the LRO will never start in those cases.

Fixed in 8a28d7a

sararob · 2022-05-12T13:25:41Z

tests/unit/aiplatform/test_endpoints.py

+            )
+            test_endpoint.deploy(
+                model=test_model,
+                machine_type=_TEST_MACHINE_TYPE,


You can remove the machine_type, service_account, and deploy_request_timeout parameters from this call.

Fixed in fbceb92

sararob · 2022-05-12T13:27:22Z

tests/unit/aiplatform/test_endpoints.py

+        )
+
+        expected_autoscaling_metric_spec = gca_machine_resources.AutoscalingMetricSpec(
+            metric_name="aiplatform.googleapis.com/prediction/online/cpu/utilization",


Could you save this metric_name string to variable at the top of the file, something like _TEST_METRIC_NAME_CPU_UTILIZATION?

Fixed in e14d1ae

sararob · 2022-05-12T13:27:57Z

tests/unit/aiplatform/test_endpoints.py

+        )
+
+        expected_autoscaling_metric_spec = gca_machine_resources.AutoscalingMetricSpec(
+            metric_name="aiplatform.googleapis.com/prediction/online/accelerator/duty_cycle",


Same comment as above re: metric_name string.

Fixed in e14d1ae

munagekar · 2022-05-13T09:02:13Z

Thank you for reviewing the changes. I don't have enough bandwidth this week. I will send in patches early next week, (however feel free to take over the PR if required.)

…ZATION

munagekar · 2022-05-17T10:23:41Z

@sararob I have addressed review comments. PTAL.

sararob

After adding the wait() call in test_deploy_with_autoscaling_target_accelerator_duty_cycle_and_no_accelerator_type_or_count_raises, this should be good to go.

munagekar · 2022-05-17T23:40:15Z

@sararob Thank you for review and clarifications. Updated the PR, PTAL.

sararob

The unit tests passed, you just need to run the linter. You can do this in the root of the repo with:

pip3 install nox && nox -s lint

And then push a commit with the changes. Or if you give me push access to your branch I can run it and merge the PR. Thanks!

munagekar · 2022-05-20T13:48:29Z

@sararob Thank you for the review.I have fixed the linting error, lint check seems to pass locally.

I have already allowed edit by maintainers of googleapis/python-aiplatform for this PR.
Additionally I have sent a collaborator request to you for munagekar/python-aiplatform.

Feel free to take over the PR if there are any other issues.

) * feat: support autoscaling metrics when deploying models * feat: support model deploy to endpoint with autoscaling metrics * fix autoscaling_target_accelerator_duty_cycle check * fix docstring: specify that autoscaling_params are optional * bug fix: add autoscaling_target_cpu_utilization to custom_resource_spec * add tests * add _TEST_METRIC_NAME_CPU_UTILIZATION and _TEST_METRIC_NAME_GPU_UTILIZATION * remove not required arguments in tests * fix tests: wait for LRO to complete even if not sync * fix lint: run black Co-authored-by: Sara Robinson <[email protected]>

product-auto-label bot added size: m Pull request size is medium. api: vertex-ai Issues related to the googleapis/python-aiplatform API. labels May 5, 2022

munagekar added 2 commits May 5, 2022 12:22

feat: support autoscaling metrics when deploying models

03d7885

feat: support model deploy to endpoint with autoscaling metrics

4bb666b

munagekar changed the title ~~support autoscaling metrics when deploying models~~ feat: support autoscaling metrics when deploying models May 5, 2022

munagekar marked this pull request as ready for review May 5, 2022 03:38

sararob added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label May 5, 2022

sararob removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label May 10, 2022

sasha-gitg requested a review from sararob May 10, 2022 18:42

sararob requested changes May 10, 2022

View reviewed changes

munagekar added 4 commits May 11, 2022 16:38

fix autoscaling_target_accelerator_duty_cycle check

9501eb0

fix docstring: specify that autoscaling_params are optional

7a601c7

bug fix: add autoscaling_target_cpu_utilization to custom_resource_spec

b278298

add tests

c82ba65

munagekar requested a review from sararob May 11, 2022 09:56

sararob requested changes May 12, 2022

View reviewed changes

munagekar added 3 commits May 17, 2022 19:01

add _TEST_METRIC_NAME_CPU_UTILIZATION and _TEST_METRIC_NAME_GPU_UTILI…

e14d1ae

…ZATION

remove not required arguments in tests

fbceb92

Merge branch 'main' into feat/autoscaling-endpoint

0f2353e

munagekar requested a review from sararob May 17, 2022 10:23

sararob requested changes May 17, 2022

View reviewed changes

fix tests: wait for LRO to complete even if not sync

8a28d7a

munagekar requested a review from sararob May 17, 2022 23:39

Merge branch 'main' into feat/autoscaling-endpoint

779b805

sararob added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 19, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 19, 2022

sararob requested changes May 20, 2022

View reviewed changes

fix lint: run black

12ad857

munagekar requested a review from sararob May 20, 2022 13:48

sararob added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 20, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 20, 2022

Merge branch 'main' into feat/autoscaling-endpoint

4acd36d

sararob approved these changes May 20, 2022

View reviewed changes

Merge branch 'main' into feat/autoscaling-endpoint

1f19010

sararob added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 23, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 23, 2022

sararob merged commit 095717c into googleapis:main May 23, 2022

release-please bot mentioned this pull request May 23, 2022

chore(main): release 1.13.1 #1216

Merged

munagekar deleted the feat/autoscaling-endpoint branch May 24, 2022 03:55

release-please bot mentioned this pull request Jun 8, 2023

chore(main): release 1.24.1 #2196

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support autoscaling metrics when deploying models #1197

feat: support autoscaling metrics when deploying models #1197

munagekar commented May 5, 2022 •

edited

Loading

munagekar commented May 5, 2022 •

edited

Loading

munagekar commented May 10, 2022

sararob left a comment

sararob May 10, 2022

munagekar May 11, 2022

sararob May 10, 2022

munagekar May 11, 2022

munagekar commented May 11, 2022 •

edited

Loading

sararob left a comment •

edited

Loading

sararob May 12, 2022

munagekar May 17, 2022 •

edited

Loading

sararob May 17, 2022

munagekar May 17, 2022

sararob May 12, 2022

munagekar May 17, 2022

sararob May 12, 2022

munagekar May 17, 2022

sararob May 12, 2022

munagekar May 17, 2022

munagekar commented May 13, 2022 •

edited

Loading

munagekar commented May 17, 2022 •

edited

Loading

sararob left a comment

munagekar commented May 17, 2022

sararob left a comment

munagekar commented May 20, 2022

feat: support autoscaling metrics when deploying models #1197

feat: support autoscaling metrics when deploying models #1197

Conversation

munagekar commented May 5, 2022 • edited Loading

munagekar commented May 5, 2022 • edited Loading

munagekar commented May 10, 2022

sararob left a 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

munagekar commented May 11, 2022 • edited Loading

sararob left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

munagekar May 17, 2022 • edited Loading

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

munagekar commented May 13, 2022 • edited Loading

munagekar commented May 17, 2022 • edited Loading

sararob left a comment

Choose a reason for hiding this comment

munagekar commented May 17, 2022

sararob left a comment

Choose a reason for hiding this comment

munagekar commented May 20, 2022

munagekar commented May 5, 2022 •

edited

Loading

munagekar commented May 5, 2022 •

edited

Loading

munagekar commented May 11, 2022 •

edited

Loading

sararob left a comment •

edited

Loading

munagekar May 17, 2022 •

edited

Loading

munagekar commented May 13, 2022 •

edited

Loading

munagekar commented May 17, 2022 •

edited

Loading