Migrate base inference toolkit scripts and unit tests #157

sachanub · 2023-10-16T20:52:43Z

Issue #, if available:

Description of changes:

The objective of this PR is to migrate the base inference toolkit scripts and unit tests to the PyTorch inference toolkit to remove the dependency on the base inference toolkit repository.

Base inference toolkit package src/sagemaker_inference:
- __init__.py (original script): No change.
- utils.py (original script): No change.
- logging.py (original script): No change.
- errors.py (original script): No change.
- content_types.py (original script): No change.
- encoder.py (original script): No change.
- decoder.py (original script): No change.
- parameters.py (original script):
  - Retained:
    - BASE_PATH_ENV
    - USER_PROGRAM_ENV
    - DEFAULT_INVOCATIONS_ACCEPT_ENV
    - MODEL_SERVER_WORKERS_ENV
    - MODEL_SERVER_TIMEOUT_ENV
    - BIND_TO_PORT_ENV
    - MULTI_MODEL_ENV
    - LOG_LEVEL_ENV
    - SAFE_PORT_RANGE_ENV
  - Removed:
    - MODEL_SERVER_VMARGS: While it is used in model_server.py, it is not used anywhere in torchserve.py. This is defined in default-ts.properties and mme-ts.properties.
    - MODEL_SERVER_TIMEOUT_SECONDS_ENV: This was required for MMS, but not TorchServe (Add support for SAGEMAKER_MODEL_SERVER_TIMEOUT_SECONDS variable sagemaker-inference-toolkit#129)
    - STARTUP_TIMEOUT_ENV: Used to retrieve a MMS server process with a custom timeout value in model_server.py. In torchserve.py, the timeout value is fixed.
    - MAX_REQUEST_SIZE: While it is used in model_server.py it is not used anywhere in torchserve.py.
- environment.py (original script): Any methods/attributes corresponding to parameters retained in parameters.py will be retained. The rest will be removed. Specifically, we remove the following:
  - DEFAULT_STARTUP_TIMEOUT, self._startup_timeout, self.startup_timeout()
  - self. _model_server_timeout_seconds, self.model_server_timeout_seconds()
  - DEFAULT_VMARGS, self._vmargs, self.vmargs()
  - DEFAULT_MAX_REQUEST_SIZE, self._max_request_size_in_mb, self. max_request_size()
- model_server.py (original script):
  - Attributes:
    - Retained: We retained the attributes which are used to install requirements from requirements.txt in torchserve.py.
      - REQUIREMENTS_PATH
    - Removed: The attributes mentioned below were removed because either their equivalents are present in torchserve.py or they are used by legacy methods:
      - MMS_CONFIG_FILE
      - DEFAULT_HANDLER_SERVICE
      - DEFAULT_MMS_CONFIG_FILE
      - MME_MMS_CONFIG_FILE
      - DEFAULT_MMS_LOG_FILE
      - DEFAULT_MMS_MODEL_EXPORT_DIRECTORY
      - DEFAULT_MMS_MODEL_NAME
      - ENABLE_MULTI_MODEL, MODEL_STORE
      - PYTHON_PATH_ENV
      - MMS_NAMESPACE
  - Methods:
    - Retained: We retained the methods which are used to install requirements from requirements.txt in torchserve.py.
      - _install_requirements()
      - _get_codeartifact_index()
    - Removed: The methods mentioned below have been removed because either their equivalents are present in torchserve.py or they are legacy methods.
      - _start_model_server()
      - _adapt_to_mms_format()
      - _set_python_path()
      - _create_model_server_config_file()
      - _generate_mms_config_properties()
      - _add_sigterm_handler()
      - _retry_retrieve_mms_server_process()
      - _retrieve_mms_server_process()
      - _reap_children()
      - _add_sigchild_handler()
- default_handler_service.py (original script): No change.
- default_inference_handler.py (original script): No change.
- transformer.py (original script): No change.

All the unit tests from the base inference toolkit have been moved to the PyTorch inference toolkit. Removed any unit tests which test functions and attributes which have been removed from the base inference toolkit scripts. Specifically, we will have the following:

Unit tests test/unit:
- Renamed test_default_inference_handler.py to test_default_pytorch_inference_handler.py.
- Renamed test_model_server.py to test_torchserve.py.
- test_decoder.py (original script): No change.
- test_default_handler_service.py (original script): No change.
- test_default_inference_handler.py (original script): No change.
- test_encoder.py (original script): No change.
- test_environment.py (original script): In the test_env(), removed the assertions for the following:
  - env.max_request_size
  - env.vmargs
  - env.startup_timeout
  - env.model_server_timeout_seconds
- test_model_server.py (original script):
  - Retained:
    - test_install_requirements()
    - test_install_requirements_installation_failed()
    - test_install_requirements_codeartifact_invalid_arn_installation_failed()
    - test_install_requirements_codeartifact()
  - Removed:
    - test_start_model_server_default_service_handler()
    - test_start_model_server_custom_handler_service()
    - test_adapt_to_mms_format()
    - test_adapt_to_mms_format_existing_path()
    - test_set_existing_python_path()
    - test_new_python_path()
    - test_create_model_server_config_file()
    - test_generate_mms_config_properties()
    - test_generate_mms_config_properties_default_workers()
    - test_add_sigterm_handler()
    - test_retrieve_mms_server_process()
    - test_retrieve_mms_server_process_no_server()
    - test_retrieve_mms_server_process_too_many_servers()
    - test_retry_retrieve_mms_server_process()
- test_transformer.py (original script): No change.
- test_utils.py (original script): No change.
tox.ini: Modify the coverage run command to include sagemaker_inference in the source .
setup.py: Remove sagemaker_inference as a dependency and include the base toolkit’s dependencies.
test/container: Uninstall sagemaker_inference in the following Dockerfiles.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

namannandan

Overall LGTM, apart from a few minor comments/questions.

namannandan · 2023-10-18T22:45:54Z

src/sagemaker_inference/content_types.py

+# distributed on an 'AS IS' BASIS, WITHOUT WARRANTIES OR CONDITIONS OF
+# ANY KIND, either express or implied. See the License for the specific
+# language governing permissions and limitations under the License.
+from __future__ import absolute_import


Why do we need this package? Is it for backwards compatibility with older python versions?
Same applies to following files in which this line is added.

This link offers details to the relevance of from __future__ import absolute_import:
https://portingguide.readthedocs.io/en/latest/imports.html

Also, this line is required for the flake8 checks.

namannandan · 2023-10-18T23:42:50Z

test/container/2.0.0/Dockerfile.dlc.cpu

@@ -3,6 +3,6 @@ FROM 763104351884.dkr.ecr.$region.amazonaws.com/pytorch-inference:2.0.0-cpu-py31

 COPY dist/sagemaker_pytorch_inference-*.tar.gz /sagemaker_pytorch_inference.tar.gz

-RUN pip uninstall -y sagemaker_pytorch_inference && \
+RUN pip uninstall -y sagemaker_inference sagemaker_pytorch_inference && \


Nit: package name

Suggested change

RUN pip uninstall -y sagemaker_inference sagemaker_pytorch_inference && \

RUN pip uninstall -y sagemaker-inference sagemaker_pytorch_inference && \

Same applies to the remaining references to sagemaker-inference where we uninstall it.

Although this does not seem to affect the container build, it does successfully uninstall sagemaker-inference from the base image.

Maybe I am missing something here, but I think we can specify either sagemaker-inference or sagemaker_inference when attempting pip uninstall. I tried uninstalling the base toolkit with both names and it works successfully with both.

ubuntu@ip-172-31-31-136:~/code$ pip uninstall -y sagemaker-inference Found existing installation: sagemaker-inference 1.10.0 Uninstalling sagemaker-inference-1.10.0: Successfully uninstalled sagemaker-inference-1.10.0 ubuntu@ip-172-31-31-136:~/code$ pip uninstall -y sagemaker_inference Found existing installation: sagemaker-inference 1.10.0 Uninstalling sagemaker-inference-1.10.0: Successfully uninstalled sagemaker-inference-1.10.0

Also, according to the logs while building the container, the base toolkit is successfully uninstalled:

#8 [3/3] RUN pip uninstall -y sagemaker_inference sagemaker_pytorch_inference && pip install --upgrade --no-cache-dir /sagemaker_pytorch_inference.tar.gz && rm /sagemaker_pytorch_inference.tar.gz -- 945 | #8 8.150 Found existing installation: sagemaker-inference 1.9.2 946 | #8 8.152 Uninstalling sagemaker-inference-1.9.2: 947 | #8 8.153 Successfully uninstalled sagemaker-inference-1.9.2 948 | #8 8.155 Found existing installation: sagemaker-pytorch-inference 2.0.14 949 | #8 8.157 Uninstalling sagemaker-pytorch-inference-2.0.14: 950 | #8 8.159 Successfully uninstalled sagemaker-pytorch-inference-2.0.14

namannandan · 2023-10-18T23:43:32Z

test/container/2.0.0/Dockerfile.dlc.gpu

@@ -3,6 +3,6 @@ FROM 763104351884.dkr.ecr.$region.amazonaws.com/pytorch-inference:2.0.0-gpu-py31

 COPY dist/sagemaker_pytorch_inference-*.tar.gz /sagemaker_pytorch_inference.tar.gz

-RUN pip uninstall -y sagemaker_pytorch_inference && \
+RUN pip uninstall -y sagemaker_inference sagemaker_pytorch_inference && \


Suggested change

RUN pip uninstall -y sagemaker_inference sagemaker_pytorch_inference && \

RUN pip uninstall -y sagemaker-inference sagemaker_pytorch_inference && \

namannandan · 2023-10-18T23:43:48Z

test/container/2.0.1/Dockerfile.dlc.cpu

@@ -3,6 +3,6 @@ FROM 763104351884.dkr.ecr.$region.amazonaws.com/pytorch-inference:2.0.1-cpu-py31

 COPY dist/sagemaker_pytorch_inference-*.tar.gz /sagemaker_pytorch_inference.tar.gz

-RUN pip uninstall -y sagemaker_pytorch_inference && \
+RUN pip uninstall -y sagemaker_inference sagemaker_pytorch_inference && \


Suggested change

RUN pip uninstall -y sagemaker_inference sagemaker_pytorch_inference && \

RUN pip uninstall -y sagemaker-inference sagemaker_pytorch_inference && \

namannandan · 2023-10-18T23:44:02Z

test/container/2.0.1/Dockerfile.dlc.gpu

@@ -3,6 +3,6 @@ FROM 763104351884.dkr.ecr.$region.amazonaws.com/pytorch-inference:2.0.1-gpu-py31

 COPY dist/sagemaker_pytorch_inference-*.tar.gz /sagemaker_pytorch_inference.tar.gz

-RUN pip uninstall -y sagemaker_pytorch_inference && \
+RUN pip uninstall -y sagemaker_inference sagemaker_pytorch_inference && \


Suggested change

RUN pip uninstall -y sagemaker_inference sagemaker_pytorch_inference && \

RUN pip uninstall -y sagemaker-inference sagemaker_pytorch_inference && \

Merge base inference toolkit scripts and unit tests

5cff461

sachanub changed the title ~~Merge base inference toolkit scripts and unit tests~~ Migrate base inference toolkit scripts and unit tests Oct 16, 2023

Remove methods from environment.py

c97a606

sachanub requested review from nikhil-sk and chen3933 October 17, 2023 20:42

chen3933 approved these changes Oct 18, 2023

View reviewed changes

Merge branch 'master' into merge_base_toolkit

5693eaf

namannandan approved these changes Oct 19, 2023

View reviewed changes

nikhil-sk approved these changes Oct 19, 2023

View reviewed changes

sachanub merged commit 1e8e043 into aws:master Oct 19, 2023

sachanub deleted the merge_base_toolkit branch October 20, 2023 18:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Migrate base inference toolkit scripts and unit tests #157

Migrate base inference toolkit scripts and unit tests #157

Uh oh!

sachanub commented Oct 16, 2023 •

edited

Loading

Uh oh!

namannandan left a comment

Uh oh!

namannandan Oct 18, 2023

Uh oh!

sachanub Oct 19, 2023

Uh oh!

namannandan Oct 18, 2023

Uh oh!

sachanub Oct 19, 2023

Uh oh!

namannandan Oct 18, 2023

Uh oh!

namannandan Oct 18, 2023

Uh oh!

namannandan Oct 18, 2023

Uh oh!

Uh oh!

	RUN pip uninstall -y sagemaker_inference sagemaker_pytorch_inference && \
	RUN pip uninstall -y sagemaker-inference sagemaker_pytorch_inference && \

Migrate base inference toolkit scripts and unit tests #157

Migrate base inference toolkit scripts and unit tests #157

Uh oh!

Conversation

sachanub commented Oct 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

namannandan left a comment

Choose a reason for hiding this comment

Uh oh!

namannandan Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

sachanub Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

namannandan Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

sachanub Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

namannandan Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

namannandan Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

namannandan Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sachanub commented Oct 16, 2023 •

edited

Loading