Skip to content

fix: adjacent slash in s3 key #3427

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.

Sign up for GitHub

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
Oct 25, 2022
Merged

fix: adjacent slash in s3 key #3427

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 2 additions & 2 deletions src/sagemaker/model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -569,8 +569,8 @@ def _upload_code(self, key_prefix: str, repack: bool = False) -> None:
)
return
self.sagemaker_session.context.need_runtime_repack.add(id(self))
self.sagemaker_session.context.runtime_repack_output_prefix = "s3://{}/{}".format(
bucket, key_prefix
self.sagemaker_session.context.runtime_repack_output_prefix = s3.s3_path_join(
"s3://", bucket, key_prefix
)
# Add the uploaded_code and repacked_model_data to update the container env
self.repacked_model_data = self.model_data
Expand Down
4 changes: 2 additions & 2 deletions src/sagemaker/tensorflow/model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,8 +396,8 @@ def prepare_container_def(
# model is not yet there, defer repacking to later during pipeline execution
if isinstance(self.sagemaker_session, PipelineSession):
self.sagemaker_session.context.need_runtime_repack.add(id(self))
self.sagemaker_session.context.runtime_repack_output_prefix = "s3://{}/{}".format(
bucket, key_prefix
self.sagemaker_session.context.runtime_repack_output_prefix = s3.s3_path_join(
"s3://", bucket, key_prefix
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems the s3.s3_path_join will remove the "/" in the end of the path and return "s3://my-bucket/my-key-prefix".

But given the customer input in #3413

Removing the slash from the code location “works” (as in does not create an error message), but then I have no separation between the pipeline prefix and the artifact name, e.g. pipeline-codepytorchinference ...

Should we also check the estimator side on how it generates the entire code output path with the code_location?

Or if this is already checked in the unit test that a separation is correctly added between the prefix and the artifact name, please let me know. Then we're good

Copy link
Contributor

Choose a reason for hiding this comment

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

Plus, if we decide to update the model side rather than the estimator side, as this only updates the tensorflow model, same changes are needed in the model class as well:

Copy link
Member Author

@nmadan nmadan Oct 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Or if this is already checked in the unit test that a separation is correctly added between the prefix and the artifact name, please let me know. Then we're good

yep, I verified in the unit test that for input code_location f's3://my-bucket/pipeline-code/' and model-name pytorchinference the s3 path will be s3://bucket/pipeline-code/pytorchinference/.... Note that key_prefix in the code above is actually pipeline-code//pytorchinference. Basically this code change will convert the // to /

same changes are needed in the model class as well

Yes, forgot to commit that class adding it now

Copy link
Contributor

@qidewenwhen qidewenwhen Oct 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

After a second thought, how about we directly update the joins in _stage_user_code_in_s3 in estimator.py (see here)? The reasons are:

  • even if we update the model side to remove the "/" at the end of code_location, if user supplies ...code_location/ directly to the code_location to an estimator, the code_location//some-key would still persist in estimator code output. Though it may not cause any errors but it may not work as customer's expectation.
  • If update the estimator side, we can just update in one place and no need to update the model sides.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am assuming that by "artifact" the user means "model-name"

Copy link
Contributor

Choose a reason for hiding this comment

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

how about we directly update the joins in _stage_user_code_in_s3 in estimator.py

Synced up offline, this may break backward compatibility. Given that we don't see any customer complains on the estimator's code location output path, we can just simply update the model side

else:
logging.warning(
Expand Down
11 changes: 7 additions & 4 deletions tests/unit/sagemaker/workflow/test_model_step.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@
_TENSORFLOW_PATH = os.path.join(DATA_DIR, "tfs/tfs-test-entrypoint-and-dependencies")
_REPACK_OUTPUT_KEY_PREFIX = "code-output"
_MODEL_CODE_LOCATION = f"s3://{_BUCKET}/{_REPACK_OUTPUT_KEY_PREFIX}"
_MODEL_CODE_LOCATION_TRAILING_SLASH = _MODEL_CODE_LOCATION + "/"


@pytest.fixture
Expand Down Expand Up @@ -690,7 +691,7 @@ def test_conditional_model_create_and_regis(
entry_point=f"{DATA_DIR}/{_SCRIPT_NAME}",
role=_ROLE,
enable_network_isolation=True,
code_location=_MODEL_CODE_LOCATION,
code_location=_MODEL_CODE_LOCATION_TRAILING_SLASH,
),
2,
),
Expand All @@ -714,7 +715,7 @@ def test_conditional_model_create_and_regis(
entry_point=f"{DATA_DIR}/{_SCRIPT_NAME}",
role=_ROLE,
framework_version="1.5.0",
code_location=_MODEL_CODE_LOCATION,
code_location=_MODEL_CODE_LOCATION_TRAILING_SLASH,
),
2,
),
Expand Down Expand Up @@ -746,7 +747,7 @@ def test_conditional_model_create_and_regis(
image_uri=_IMAGE_URI,
entry_point=f"{DATA_DIR}/{_SCRIPT_NAME}",
role=_ROLE,
code_location=_MODEL_CODE_LOCATION,
code_location=_MODEL_CODE_LOCATION_TRAILING_SLASH,
),
2,
),
Expand All @@ -769,7 +770,9 @@ def assert_test_result(steps: list):
assert len(steps) == expected_step_num
if expected_step_num == 2:
assert steps[0]["Type"] == "Training"
if model.key_prefix == _REPACK_OUTPUT_KEY_PREFIX:
if model.key_prefix is not None and model.key_prefix.startswith(
_REPACK_OUTPUT_KEY_PREFIX
):
assert steps[0]["Arguments"]["OutputDataConfig"]["S3OutputPath"] == (
f"{_MODEL_CODE_LOCATION}/{model.name}"
)
Expand Down