-
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 7 commits
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 |
---|---|---|
|
@@ -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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed the same with Saumitra and got approval for this change