Skip to content

fix: ensure generated names are < 63 characters when deploying compiled models #1645

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

Merged
merged 1 commit into from
Jun 29, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 4 additions & 2 deletions src/sagemaker/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -477,8 +477,10 @@ def deploy(

compiled_model_suffix = "-".join(instance_type.split(".")[:-1])
if self._is_compiled_model:
name_prefix = self.name or utils.name_from_image(self.image)
self.name = "{}{}".format(name_prefix, compiled_model_suffix)
name_prefix = self.name or utils.name_from_image(
self.image, max_length=(62 - len(compiled_model_suffix))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be 63 - len(compiled_model_suffix) ?

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 added a dash to join the prefix and the suffix, so that's why I took one off from the 63 limit

)
self.name = "{}-{}".format(name_prefix, compiled_model_suffix)

self._create_sagemaker_model(instance_type, accelerator_type, tags)
production_variant = sagemaker.production_variant(
Expand Down
11 changes: 6 additions & 5 deletions src/sagemaker/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,17 +42,18 @@


# Use the base name of the image as the job name if the user doesn't give us one
def name_from_image(image):
def name_from_image(image, max_length=63):
"""Create a training job name based on the image name and a timestamp.

Args:
image (str): Image name.

Returns:
str: Training job name using the algorithm from the image name and a
timestamp.
timestamp.
max_length (int): Maximum length for the resulting string (default: 63).
"""
return name_from_base(base_name_from_image(image))
return name_from_base(base_name_from_image(image), max_length=max_length)


def name_from_base(base, max_length=63, short=False):
Expand All @@ -64,8 +65,8 @@ def name_from_base(base, max_length=63, short=False):

Args:
base (str): String used as prefix to generate the unique name.
max_length (int): Maximum length for the resulting string.
short (bool): Whether or not to use a truncated timestamp.
max_length (int): Maximum length for the resulting string (default: 63).
short (bool): Whether or not to use a truncated timestamp (default: False).

Returns:
str: Input parameter with appended timestamp.
Expand Down
13 changes: 13 additions & 0 deletions tests/unit/sagemaker/model/test_neo.py
Original file line number Diff line number Diff line change
Expand Up @@ -210,3 +210,16 @@ def test_check_neo_region(sagemaker_session):
for partition in boto_session.get_available_partitions():
for region_name in boto_session.get_available_regions("ec2", partition_name=partition):
assert (region_name in NEO_REGION_LIST) is model.check_neo_region(region_name)


def test_deploy_valid_model_name(sagemaker_session):
model = Model(
image="long-base-name-that-is-over-the-63-character-limit-for-model-names",
model_data=MODEL_DATA,
role="role",
sagemaker_session=sagemaker_session,
)
model._is_compiled_model = True

model.deploy(1, "ml.c4.xlarge")
assert len(model.name) <= 63
11 changes: 11 additions & 0 deletions tests/unit/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,17 @@ def test_bad_import():
pd.DataFrame()


@patch("sagemaker.utils.name_from_base")
@patch("sagemaker.utils.base_name_from_image")
def test_name_from_image(base_name_from_image, name_from_base):
image = "image:latest"
max_length = 32

sagemaker.utils.name_from_image(image, max_length=max_length)
base_name_from_image.assert_called_with(image)
name_from_base.assert_called_with(base_name_from_image.return_value, max_length=max_length)


@patch("sagemaker.utils.sagemaker_timestamp")
def test_name_from_base(sagemaker_timestamp):
sagemaker.utils.name_from_base(NAME, short=False)
Expand Down