fix: double Run create on load_run #3821

danabens · 2023-04-27T19:46:42Z

Issue #, if available: #3673

Description of changes: Fixes a bug in load_run which would result in a second Run created with a similar name

Testing done:

tox -e py39 -- tests/unit/sagemaker/experiments

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.

sagemaker-bot · 2023-04-27T19:54:21Z

AWS CodeBuild CI Report

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

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

qidewenwhen · 2023-04-27T20:14:42Z

src/sagemaker/experiments/run.py

@@ -843,7 +844,6 @@ def list_runs(
    created_after: Optional[datetime.datetime] = None,
    sagemaker_session: Optional["Session"] = None,
    max_results: Optional[int] = None,
-    next_token: Optional[str] = None,


To remove a field from a public method can be a breaking change. Is it causing any issues so we need to remove it?

Or are we sure the list_runs is not widely used? If so it's fine to remove it

the argument is unusable as it currently is, so no one is currently using it

+1 on @qidewenwhen 's comment.

Can we keep this parameter in public method but instead doing nothing with it?

ill separate the next_token change from the bug fix and we can discuss what to do with it separately

qidewenwhen · 2023-04-27T20:19:59Z

src/sagemaker/experiments/run.py

        sort_by (SortByType): The property to sort results by. One of NAME, CREATION_TIME
            (default: CREATION_TIME).
        sort_order (SortOrderType): One of ASCENDING, or DESCENDING (default: DESCENDING).

    Returns:
        list: A list of ``Run`` objects.
    """
+
+    # all trial components retrieved by default


You mean the _TrialComponent.list can retrieve all trial components?

sagemaker-python-sdk/src/sagemaker/experiments/trial_component.py

Lines 248 to 262 in 8d2c16b

return super(_TrialComponent, cls)._list(

"list_trial_components",

_api_types.TrialComponentSummary.from_boto,

"TrialComponentSummaries",

source_arn=source_arn,

created_before=created_before,

created_after=created_after,

sort_by=sort_by,

sort_order=sort_order,

sagemaker_session=sagemaker_session,

trial_name=trial_name,

experiment_name=experiment_name,

max_results=max_results,

next_token=next_token,

)

Or the list trial component API can retrieve all TCs in one call?

If the next_token is not causing any issues, I'd suggest to keep it to avoid a breaking change

all trial components are retrieved by default:

https://github.com/aws/sagemaker-python-sdk/blob/master/src/sagemaker/apiutils/_base_types.py#L118-L147

The problem with this parameter is where is the customer going to get the next_token to supply to method in the first place? The only way to get next_token is to call ListTrialComponents in which case they have no reason to use this method.

Also this method doesn't return a NextToken.

I prefer to remove since it is not providing useful behavior as it currently is. In the future we could re-add it but will need to return a NextToken so the customer can supply it in their own page-loop behavior.

qidewenwhen · 2023-04-27T20:32:07Z

src/sagemaker/experiments/run.py

+        # TODO: we should revert the lower casting once backend fix reaches prod
+        return trial_component_name.replace(
+            "{}{}".format(experiment_name.lower(), DELIMITER), "", 1
+        )


Good catch! So I assume this line would fix the issue of second run created? And will fix the list_runs issue #3673?

Can we add a unit test which does list_runs twice in case to validate in case any future changes break it again?

sure, will do

sagemaker-bot · 2023-04-27T21:15:50Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-04-27T21:20:21Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-04-27T21:30:05Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-04-27T21:30:28Z

AWS CodeBuild CI Report

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

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

codecov-commenter · 2023-04-27T21:33:26Z

Codecov Report

Merging #3821 (883d028) into master (4844aa1) will decrease coverage by 0.72%.
The diff coverage is 100.00%.

❗ Current head 883d028 differs from pull request most recent head 5a9cd8c. Consider uploading reports for the commit 5a9cd8c to get more accurate results

@@            Coverage Diff             @@
##           master    #3821      +/-   ##
==========================================
- Coverage   89.71%   88.99%   -0.72%     
==========================================
  Files        1060      247     -813     
  Lines      101652    25134   -76518     
==========================================
- Hits        91193    22368   -68825     
+ Misses      10459     2766    -7693

Impacted Files	Coverage Δ
src/sagemaker/experiments/run.py	`99.14% <100.00%> (ø)`

... and 1306 files with indirect coverage changes

sagemaker-bot · 2023-04-27T21:35:44Z

AWS CodeBuild CI Report

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

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

danabens · 2023-04-27T22:53:22Z

Updates:

remove removal of next_token and created backlog item
updated list_runs test to include a mixed case experiment name argument to cover root cause of issue

sagemaker-bot · 2023-04-27T22:56:51Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-04-27T23:46:58Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-04-27T23:47:18Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-04-27T23:47:59Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-04-28T01:45:26Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-04-28T17:58:54Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-04-28T18:12:35Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-04-28T18:18:49Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-04-28T18:26:29Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-04-28T18:34:56Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-04T17:36:11Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-04T17:38:45Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-04T17:41:20Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-04T17:52:54Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-04T19:47:01Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-04T21:58:45Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-04T22:04:32Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-04T22:16:22Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-04T22:19:58Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-04T22:29:37Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-pr
Commit ID: 883d028
Result: FAILED
Build Logs (available for 30 days)

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

sagemaker-bot · 2023-05-09T02:44:16Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-09T03:37:06Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-09T03:40:28Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-09T03:43:06Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-09T03:52:34Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-09T05:47:42Z

AWS CodeBuild CI Report

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

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

fixes aws#3673

sagemaker-bot · 2023-05-16T16:02:50Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-16T16:02:53Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-16T16:02:58Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-16T16:03:54Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-05-16T17:12:04Z

AWS CodeBuild CI Report

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

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

danabens requested a review from a team as a code owner April 27, 2023 19:46

danabens requested review from trajanikant and removed request for a team April 27, 2023 19:46

danabens assigned qidewenwhen Apr 27, 2023

danabens force-pushed the sdk-double-fix branch from 45a3717 to fb34f93 Compare April 27, 2023 19:49

danabens mentioned this pull request Apr 27, 2023

list_runs doesn't work twice. Raises a ValueError about the name length of the run. #3673

Closed

danabens force-pushed the sdk-double-fix branch from fb34f93 to 8694237 Compare April 27, 2023 20:24

qidewenwhen reviewed Apr 27, 2023

View reviewed changes

danabens force-pushed the sdk-double-fix branch from 8694237 to 7d7bf2c Compare April 27, 2023 22:52

leoroy approved these changes Apr 28, 2023

View reviewed changes

danabens force-pushed the sdk-double-fix branch 2 times, most recently from 0e55555 to ed38720 Compare April 28, 2023 17:21

trajanikant force-pushed the sdk-double-fix branch from e2a0307 to 883d028 Compare May 4, 2023 21:08

danabens force-pushed the sdk-double-fix branch from 883d028 to 5efbbd7 Compare May 9, 2023 02:39

danabens force-pushed the sdk-double-fix branch from 5efbbd7 to 5a9cd8c Compare May 9, 2023 02:46

mufaddal-rohawala assigned trajanikant May 9, 2023

knikure linked an issue May 9, 2023 that may be closed by this pull request

list_runs doesn't work twice. Raises a ValueError about the name length of the run. #3673

Closed

leoroy approved these changes May 9, 2023

View reviewed changes

fix: double Run create on load_run

0626b0a

fixes aws#3673

trajanikant force-pushed the sdk-double-fix branch from 5a9cd8c to 0626b0a Compare May 16, 2023 16:01

trajanikant merged commit e4c9bfc into aws:master May 16, 2023

	return super(_TrialComponent, cls)._list(
	"list_trial_components",
	_api_types.TrialComponentSummary.from_boto,
	"TrialComponentSummaries",
	source_arn=source_arn,
	created_before=created_before,
	created_after=created_after,
	sort_by=sort_by,
	sort_order=sort_order,
	sagemaker_session=sagemaker_session,
	trial_name=trial_name,
	experiment_name=experiment_name,
	max_results=max_results,
	next_token=next_token,
	)

fix: double Run create on load_run #3821

fix: double Run create on load_run #3821

Conversation

danabens commented Apr 27, 2023 • edited Loading

Merge Checklist

General

Tests

sagemaker-bot commented Apr 27, 2023

AWS CodeBuild CI Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagemaker-bot commented Apr 27, 2023

AWS CodeBuild CI Report

sagemaker-bot commented Apr 27, 2023

AWS CodeBuild CI Report

sagemaker-bot commented Apr 27, 2023

AWS CodeBuild CI Report

sagemaker-bot commented Apr 27, 2023

AWS CodeBuild CI Report

codecov-commenter commented Apr 27, 2023 • edited Loading

Codecov Report

sagemaker-bot commented Apr 27, 2023

AWS CodeBuild CI Report

danabens commented Apr 27, 2023

sagemaker-bot commented Apr 27, 2023

AWS CodeBuild CI Report

sagemaker-bot commented Apr 27, 2023

AWS CodeBuild CI Report

sagemaker-bot commented Apr 27, 2023

AWS CodeBuild CI Report

sagemaker-bot commented Apr 27, 2023

AWS CodeBuild CI Report

sagemaker-bot commented Apr 28, 2023

AWS CodeBuild CI Report

sagemaker-bot commented Apr 28, 2023

AWS CodeBuild CI Report

sagemaker-bot commented Apr 28, 2023

AWS CodeBuild CI Report

sagemaker-bot commented Apr 28, 2023

AWS CodeBuild CI Report

sagemaker-bot commented Apr 28, 2023

AWS CodeBuild CI Report

sagemaker-bot commented Apr 28, 2023

AWS CodeBuild CI Report

sagemaker-bot commented May 4, 2023

AWS CodeBuild CI Report

sagemaker-bot commented May 4, 2023

AWS CodeBuild CI Report

sagemaker-bot commented May 4, 2023

AWS CodeBuild CI Report

sagemaker-bot commented May 4, 2023

AWS CodeBuild CI Report

sagemaker-bot commented May 4, 2023

AWS CodeBuild CI Report

sagemaker-bot commented May 4, 2023

AWS CodeBuild CI Report

sagemaker-bot commented May 4, 2023

AWS CodeBuild CI Report

sagemaker-bot commented May 4, 2023

AWS CodeBuild CI Report

sagemaker-bot commented May 4, 2023

AWS CodeBuild CI Report

sagemaker-bot commented May 4, 2023

AWS CodeBuild CI Report

sagemaker-bot commented May 9, 2023

AWS CodeBuild CI Report

sagemaker-bot commented May 9, 2023

AWS CodeBuild CI Report

sagemaker-bot commented May 9, 2023

AWS CodeBuild CI Report

sagemaker-bot commented May 9, 2023

AWS CodeBuild CI Report

sagemaker-bot commented May 9, 2023

danabens commented Apr 27, 2023 •

edited

Loading

codecov-commenter commented Apr 27, 2023 •

edited

Loading