feat: InferenceSpec support for MMS and testing #4763

bryannahm1 · 2024-07-01T22:29:41Z

Issue #, if available:

Description of changes:
Added InferenceSpec support for MMS by adding inference.py file and code changes to testing script, and enabled support in local and endpoint mode.

Testing done:
Integr tests run locally

Merge Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.

General

I have read the CONTRIBUTING doc
I certify that the changes I am introducing will be backward compatible, and I have discussed concerns about this, if any, with the Python SDK team
I used the commit message format described in CONTRIBUTING
I have passed the region in to all S3 and STS clients that I've initialized as part of this change.
I have updated any necessary documentation, including READMEs and API docs (if appropriate)

Tests

I have added tests that prove my fix is effective or that my feature works (if appropriate)
I have added unit and/or integration tests as appropriate to ensure backward compatibility of the changes
I have checked that my tests are not configured for a specific region or account (if appropriate)
I have used unique_name_from_base to create resource names in integ tests (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

grenmester · 2024-07-02T22:52:05Z

src/sagemaker/serve/model_server/multi_model_server/prepare.py

+    image_uri: str,
+    inference_spec: InferenceSpec = None,
+) -> str:
+    """This is a one-line summary of the function.


update docstring

Has been updated, thank you

grenmester · 2024-07-02T22:54:31Z

src/sagemaker/serve/builder/transformers_builder.py

@@ -109,7 +119,7 @@ def _get_hf_metadata_create_model(self) -> Type[Model]:
        """

        hf_model_md = get_huggingface_model_metadata(
-            self.model, self.env_vars.get("HUGGING_FACE_HUB_TOKEN")
+            self.env_vars.get("HF_MODEL_ID"), self.env_vars.get("HUGGING_FACE_HUB_TOKEN")


what happens if model is set with an HF model ID but the environment variable doesn't exist? does the environment variable get set based on self.model before this line is executed?

Yes, HF Model ID gets set before the line is executed. This is where it is called:
https://github.com/aws/sagemaker-python-sdk/blob/master/src/sagemaker/serve/builder/transformers_builder.py#L248-L261
https://github.com/aws/sagemaker-python-sdk/blob/master/src/sagemaker/serve/builder/transformers_builder.py#L263
https://github.com/aws/sagemaker-python-sdk/blob/master/src/sagemaker/serve/builder/transformers_builder.py#L83

grenmester · 2024-07-02T22:56:00Z

src/sagemaker/serve/builder/transformers_builder.py

                )

        self.pysdk_model = self._create_transformers_model()

        if self.mode == Mode.LOCAL_CONTAINER:
            self._prepare_for_mode()

+        logger.info("Model configuration %s", self.pysdk_model)


this logs a lot of info that customers may not necessarily need to see, what's the reason for adding this log line here?

Great comment, I will remove it.

makungaj1 · 2024-07-03T16:39:40Z

src/sagemaker/serve/builder/model_builder.py

@@ -881,8 +881,8 @@ def _build_for_model_server(self):  # pylint: disable=R0911, R1710
        if self.model_metadata:
            mlflow_path = self.model_metadata.get(MLFLOW_MODEL_PATH)

-        if not self.model and not mlflow_path:
-            raise ValueError("Missing required parameter `model` or 'ml_flow' path")
+        if not self.model and not mlflow_path and not self.inference_spec:


Do we need this here? Since we already have a PR for it? #4769

yes these changes can be removed, since i've moved this to another PR and added UT coverage for it

Did the sync, thank you

samruds · 2024-07-04T02:30:10Z

tests/unit/sagemaker/serve/model_server/multi_model_server/test_multi_model_server_prepare.py


 class MultiModelServerPrepareTests(TestCase):
+    def test_start_invoke_destroy_local_multi_model_server(self):


I think we need to re-run this locally to understand where it is failing

samruds · 2024-07-04T02:32:01Z

src/sagemaker/serve/model_server/multi_model_server/inference.py

+        if isinstance(obj[0], InferenceSpec):
+            inference_spec, schema_builder = obj
+
+    logger.info("in model_fn")


This log statement can be removed

It has been removed

samruds · 2024-07-04T02:32:24Z

src/sagemaker/serve/model_server/multi_model_server/inference.py

+
+def predict_fn(input_data, predict_callable):
+    """Placeholder docstring"""
+    logger.info("in predict_fn")


This log statement can be removed

It's now removed

samruds · 2024-07-04T02:32:46Z

src/sagemaker/serve/model_server/multi_model_server/inference.py

+
+
+def predict_fn(input_data, predict_callable):
+    """Placeholder docstring"""


Can we update docstring here with what the method does?

Updated, thank you

samruds · 2024-07-04T02:32:57Z

src/sagemaker/serve/model_server/multi_model_server/inference.py

+
+
+def output_fn(predictions, accept_type):
+    """Placeholder docstring"""


Nit: Update doc string

It's now updated

Aditi2424 · 2024-07-10T07:01:25Z

src/sagemaker/serve/builder/transformers_builder.py

+                self.instance_type,
+            )
+        else:
+            raise ValueError("Cannot detect required model or inference spec")


Can we add more details on how to fix this error. Like what parameter does the customer need to pass to fix this.

