feat: Curated hub improvements #4760

malav-shastri · 2024-06-27T14:53:32Z

Issue #, if available:

Description of changes:

Adding support for private hubs in model attach functionality
Jumpstart PySDK telemetry support
adding hub_content_arn instead of hub_arn for adding the tags for training and inference jobs
Fixes and improvements
- nits from JumpStart CuratedHub Launch #4748

Testing done:

python3.10 -m pytest tests/unit/**/jumpstart/
end to end test, using notebook

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.

…y due to the circular dependancy issue

src/sagemaker/jumpstart/accessors.py

evakravi · 2024-07-09T15:03:24Z

src/sagemaker/jumpstart/hub/hub.py

@@ -68,7 +67,9 @@ def __init__(
        self,
        hub_name: str,
        bucket_name: Optional[str] = None,
-        sagemaker_session: Optional[Session] = DEFAULT_JUMPSTART_SAGEMAKER_SESSION,
+        sagemaker_session: Optional[


i wouldn't set this as a default argument since this function will get invoked whenever the module is imported, which may cause slow latency or errors on some systems. can you set in constructor body instead?

thanks changed it in the new revision

evakravi · 2024-07-09T15:05:25Z

src/sagemaker/jumpstart/utils.py

+
+    if os.getenv(constants.ENV_VARIABLE_DISABLE_JUMPSTART_TELEMETRY, None):
+        headers = sagemaker_python_sdk_headers
+    elif model_id is None and model_version is None:


can we only add the tag md/js_is_hub_content if it is a hub content? we don't want to add unnecessary characters to the user agent, there's a char limit

thanks changed it in the new revision

…s available

AWS-pratab

Looks good over all

AWS-pratab · 2024-07-09T17:29:10Z

src/sagemaker/jumpstart/accessors.py

+                    "Recieved exeption while calling APIs for ContentType Model, \
+                        retrying with ContentType ModelReference: "


Actually it other way around. The code is first attempting using ModelReference and then as Model.

ohh yeah I forgot I changed it recently, let me correct this. Thanks

also identified one more place where we were trying with Model first and then ModelRef. Changed that as well, in the Hub class. Thanks

JGuinegagne

Careful with backward compatibility

JGuinegagne · 2024-07-09T20:27:17Z

src/sagemaker/jumpstart/accessors.py

@@ -307,6 +299,21 @@ def get_model_specs(
                model_specs.set_hub_content_type(HubContentType.MODEL_REFERENCE)
                return model_specs

+            except Exception as ex:
+                logging.info(
+                    "Recieved exeption while calling APIs for ContentType ModelReference, \


typo: Recieved - please also check error message with @judyheflin

also, high-level question for future PRs: shall we retry on all types of error? As in, if retry throttling as well?

That may be fine, but just want to make sure that's a conscious decision.

yeah I think its fine to retry on all error types but now that I am thinking about it I feel like I can just restrict retry with a different contentType only in the case of ResourceNotFound errors

regardless engaging with @judyheflin for the error message

JGuinegagne · 2024-07-09T20:31:59Z

src/sagemaker/jumpstart/hub/hub.py

            )

        except Exception as ex:
-            logging.info("Recieved expection while calling APIs for ContentType Model: " + str(ex))


JGuinegagne · 2024-07-09T20:32:04Z

src/sagemaker/jumpstart/hub/hub.py

            )

        except Exception as ex:
-            logging.info("Recieved expection while calling APIs for ContentType Model: " + str(ex))
+            logging.info(
+                "Recieved exeption while calling APIs for ContentType ModelReference, retrying with ContentType Model: "


JGuinegagne · 2024-07-09T20:34:01Z

src/sagemaker/jumpstart/hub/utils.py

+    """Returns available Jumpstart hub model version
+
+    Raises:
+        ResourceNotFound: If the specified model is not found in the hub.


the exception type should be a Python class, not the error returned by the API.
Doesn't this method raise or Exception, KeyError or potentially a ClientException on line 203?

yes my bad, updated it to ClientError exception instead

JGuinegagne · 2024-07-09T20:34:52Z

src/sagemaker/jumpstart/model.py

@@ -429,6 +429,7 @@ def attach(
        cls,
        endpoint_name: str,
        inference_component_name: Optional[str] = None,
+        hub_name: Optional[str] = None,


backward-incompatible change, please add arg to the end of the list.

Malav Shastri and others added 4 commits June 24, 2024 17:04

fix: list_models() for python3.8

b1f5cd8

fix linting

6b9f390

fix: Address nits and improvements

cb66608

Merge branch 'aws:master' into curated_hub_improvements

964de22

malav-shastri requested a review from a team as a code owner June 27, 2024 14:53

malav-shastri requested a review from liujiaorr June 27, 2024 14:53

malav-shastri temporarily deployed to auto-approve June 27, 2024 14:53 — with GitHub Actions Inactive

malav-shastri temporarily deployed to auto-approve June 27, 2024 15:21 — with GitHub Actions Inactive

fix codestyle issues

5392504

malav-shastri force-pushed the curated_hub_improvements branch from 6d6345c to 5392504 Compare June 27, 2024 15:25

malav-shastri temporarily deployed to auto-approve June 27, 2024 15:25 — with GitHub Actions Inactive

fix: don't force automatic bucket creation if user don't specify it

269dc08

malav-shastri temporarily deployed to auto-approve June 27, 2024 17:06 — with GitHub Actions Inactive

fix formatting

502063f

malav-shastri temporarily deployed to auto-approve June 27, 2024 17:07 — with GitHub Actions Inactive

fix flake8

f553357

malav-shastri temporarily deployed to auto-approve June 27, 2024 17:33 — with GitHub Actions Inactive

Merge branch 'aws:master' into curated_hub_improvements

7571a55

malav-shastri temporarily deployed to auto-approve June 30, 2024 21:23 — with GitHub Actions Inactive

address nits

5ab02e4

malav-shastri temporarily deployed to auto-approve July 3, 2024 19:29 — with GitHub Actions Inactive

revert HUB_ARN_REGEX and HUB_CONTENT_ARN_REGEX constants from types.p…

37a36c8

…y due to the circular dependancy issue

malav-shastri temporarily deployed to auto-approve July 7, 2024 22:29 — with GitHub Actions Inactive

malav-shastri temporarily deployed to auto-approve July 8, 2024 22:06 — with GitHub Actions Inactive

malav-shastri force-pushed the curated_hub_improvements branch from c1cae63 to 39a22fb Compare July 8, 2024 22:08

malav-shastri temporarily deployed to auto-approve July 8, 2024 22:08 — with GitHub Actions Inactive

revert: don't force automatic bucket creation if user don't specify it

3fe2774

malav-shastri force-pushed the curated_hub_improvements branch from 39a22fb to 3fe2774 Compare July 8, 2024 22:09

malav-shastri temporarily deployed to auto-approve July 8, 2024 22:09 — with GitHub Actions Inactive

fix: fix _add_tags_to_kwargs to use hub_content_arn instead of hub_arn

10dba2c

malav-shastri temporarily deployed to auto-approve July 9, 2024 13:53 — with GitHub Actions Inactive

change default session object in hub class to one with user agent string

c331b0c

malav-shastri temporarily deployed to auto-approve July 9, 2024 13:55 — with GitHub Actions Inactive

fix flake8

ac45eea

malav-shastri temporarily deployed to auto-approve July 9, 2024 14:03 — with GitHub Actions Inactive

evakravi reviewed Jul 9, 2024

View reviewed changes

src/sagemaker/jumpstart/accessors.py Show resolved Hide resolved

evakravi reviewed Jul 9, 2024

View reviewed changes

address comments: moving get default JS session to constructor body

2f07130

malav-shastri temporarily deployed to auto-approve July 9, 2024 18:28 — with GitHub Actions Inactive

Address comments: only add is_hub_content to user aggent suffix if it…

6fb3223

…s available

malav-shastri temporarily deployed to auto-approve July 9, 2024 18:39 — with GitHub Actions Inactive

AWS-pratab reviewed Jul 9, 2024

View reviewed changes

try with ModelReference first then with Model type

0f3f434

malav-shastri temporarily deployed to auto-approve July 9, 2024 18:55 — with GitHub Actions Inactive

fix: describe_model if hub_name has been explicitly provided

65b61a6

malav-shastri temporarily deployed to auto-approve July 9, 2024 19:05 — with GitHub Actions Inactive

JGuinegagne requested changes Jul 9, 2024

View reviewed changes

malav-shastri requested a review from judyheflin July 9, 2024 20:49

Address comments

d8b173d

malav-shastri temporarily deployed to auto-approve July 9, 2024 21:17 — with GitHub Actions Inactive

JGuinegagne previously approved these changes Jul 9, 2024

View reviewed changes

AWS-pratab approved these changes Jul 9, 2024

View reviewed changes

Merge branch 'master' into curated_hub_improvements

ccb640c

malav-shastri dismissed JGuinegagne’s stale review via ccb640c July 10, 2024 15:21

malav-shastri temporarily deployed to auto-approve July 10, 2024 15:22 — with GitHub Actions Inactive

Address merge conflicts

2f5f29b

malav-shastri temporarily deployed to auto-approve July 10, 2024 17:30 — with GitHub Actions Inactive

Aditi2424 approved these changes Jul 10, 2024

View reviewed changes

Aditi2424 merged commit 4c5dd1f into aws:master Jul 10, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Curated hub improvements #4760

feat: Curated hub improvements #4760

malav-shastri commented Jun 27, 2024 •

edited

Loading

evakravi Jul 9, 2024

malav-shastri Jul 9, 2024

evakravi Jul 9, 2024

malav-shastri Jul 9, 2024

AWS-pratab left a comment

AWS-pratab Jul 9, 2024

malav-shastri Jul 9, 2024

malav-shastri Jul 9, 2024

JGuinegagne left a comment

JGuinegagne Jul 9, 2024

JGuinegagne Jul 9, 2024

malav-shastri Jul 9, 2024

malav-shastri Jul 9, 2024

JGuinegagne Jul 9, 2024

JGuinegagne Jul 9, 2024

JGuinegagne Jul 9, 2024

malav-shastri Jul 9, 2024

JGuinegagne Jul 9, 2024

		"Recieved exeption while calling APIs for ContentType Model, \
		retrying with ContentType ModelReference: "

feat: Curated hub improvements #4760

feat: Curated hub improvements #4760

Conversation

malav-shastri commented Jun 27, 2024 • edited Loading

Merge Checklist

General

Tests

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

AWS-pratab left a 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

JGuinegagne left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malav-shastri commented Jun 27, 2024 •

edited

Loading