-
Notifications
You must be signed in to change notification settings - Fork 72
Add new unit and integration tests #155
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@@ -0,0 +1,39 @@ | |||
# Copyright 2019-2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably want to cover is_env_set
as well : https://github.com/aws/sagemaker-pytorch-inference-toolkit/blob/master/src/sagemaker_pytorch_serving_container/ts_environment.py#L58-L64
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added test for is_env_set()
in test_ts_environment.py
.
I notice we don't have local integration test. Is there any use case we want to cover in local integration test? |
I didn't see user script overwrites |
ee0b329
to
25f594d
Compare
Added the following local integration tests:
|
We define custom |
48d5d98
to
a424fc9
Compare
device = torch.device("cuda:" + str(context.system_properties.get("gpu_id"))) | ||
device_str = str(device)[-1] | ||
with open(file_path, "a") as file: | ||
file.write(device_str + "\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add both pid-thread-id and device_id. With the identifier and we can verify device_id are consistent when calling input_fn etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acknowledged. Fixed in latest commit.
input_fn_device_info, output_fn_device_info, predict_fn_device_info | ||
): | ||
|
||
device_id_input_fn, pid_input_fn, threadid_input_fn = input_fn_row |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot guarantee the sequence of input_fn_device_info.csv
, output_fn_device_info.csv
and predict_fn_device_info.csv
are the same, right?
Eg:
input_fn_device_info.csv
pid=0 device_id=0
pid=1 device_id=1
output_fn_device_info.csv
pid=1 device_id=1
pid=0 device_id=0
in above, the behavior is correct but test will fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we want to check device_id consistency across all four functions, namely model_fn
, input_fn
, output_fn
and predict_fn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acknowledged. Fixed in latest commit.
dfc96bc
to
79e49d2
Compare
test/integration/__init__.py
Outdated
|
||
traced_resnet18_model_dir = os.path.join(mme_path, traced_resnet18_sub_dir) | ||
traced_resnet18_script = os.path.join(traced_resnet18_model_dir, code_sub_dir, "inference.py") | ||
traced_resnet18_tar = file_utils.make_tarfile( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: it seems that we're repeating the process of making the tarfile for each model. Can we loop through this process to make the file cleaner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acknowledged. Updated in latest commit.
def container(image_uri, use_gpu): | ||
try: | ||
gpu_option = "--gpus device=0" if use_gpu else "" | ||
resnet18_path = os.path.join(mme_path, 'resnet18') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fixture is using resnet18_path as default - would be good to make the model path name generic or parametrize it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acknowledged. Updated in latest commit.
try: | ||
import transformers | ||
except ImportError: | ||
raise ImportError("The 'transformers' module was not found.'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why this explicit try-catch is necessary here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acknowledged. Removed try-catch block. If transformers
is not present, the error will show up anyways. Added comment above import
statement to explain the reason for importing transformers
i.e. to check that dependencies in requirements.txt
get installed.
17eb1fd
to
1aa7032
Compare
bbd5c0e
to
d0b94e3
Compare
Issue #, if available:
Description of changes:
The objective of this PR is to add new unit and integration tests. Specifically, we include the following:
ts_environment.py
:test/unit/test_ts_environment.py
test/integration/sagemaker/test_mnist_inductor.py
test/resources/mnist/model_inductor
:code/mnist.py
andtorch_model.pth
test/integration/__init__.py
:test/integration/sagemaker/test_mnist_inductor.py
:test/integration/sagemaker/test_multi_model_endpoint_sagemaker.py
test/resources/mme
:test/resources/resnet18/default_model
.test/resources/resnet18/default_traced_resnet
.test/integration/__init__.py
:test/integration/sagemaker/test_multi_model_endpoint_sagemaker.py
:requirements.txt
are installed:requirements.txt
file: This file has thetransformers
dependency.test/resources/mnist/model_cpu/code/requirements.txt
.transformers
in the following script:test/resources/mnist/model_cpu/code/mnist.py
.test/integration/__init__.py
:test/utils/file_utils.py
:requirements.txt
in tar file creation.UPDATE:
test/integration/__init__.py
to obtain model information fromtest/integration/all_models_info.json
and iteratively create attributes for the model directory, model script name and model tar file.Added the following local integration tests:
test/integration/sagemaker/test_multi_model_endpoint_local.py
:test/integration/__init__.py
:model_gpu_context
.test/resources/model_gpu_context/code/inference.py
:model_fn
,input_fn
,predict_fn
andoutput_fn
to dynamically get all the GPU IDs from thecontext
object and their corresponding PIDs and thread IDs and write them in different CSV files. Return dummy values from the functions.test/integration/local/test_model_fn_context.py
:context
object in themodel_fn
,input_fn
,predict_fn
andoutput_fn
functions by reading from the CSV files created intest/resources/model_gpu_context/code/inference.py
.model_fn
,input_fn
,predict_fn
,output_fn
functions.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.