Yes, it has just been updated. Thank you.

feat: InferenceSpec support for MMS and testing

aa4a62e

bryannahm1 temporarily deployed to auto-approve July 1, 2024 22:29 — with GitHub Actions Inactive

bryannahm1 temporarily deployed to auto-approve July 2, 2024 20:25 — with GitHub Actions Inactive

bryannahm1 force-pushed the hf-inf-spec-pr branch from 719fe7e to aa4a62e Compare July 2, 2024 20:30

bryannahm1 temporarily deployed to auto-approve July 2, 2024 20:31 — with GitHub Actions Inactive

Fix formatting

895fcf2

bryannahm1 temporarily deployed to auto-approve July 2, 2024 20:32 — with GitHub Actions Inactive

grenmester reviewed Jul 2, 2024

View reviewed changes

bryannahm1 marked this pull request as ready for review July 2, 2024 23:50

bryannahm1 requested a review from a team as a code owner July 2, 2024 23:50

bryannahm1 requested a review from zhaoqizqwang July 2, 2024 23:50

samruds requested review from makungaj1, samruds and jiapinw and removed request for zhaoqizqwang July 2, 2024 23:53

CR Fixes for InferenceSpec MMS

e6209de

bryannahm1 temporarily deployed to auto-approve July 2, 2024 23:59 — with GitHub Actions Inactive

samruds requested a review from gwang111 July 3, 2024 05:34

makungaj1 reviewed Jul 3, 2024

View reviewed changes

remove code

c3aea27

bryannahm1 temporarily deployed to auto-approve July 3, 2024 19:59 — with GitHub Actions Inactive

Merge branch 'master' into hf-inf-spec-pr

b7417eb

bryannahm1 temporarily deployed to auto-approve July 3, 2024 20:01 — with GitHub Actions Inactive

samruds reviewed Jul 4, 2024

View reviewed changes

Add secret_key to endpoint mode

8a2d524

bryannahm1 temporarily deployed to auto-approve July 8, 2024 20:55 — with GitHub Actions Inactive

get_model, docstring, and if changes

368a3d5

bryannahm1 temporarily deployed to auto-approve July 8, 2024 21:20 — with GitHub Actions Inactive

pre-push fixes

3e133b2

bryannahm1 temporarily deployed to auto-approve July 8, 2024 21:29 — with GitHub Actions Inactive

jiapinw approved these changes Jul 8, 2024

View reviewed changes

integ test edits

3e0a98a

bryannahm1 temporarily deployed to auto-approve July 9, 2024 00:23 — with GitHub Actions Inactive

makungaj1 approved these changes Jul 9, 2024

View reviewed changes

bryannahm1 temporarily deployed to auto-approve July 9, 2024 21:03 — with GitHub Actions Inactive

Aditi2424 previously approved these changes Jul 9, 2024

View reviewed changes

Merge branch 'master' into hf-inf-spec-pr

d201b41

bryannahm1 temporarily deployed to auto-approve July 9, 2024 22:21 — with GitHub Actions Inactive

Merge branch 'master' into hf-inf-spec-pr

72055b9

bryannahm1 dismissed Aditi2424’s stale review via 72055b9 July 9, 2024 23:15

bryannahm1 temporarily deployed to auto-approve July 9, 2024 23:15 — with GitHub Actions Inactive

formatting fixes

5c9fd74

bryannahm1 temporarily deployed to auto-approve July 9, 2024 23:24 — with GitHub Actions Inactive

format changes

99d3123

bryannahm1 temporarily deployed to auto-approve July 10, 2024 00:32 — with GitHub Actions Inactive

Aditi2424 reviewed Jul 10, 2024

View reviewed changes

updated value error

52f7fc6

bryannahm1 temporarily deployed to auto-approve July 10, 2024 16:28 — with GitHub Actions Inactive

formatting changes for value error update

a5a7d3e

bryannahm1 temporarily deployed to auto-approve July 10, 2024 16:39 — with GitHub Actions Inactive

bryannahm1 temporarily deployed to auto-approve July 10, 2024 18:05 — with GitHub Actions Inactive

Aditi2424 approved these changes Jul 10, 2024

View reviewed changes

Aditi2424 merged commit 6789b61 into aws:master Jul 10, 2024
10 of 11 checks passed

bryannahm1 deleted the hf-inf-spec-pr branch July 10, 2024 20:11


		class MultiModelServerPrepareTests(TestCase):
		def test_start_invoke_destroy_local_multi_model_server(self):



		def predict_fn(input_data, predict_callable):
		"""Placeholder docstring"""



		def output_fn(predictions, accept_type):
		"""Placeholder docstring"""

feat: InferenceSpec support for MMS and testing #4763

feat: InferenceSpec support for MMS and testing #4763

Uh oh!

Conversation

bryannahm1 commented Jul 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Checklist

General

Tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grenmester Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samruds Jul 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samruds Jul 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samruds Jul 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bryannahm1 commented Jul 1, 2024 •

edited

Loading

grenmester Jul 2, 2024 •

edited

Loading

samruds Jul 3, 2024 •

edited

Loading

samruds Jul 4, 2024 •

edited

Loading

samruds Jul 4, 2024 •

edited

Loading