Skip to content

Conversation

@zhaoqizqwang
Copy link
Collaborator

@zhaoqizqwang zhaoqizqwang commented Dec 17, 2025

Issue #, if available:

Bring selected recent commits in master-v2 branch to master

Description of changes:

See commit message for change details

Tested by running updated unit tests. Only AI registry unit tests are failing in sagemaker-train (which is known):

sagemaker-serve has 2 failures which seemed flaky recently. Verified and they could pass locally:

image image

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

For commit: aws/sagemaker-python-sdk-staging@99210b2

Tested by running sagemaker-serve unit tests
aviruthen
aviruthen previously approved these changes Dec 17, 2025
Copy link
Collaborator

@aviruthen aviruthen left a comment

Choose a reason for hiding this comment

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

Approved pending unit/integ tests passing

NextToken=next_token,
MaxResults=max_results,
)
return self.sagemaker_session.sagemaker_client.list_pipeline_versions(**kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

Todo: we should move this to sagemaker core? can you note this down

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added # TODO: replace with sagemaker-core methods before this line

@@ -1,1030 +0,0 @@
# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
Copy link
Member

Choose a reason for hiding this comment

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

Can you confirm nothing is being missed from this file to the duplicate file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes this is a duplicate file and ModelTrainer in this file is not referenced anywhere. All the tests and recent commits are running against ModelTrainer in the other file

@mufaddal-rohawala
Copy link
Member

About massice image uri changes in this file, are units sufficiently covering any testing gaps? Can you confirm.
Also about deleietion of "image_uri_config_updated"folder, can you confirm no parity gaps w.r.t image_uri_config?

@zhaoqizqwang
Copy link
Collaborator Author

zhaoqizqwang commented Dec 18, 2025

About massice image uri changes in this file, are units sufficiently covering any testing gaps? Can you confirm.

I added 100+ V2 unit tests on image_uri_config so coverage should be good now. The changes were created by replacing image_uri with image_uri_config.json in master-v2 though

Also about deleietion of "image_uri_config_updated"folder, can you confirm no parity gaps w.r.t image_uri_config?

image_uri_config_updated is an unused folder I believe created during development. Every json there is already in image_uri_config

@zhaoqizqwang zhaoqizqwang merged commit 1c7faf0 into aws:master Dec 18, 2025
12 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants