feat: Enable customizing artifact output path #3965

rd-pong · 2023-06-28T17:23:37Z

Issue #, if available:
#3609

Description of changes:

Allow customers to define the bucket and the prefix where the output artifacts would be stored.
Add artifact_bucket and artifact_prefix to Run constructor
Add artifact_bucket and artifact_prefix to load_run

Testing done:

Existing unit tests pass
New unit test added to cover this case
- tests/unit/sagemaker/experiments/test_run.py
- tests/unit/sagemaker/experiments/helpers.py

Merge Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.

General

I have read the CONTRIBUTING doc
I certify that the changes I am introducing will be backward compatible, and I have discussed concerns about this, if any, with the Python SDK team
I used the commit message format described in CONTRIBUTING
I have passed the region in to all S3 and STS clients that I've initialized as part of this change.
I have updated any necessary documentation, including READMEs and API docs (if appropriate)

Tests

I have added tests that prove my fix is effective or that my feature works (if appropriate)
I have added unit and/or integration tests as appropriate to ensure backward compatibility of the changes
I have checked that my tests are not configured for a specific region or account (if appropriate)
I have used unique_name_from_base to create resource names in integ tests (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

knikure

/bot run all

sagemaker-bot · 2023-06-29T16:57:49Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-unit-tests
Commit ID: 9801ffc
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2023-06-29T17:33:50Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-notebook-tests
Commit ID: 9801ffc
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2023-06-29T17:34:59Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-pr
Commit ID: 9801ffc
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2023-06-29T17:40:47Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-local-mode-tests
Commit ID: 9801ffc
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2023-06-29T17:41:46Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-local-mode-tests
Commit ID: 9801ffc
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2023-06-29T21:53:05Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-slow-tests
Commit ID: 9801ffc
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

danabens · 2023-06-30T02:20:03Z

src/sagemaker/experiments/run.py

@@ -29,6 +29,7 @@
 from sagemaker.experiments._helper import (
    _ArtifactUploader,
    _LineageArtifactTracker,
+    _DEFAULT_ARTIFACT_PREFIX,


nit the name of this sounds like it will prefix the actual name of the artifact but its really a prefix for a s3 path where the associated files will be stored

Yes, it's a bit confusing to use this name here. But _DEFAULT_ARTIFACT_PREFIX is not introduced in this PR, it's already defined in _helper.py. There is a variable called artifact_prefix, so I'm assuming that is why we called the default variable to be _DEFAULT_ARTIFACT_PREFIX.

A workaround could be change the following method defined in _ArtifactUploader.

class _ArtifactUploader(object): """Artifact uploader""" def __init__( self, trial_component_name, sagemaker_session, artifact_bucket=None, artifact_prefix=None, # HERE!!! ): self.sagemaker_session = sagemaker_session self.trial_component_name = trial_component_name self.artifact_bucket = artifact_bucket self.artifact_prefix = ( _DEFAULT_ARTIFACT_PREFIX if artifact_prefix is None else artifact_prefix # HERE!!! ) self._s3_client = self.sagemaker_session.boto_session.client("s3")

In this way, we don't need to import _DEFAULT_ARTIFACT_PREFIX to run.py

danabens · 2023-06-30T02:20:58Z

src/sagemaker/experiments/run.py

+            artifact_prefix=_DEFAULT_ARTIFACT_PREFIX
+            if artifact_prefix is None
+            else artifact_prefix,


nit this indentation is unintuitive

nm looks like there is not many better alternatives https://stackoverflow.com/questions/28897010/how-to-make-a-line-break-on-the-python-ternary-operator

knikure

/bot run all

sagemaker-bot · 2023-06-30T19:03:30Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-notebook-tests
Commit ID: 3eeba30
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2023-06-30T19:05:13Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-pr
Commit ID: 3eeba30
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2023-06-30T19:13:48Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-local-mode-tests
Commit ID: 3eeba30
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2023-06-30T19:24:58Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-slow-tests
Commit ID: 3eeba30
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2023-06-30T20:01:53Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-unit-tests
Commit ID: 3eeba30
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

codecov-commenter · 2023-06-30T20:03:33Z

Codecov Report

Merging #3965 (3eeba30) into master (a68faf1) will decrease coverage by 0.73%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3965      +/-   ##
==========================================
- Coverage   90.32%   89.59%   -0.73%     
==========================================
  Files        1292      305     -987     
  Lines      113972    28188   -85784     
==========================================
- Hits       102940    25256   -77684     
+ Misses      11032     2932    -8100

Impacted Files	Coverage Δ
src/sagemaker/experiments/run.py	`99.14% <ø> (ø)`
src/sagemaker/jumpstart/estimator.py	`98.36% <100.00%> (ø)`
src/sagemaker/jumpstart/model.py	`97.82% <100.00%> (ø)`
src/sagemaker/session.py	`79.32% <100.00%> (ø)`

... and 1593 files with indirect coverage changes

feature: enable customizing artifact output path

cf9fd65

rd-pong requested a review from a team as a code owner June 28, 2023 17:23

rd-pong requested review from knikure and removed request for a team June 28, 2023 17:23

Merge branch 'master' into customize-artifact-path

9801ffc

knikure self-assigned this Jun 29, 2023

knikure linked an issue Jun 29, 2023 that may be closed by this pull request

Customizing the artifact output path. #3609

Closed

knikure reviewed Jun 29, 2023

View reviewed changes

Format code with black

472b920

jiahaoli97 approved these changes Jun 29, 2023

View reviewed changes

danabens reviewed Jun 30, 2023

View reviewed changes

danabens approved these changes Jun 30, 2023

View reviewed changes

Merge branch 'master' into customize-artifact-path

3eeba30

knikure reviewed Jun 30, 2023

View reviewed changes

knikure approved these changes Jun 30, 2023

View reviewed changes

knikure merged commit 521a255 into aws:master Jun 30, 2023

maltelorbach mentioned this pull request Jul 25, 2023

Support for setting artifact bucket and prefix via PipelineExperimentConfig #4022

Closed

feat: Enable customizing artifact output path #3965

feat: Enable customizing artifact output path #3965

Uh oh!

Conversation

rd-pong commented Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Checklist

General

Tests

Uh oh!

knikure left a comment

Choose a reason for hiding this comment

Uh oh!

sagemaker-bot commented Jun 29, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 29, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 29, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 29, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 29, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 29, 2023

AWS CodeBuild CI Report

Uh oh!

danabens Jun 30, 2023

Choose a reason for hiding this comment

Uh oh!

rd-pong Jun 30, 2023

Choose a reason for hiding this comment

Uh oh!

danabens Jun 30, 2023

Choose a reason for hiding this comment

Uh oh!

danabens Jun 30, 2023

Choose a reason for hiding this comment

Uh oh!

knikure left a comment

Choose a reason for hiding this comment

Uh oh!

sagemaker-bot commented Jun 30, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 30, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 30, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 30, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 30, 2023

AWS CodeBuild CI Report

Uh oh!

codecov-commenter commented Jun 30, 2023

Codecov Report

Uh oh!

Uh oh!

rd-pong commented Jun 28, 2023 •

edited

Loading