feat: Support for ModelBuilder In_Process Mode (1/2) #4784

bryannahm1 · 2024-07-13T08:06:30Z

Issue #, if available:

Description of changes:
Adding new mode of deployment named in_process_mode. Added in_process script to mode, made edits to transformers and model builder in order to support.

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.

makungaj1 · 2024-07-16T02:43:59Z

src/sagemaker/serve/builder/model_builder.py

+                "IN_PROCESS mode is not supported yet for model server. It is "
+                "supported for MMS/Transformers server in beta release."


nit: IN_PROCESS mode is only supported for MMS/Transformers server in beta release.

This is better wording, I will change it thank you!

makungaj1 · 2024-07-16T02:46:56Z

src/sagemaker/serve/builder/transformers_builder.py

@@ -161,7 +164,7 @@ def _get_hf_metadata_create_model(self) -> Type[Model]:
                vpc_config=self.vpc_config,
            )

-        if not self.image_uri and self.mode == Mode.LOCAL_CONTAINER:
+        if self.mode == Mode.LOCAL_CONTAINER or self.mode == Mode.IN_PROCESS:


nit:

LOCAL_MODES = [Mode.LOCAL_CONTAINER, Mode.IN_PROCESS] if self.mode in LOCAL_MODES: ...

Good suggestion, I've made the edit, thank you

makungaj1 · 2024-07-16T02:48:36Z

src/sagemaker/serve/builder/transformers_builder.py

@@ -274,7 +293,7 @@ def _build_transformers_env(self):

        self.pysdk_model = self._create_transformers_model()

-        if self.mode == Mode.LOCAL_CONTAINER:
+        if self.mode == Mode.LOCAL_CONTAINER or self.mode == Mode.IN_PROCESS:


nit:

if self.mode in LOCAL_MODES: ...

Made the change

makungaj1 · 2024-07-16T02:49:39Z

src/sagemaker/serve/mode/in_process_mode.py

+_PING_HEALTH_CHECK_INTERVAL_SEC = 5
+
+_PING_HEALTH_CHECK_FAIL_MSG = (
+    "Container did not pass the ping health check. "


Does IN_PROCESS mode uses Container?

No it does not, I will be sure to change this, good catch.

samruds · 2024-07-16T16:45:03Z

src/sagemaker/serve/mode/in_process_mode.py

+    ):
+        """Placeholder docstring"""
+
+        # self._pull_image(image=image)


Can remove this this comment

samruds · 2024-07-16T16:45:48Z

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

+    def _multi_model_server_deep_ping(self, predictor: PredictorBase):
+        """Placeholder docstring"""
+        response = None
+        logger.debug("AM I HERE? PING PING")


Remove this line

Removed it, thank you

samruds · 2024-07-16T16:45:58Z

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

+        )
+
+    def _invoke_multi_model_server_serving(self, request: object, content_type: str, accept: str):
+        """Placeholder docstring"""


Update doc strings

The docstrings are updated, good catch.

samruds · 2024-07-16T16:46:55Z

tests/unit/sagemaker/serve/builder/test_model_builder.py

@@ -67,10 +67,18 @@
 class TestModelBuilder(unittest.TestCase):
    @patch("sagemaker.serve.builder.model_builder._ServeSettings")
    def test_validation_in_progress_mode_not_supported(self, mock_serveSettings):


Can we add a test a where it is supported?

Yes, I will add it. Good suggestion.

makungaj1 · 2024-07-16T22:45:26Z

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

+
+        return (True, response)
+
+    def _multi_model_server_deep_ping(self, predictor: PredictorBase):


is this complete?

I have stubbed it.

makungaj1 · 2024-07-16T22:46:02Z

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

