-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: make instance type fields as optional #3135
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
Changes from 1 commit
1470afb
b57ad62
effd3ee
6dcfd45
2c4fa5d
54a8463
bf054de
e5b551b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4202,8 +4202,8 @@ def _intercept_create_request( | |
def get_model_package_args( | ||
content_types, | ||
response_types, | ||
inference_instances, | ||
transform_instances, | ||
inference_instances=None, | ||
transform_instances=None, | ||
model_package_name=None, | ||
model_package_group_name=None, | ||
model_data=None, | ||
|
@@ -4225,9 +4225,9 @@ def get_model_package_args( | |
content_types (list): The supported MIME types for the input data. | ||
response_types (list): The supported MIME types for the output data. | ||
inference_instances (list): A list of the instance types that are used to | ||
generate inferences in real-time. | ||
generate inferences in real-time (default: None). | ||
transform_instances (list): A list of the instance types on which a transformation | ||
job can be run or on which an endpoint can be deployed. | ||
job can be run or on which an endpoint can be deployed (default: None). | ||
model_package_name (str): Model Package name, exclusive to `model_package_group_name`, | ||
using `model_package_name` makes the Model Package un-versioned (default: None). | ||
model_package_group_name (str): Model Package Group name, exclusive to | ||
|
@@ -4363,9 +4363,9 @@ def get_create_model_package_request( | |
if validation_specification: | ||
request_dict["ValidationSpecification"] = validation_specification | ||
if containers is not None: | ||
if not all([content_types, response_types, inference_instances, transform_instances]): | ||
if not all([content_types, response_types]): | ||
raise ValueError( | ||
"content_types, response_types, inference_inferences and transform_instances " | ||
"content_types and response_types " | ||
"must be provided if containers is present." | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This thrown error was added by sreedes@. Did you check with her to see if she agree on this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussed the same with Saumitra and got approval for this change |
||
inference_specification = { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -122,3 +122,39 @@ def test_pipeline_session_context_for_model_step(pipeline_session_mock): | |
assert not register_step_args.create_model_request | ||
assert register_step_args.create_model_package_request | ||
assert len(register_step_args.need_runtime_repack) == 0 | ||
|
||
|
||
def test_pipeline_session_context_for_model_step_without_instance_types(pipeline_session_mock): | ||
model = Model( | ||
name="MyModel", | ||
image_uri="fakeimage", | ||
model_data=ParameterString(name="ModelData", default_value="s3://my-bucket/file"), | ||
sagemaker_session=pipeline_session_mock, | ||
entry_point=f"{DATA_DIR}/dummy_script.py", | ||
source_dir=f"{DATA_DIR}", | ||
role=_ROLE, | ||
) | ||
# CreateModelStep requires runtime repack | ||
create_step_args = model.create( | ||
instance_type="c4.4xlarge", | ||
accelerator_type="ml.eia1.medium", | ||
) | ||
# The context should be cleaned up before return | ||
assert pipeline_session_mock.context is None | ||
assert create_step_args.create_model_request | ||
assert not create_step_args.create_model_package_request | ||
assert len(create_step_args.need_runtime_repack) == 1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: seems that you only update the model.register() and this test should focus on your changes - "without instance_types". Thus I guess we don't need to test the model.create case here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed. |
||
|
||
# _RegisterModelStep does not require runtime repack | ||
model.entry_point = None | ||
model.source_dir = None | ||
register_step_args = model.register( | ||
content_types=["text/csv"], | ||
response_types=["text/csv"], | ||
model_package_group_name="MyModelPackageGroup", | ||
) | ||
# The context should be cleaned up before return | ||
assert not pipeline_session_mock.context | ||
assert not register_step_args.create_model_request | ||
assert register_step_args.create_model_package_request | ||
assert len(register_step_args.need_runtime_repack) == 0 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: same above, these validations should focus on your changes, which is how removing instance_type impacting the register_step_args.create_model_package_request Please remove the validations of 157, 158 and 160. And elaborate a little bit on line 159 if possible There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed as per request |
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.
Can you also update the
instance_type
argument inpipeline_container_def
and make its default value is None?https://github.com/aws/sagemaker-python-sdk/blob/master/src/sagemaker/pipeline.py#L87
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.
Done.