Skip to content

change: use recommended inference image URI from Neo API #2806

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

Closed
wants to merge 42 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
dde8d00
fix: Set ProcessingStep upload locations deterministically to avoid c…
staubhp Dec 8, 2021
0f72907
fix: Prevent repack_model script from referencing nonexistent directo…
staubhp Dec 9, 2021
0bae071
fix: S3Input - add support for instance attributes (#2754)
mufaddal-rohawala Dec 15, 2021
17fe93e
fix: typos and broken link (#2765)
mohamed-ali Dec 16, 2021
0a8ca6a
fix: Model Registration with BYO scripts (#2797)
sreedes Dec 17, 2021
08c3a7b
change: use recommended inference image uri from Neo API
HappyAmazonian Dec 20, 2021
1d5a5dc
Merge branch 'master' into inference-url
shreyapandit Dec 21, 2021
edbf61f
change InferenceImage lookup in compilation job description API respo…
HappyAmazonian Dec 27, 2021
d28b89f
Merge branch 'inference-url' of https://github.com/HappyAmazonian/sag…
HappyAmazonian Dec 27, 2021
185b4b0
fix: Add ContentType in test_auto_ml_describe
navinns Dec 27, 2021
5ec2ff4
fix: Re-deploy static integ test endpoint if it is not found
Dec 27, 2021
cec05a5
Merge branch 'dev' into inference-url
mufaddal-rohawala Dec 30, 2021
1312575
documentation :SageMaker model parallel library 1.6.0 API doc (#2814)
mchoi8739 Dec 30, 2021
6319bce
fix: fix kmeans test deletion sequence, increment lineage statics (#2…
mufaddal-rohawala Dec 31, 2021
f728e11
Merge branch 'dev' into inference-url
mufaddal-rohawala Dec 31, 2021
beeabbd
fix: Increment static lineage pipeline (#2817)
mufaddal-rohawala Jan 3, 2022
005ded3
Merge branch 'dev' into inference-url
mufaddal-rohawala Jan 3, 2022
81b77c8
fix: Model Registration with BYO scripts (#2797)
sreedes Dec 17, 2021
c8ca3b7
fix: Add ContentType in test_auto_ml_describe
navinns Dec 27, 2021
7104337
fix: Re-deploy static integ test endpoint if it is not found
Dec 27, 2021
ad29a0c
documentation :SageMaker model parallel library 1.6.0 API doc (#2814)
mchoi8739 Dec 30, 2021
bbe6284
fix: fix kmeans test deletion sequence, increment lineage statics (#2…
mufaddal-rohawala Dec 31, 2021
b026769
fix: Increment static lineage pipeline (#2817)
mufaddal-rohawala Jan 3, 2022
439d085
Merge branch 'dev' into inference-url
mufaddal-rohawala Jan 4, 2022
de9ad6b
improve unit tests
HappyAmazonian Jan 4, 2022
6f98534
Merge branch 'inference-url' of https://github.com/HappyAmazonian/sag…
HappyAmazonian Jan 4, 2022
87c1d2c
fix: Fix lineage query integ tests (#2823)
staubhp Jan 5, 2022
b31dd14
Merge branch 'dev' into inference-url
shreyapandit Jan 6, 2022
8210375
fix: fixes unnecessary session call while generating pipeline definit…
xchen909 Jan 10, 2022
972a6d2
feature: Add models_v2 under lineage context (#2800)
yzhu0 Jan 10, 2022
7206b9e
feature: enable python 3.9 (#2802)
mufaddal-rohawala Jan 10, 2022
127c964
change: Update CHANGELOG.md (#2842)
shreyapandit Jan 11, 2022
554d735
fix: update pricing link (#2805)
ahsan-z-khan Jan 11, 2022
431e853
Merge branch 'dev' into inference-url
HappyAmazonian Jan 11, 2022
81af309
feature: Adds support for Serverless inference (#2831)
bhaoz Jan 14, 2022
a949e5f
feature: Add support for SageMaker lineage queries in action (#2853)
yzhu0 Jan 14, 2022
888153b
feature: Adds Lineage queries in artifact, context and trial componen…
yzhu0 Jan 18, 2022
b796a00
feature: Add EMRStep support in Sagemaker pipeline (#2848)
EthanShouhanCheng Jan 18, 2022
9d25fcf
feature: Add support for SageMaker lineage queries context (#2830)
yzhu0 Jan 19, 2022
64e4690
fix: support specifying a facet by its column index
xgchena Oct 30, 2021
814dd3d
doc: more documentation for serverless inference (#2859)
bhaoz Jan 20, 2022
13aa858
Merge branch 'dev' into inference-url
HappyAmazonian Jan 21, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions src/sagemaker/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -658,12 +658,7 @@ def compile(
if target_instance_family == "ml_eia2":
pass
elif target_instance_family.startswith("ml_"):
self.image_uri = self._compilation_image_uri(
self.sagemaker_session.boto_region_name,
target_instance_family,
framework,
framework_version,
)
self.image_uri = job_status["InferenceImage"]
self._is_compiled_model = True
else:
LOGGER.warning(
Expand Down
31 changes: 4 additions & 27 deletions tests/unit/sagemaker/model/test_neo.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
DESCRIBE_COMPILATION_JOB_RESPONSE = {
"CompilationJobStatus": "Completed",
"ModelArtifacts": {"S3ModelArtifacts": "s3://output-path/model.tar.gz"},
"InferenceImage": "inference-container-uri",
}


Expand All @@ -52,12 +53,7 @@ def test_compile_model_for_inferentia(sagemaker_session):
framework_version="1.15.0",
job_name="compile-model",
)
assert (
"{}.dkr.ecr.{}.amazonaws.com/sagemaker-neo-tensorflow:1.15.0-inf-py3".format(
NEO_REGION_ACCOUNT, REGION
)
== model.image_uri
)
assert DESCRIBE_COMPILATION_JOB_RESPONSE["InferenceImage"] == model.image_uri
assert model._is_compiled_model is True


Expand Down Expand Up @@ -286,7 +282,7 @@ def test_compile_with_framework_version_15(session):
job_name="compile-model",
)

assert "1.5" in model.image_uri
assert model.image_uri is not None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any specific reason we are not checking "1.5" here in image uri

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the uri is fetched from Neo API now, I think it would be suffice to just check that
model.image_uri is equal to the uri inside the response.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should still check for 1.5 to make sure the uri we are receiving is as expected

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that is possible. Checking 1.5 requires API access within the unit tests. I changed it to make sure the model.image_uri is the same as what is inside the response.



@patch("sagemaker.session.Session")
Expand All @@ -304,26 +300,7 @@ def test_compile_with_framework_version_16(session):
job_name="compile-model",
)

assert "1.6" in model.image_uri


@patch("sagemaker.session.Session")
def test_compile_validates_framework_version(session):
session.return_value.boto_region_name = REGION

model = _create_model()
with pytest.raises(ValueError) as e:
model.compile(
target_instance_family="ml_c4",
input_shape={"data": [1, 3, 1024, 1024]},
output_path="s3://output",
role="role",
framework="pytorch",
framework_version="1.6.1",
job_name="compile-model",
)

assert "Unsupported neo-pytorch version: 1.6.1." in str(e)
Comment on lines -310 to -326
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any specific reason we are removing negative test case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the previous reply.
Since the uri is fetched from Neo API now, the API response will put inference uri as None if there is not a proper inference image.

assert model.image_uri is not None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here



@patch("sagemaker.session.Session")
Expand Down
15 changes: 7 additions & 8 deletions tests/unit/test_mxnet.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@

ENV_INPUT = {"env_key1": "env_val1", "env_key2": "env_val2", "env_key3": "env_val3"}

INFERENCE_IMAGE_URI = "inference-uri"


@pytest.fixture()
def sagemaker_session():
Expand All @@ -83,7 +85,10 @@ def sagemaker_session():
)

describe = {"ModelArtifacts": {"S3ModelArtifacts": "s3://m/m.tar.gz"}}
describe_compilation = {"ModelArtifacts": {"S3ModelArtifacts": "s3://m/model_c5.tar.gz"}}
describe_compilation = {
"ModelArtifacts": {"S3ModelArtifacts": "s3://m/model_c5.tar.gz"},
"InferenceImage": INFERENCE_IMAGE_URI,
}
session.sagemaker_client.create_model_package.side_effect = MODEL_PKG_RESPONSE
session.sagemaker_client.describe_training_job = Mock(return_value=describe)
session.sagemaker_client.describe_endpoint = Mock(return_value=ENDPOINT_DESC)
Expand Down Expand Up @@ -195,12 +200,6 @@ def _create_compilation_job(input_shape, output_location):
}


def _neo_inference_image(mxnet_version):
return "301217895009.dkr.ecr.us-west-2.amazonaws.com/sagemaker-inference-{}:{}-cpu-py3".format(
FRAMEWORK.lower(), mxnet_version
)


@patch("sagemaker.estimator.name_from_base")
@patch("sagemaker.utils.create_tar_file", MagicMock())
def test_create_model(
Expand Down Expand Up @@ -422,7 +421,7 @@ def test_mxnet_neo(time, strftime, sagemaker_session, neo_mxnet_version):
actual_compile_model_args = sagemaker_session.method_calls[3][2]
assert expected_compile_model_args == actual_compile_model_args

assert compiled_model.image_uri == _neo_inference_image(neo_mxnet_version)
assert compiled_model.image_uri == INFERENCE_IMAGE_URI

predictor = mx.deploy(1, CPU, use_compiled_model=True)
assert isinstance(predictor, MXNetPredictor)
Expand Down