+        else:
+            env_vars = env
+
+        self.container = client.containers.run(


Are we spinning up a docker container or using fast api for serving?

The container will be stubbed, this is only 1/2 of the full implementation of InProcess mode. My next PR will include the FastAPI.

samruds

LGTM, please ensure existing servers dont break. Please run notebook against this change to ensure backward compatibility...

makungaj1 · 2024-07-18T17:57:04Z

Can we have this merge in a feature branch and merge to master until all complete and fully tested? cc @samruds

samruds · 2024-07-18T19:00:31Z

Can we have this merge in a feature branch and merge to master until all complete and fully tested? cc @samruds

Proposed split to enable to merge to mainline.

1/2 -> introduce in process mode , it should hit no-op stubs and return an empty response. Other servers should not be impacted.

2/2 - we introduce the fast API logic into the stubs. At this point the response should be a valid inference response. Other servers should not be impacted.

Bryannah Hernandez and others added 9 commits June 26, 2024 16:42

InferenceSpec support for HF

2cc906b

Merge branch 'aws:master' into hf-inf-spec-support

b25295a

feat: InferenceSpec support for MMS and testing

fb28458

Introduce changes for InProcess Mode

3576ea9

mb_inprocess updates

d3b8e9b

In_Process mode for TGI transformers, edits

68cede1

Remove InfSpec from branch

02e54ef

merge from master for inf spec

f39cca6

changes to support in_process

cc0ca14

bryannahm1 temporarily deployed to auto-approve July 13, 2024 08:06 — with GitHub Actions Inactive

changes to get pre-checks passing

18fc3f2

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

pylint fix

495c7b4

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

unit test, test mb

1121f47

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

period missing, added

b6062a7

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

makungaj1 reviewed Jul 16, 2024

View reviewed changes

samruds self-requested a review July 16, 2024 04:06

samruds reviewed Jul 16, 2024

View reviewed changes

suggestions and test added

1ec209c

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

pre-push fix

ca6c818

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

missing an @

cd3dbaa

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

makungaj1 reviewed Jul 16, 2024

View reviewed changes

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

samruds approved these changes Jul 17, 2024

View reviewed changes

tests for in process mode

b40f36c

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

prepush fix

68000e1

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

bryannahm1 marked this pull request as ready for review July 18, 2024 18:48

bryannahm1 requested a review from a team as a code owner July 18, 2024 18:48

bryannahm1 requested a review from nargokul July 18, 2024 18:48

makungaj1 approved these changes Jul 18, 2024

View reviewed changes

This was referenced Aug 1, 2024

feat: Pulling in dependencies (in_process mode) using conda environment #4807

Merged

feat: FastAPI integration for In_Process Mode (2/2) #4808

Merged

minor fix

826c5c4

bryannahm1 temporarily deployed to auto-approve August 6, 2024 23:51 — with GitHub Actions Inactive

Merge branch 'master' into mb_in_process

1fd6291

bryannahm1 temporarily deployed to auto-approve August 7, 2024 05:15 — with GitHub Actions Inactive

Merge branch 'master' into mb_in_process

de6f861

sage-maker temporarily deployed to auto-approve August 8, 2024 20:57 — with GitHub Actions Inactive

Merge branch 'master' into mb_in_process

5cc24ba

sage-maker temporarily deployed to auto-approve August 9, 2024 06:25 — with GitHub Actions Inactive

Merge branch 'master' into mb_in_process

64efa90

sage-maker temporarily deployed to auto-approve August 9, 2024 16:02 — with GitHub Actions Inactive

sage-maker requested review from makungaj1 and samruds August 9, 2024 17:11

samruds approved these changes Aug 9, 2024

View reviewed changes

makungaj1 approved these changes Aug 9, 2024

View reviewed changes

sage-maker approved these changes Aug 9, 2024

View reviewed changes

sage-maker merged commit a870e19 into aws:master Aug 9, 2024
14 checks passed

		"IN_PROCESS mode is not supported yet for model server. It is "
		"supported for MMS/Transformers server in beta release."


		return (True, response)

		def _multi_model_server_deep_ping(self, predictor: PredictorBase):

feat: Support for ModelBuilder In_Process Mode (1/2) #4784

feat: Support for ModelBuilder In_Process Mode (1/2) #4784

Uh oh!

Conversation

bryannahm1 commented Jul 13, 2024

Merge Checklist

General

Tests

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!

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!

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

Choose a reason for hiding this comment

Uh oh!

makungaj1 commented Jul 18, 2024

Uh oh!

samruds commented Jul 18, 2024

Uh oh!

Uh oh!

Uh oh!