Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
4 changes: 4 additions & 0 deletions src/sagemaker/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -745,6 +745,8 @@ def is_repack(self) -> bool:
Returns:
bool: if the source need to be repacked or not
"""
if self.source_dir is None or self.entry_point is None:
return False
return self.source_dir and self.entry_point and not self.git_config
Comment on lines +748 to 750
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this should already be covered in the existing line . Can we run the unit test without this change to verify ?

Can we update the line to be something like return self.source_dir is not None and self.entry_point is not None and self.git_config is None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in python None and None and True/False will return None. return self.source_dir is not None and self.entry_point is not None and self.git_config is None can not cover the case of empty string


def _upload_code(self, key_prefix: str, repack: bool = False) -> None:
Expand Down Expand Up @@ -2143,6 +2145,8 @@ def is_repack(self) -> bool:
Returns:
bool: if the source need to be repacked or not
"""
if self.source_dir is None or self.entry_point is None:
return False
return self.source_dir and self.entry_point and not (self.key_prefix or self.git_config)


Expand Down
15 changes: 15 additions & 0 deletions tests/unit/sagemaker/model/test_framework_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -511,6 +511,21 @@ def test_is_repack_with_code_location(repack_model, sagemaker_session):
assert not model.is_repack()


@patch("sagemaker.utils.repack_model")
def test_is_repack_with_none_type(repack_model, sagemaker_session):
"""Test is_repack() returns a boolean value when source_dir and entry_point are None"""

model = FrameworkModel(
role=ROLE,
sagemaker_session=sagemaker_session,
source_dir="s3://codebucket/someprefix/sourcedir.tar.gz",
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be changed to None ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

image_uri=IMAGE_URI,
model_data=MODEL_DATA,
)

assert model.is_repack() is False


@patch("sagemaker.git_utils.git_clone_repo")
@patch("sagemaker.model.fw_utils.tar_and_upload_dir")
def test_is_repack_with_git_config(tar_and_upload_dir, git_clone_repo, sagemaker_session):
Expand Down
15 changes: 15 additions & 0 deletions tests/unit/sagemaker/model/test_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -1046,6 +1046,21 @@ def test_is_repack_with_code_location(repack_model, sagemaker_session):
assert model.is_repack()


@patch("sagemaker.utils.repack_model")
def test_is_repack_with_none_type(repack_model, sagemaker_session):
"""Test is_repack() returns a boolean value when source_dir and entry_point are None"""

model = Model(
role=ROLE,
sagemaker_session=sagemaker_session,
source_dir=SCRIPT_URI,
image_uri=IMAGE_URI,
model_data=MODEL_DATA,
)

assert model.is_repack() is False


@patch("sagemaker.git_utils.git_clone_repo")
@patch("sagemaker.model.fw_utils.tar_and_upload_dir")
def test_is_repack_with_git_config(tar_and_upload_dir, git_clone_repo, sagemaker_session):
Expand Down
Loading