chore: telemetry for deployment configs #4806

evakravi · 2024-07-26T13:26:41Z

Issue #, if available:

Description of changes:
No telemetry (user agent metadata) was being recorded for deployment configuration information for JumpStart models. This adds recording of deployment configuration information.

Testing done:
added tests

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.

Captainia · 2024-07-26T20:39:50Z

src/sagemaker/jumpstart/factory/estimator.py

@@ -903,20 +911,16 @@ def _add_config_name_to_kwargs(
 ) -> JumpStartEstimatorInitKwargs:
    """Sets tags in kwargs based on default or override, returns full kwargs."""

-    specs = verify_model_region_and_return_specs(
+    kwargs.config_name = kwargs.config_name or get_top_ranked_config_name(


Thanks for refactoring this!

Captainia · 2024-07-26T20:52:06Z

src/sagemaker/jumpstart/factory/model.py

-    )
+    # we need to create a default JS session (without custom user agent)
+    # in order to retrieve config name info
+    temp_session = kwargs.sagemaker_session or DEFAULT_JUMPSTART_SAGEMAKER_SESSION


Not sure if I follow this, if kwargs.sagemaker_session is None, don't we fallback to the default session here?

yes, but the problem is we need a session in order to derive what the config name is.

For the session to get the config name, we use the DEFAULT_JUMPSTART_SAGEMAKER_SESSION without any special user agent, unless a custom session is provided

whittech1 · 2024-07-29T12:52:12Z

src/sagemaker/jumpstart/exceptions.py

@@ -150,6 +150,7 @@ def __init__(
                model. (Default: None).

        """
+        version = version or "*"


sanity check: this would change the behavior on line 157 right? Is that intentional?

This was intentional since version was being supplied as None, not being omitted.
I'm going to try removing this tho

whittech1 · 2024-07-29T12:54:35Z

src/sagemaker/jumpstart/utils.py

    return headers


+def get_top_ranked_config_name(


non-blocking: are we assuming overall ranking in this method? Wouldn't we need to be provided which ranking array to use?

That's an excellent point. How about I make it an optional argument, the ranking name, with the default value being overall?

JGuinegagne · 2024-07-31T15:57:01Z

src/sagemaker/jumpstart/factory/estimator.py

        scope=JumpStartScriptScope.TRAINING,
-        region=kwargs.region,
-        tolerate_vulnerable_model=kwargs.tolerate_vulnerable_model,
+        model_type=kwargs.model_type[0],


sanity-check: any risk of getting an IndexError here?

Actually, i did this cause the typing said it was a tuple, but looking at other places of the code, we do kwargs.model_type.

@malav-shastri -- any idea what to do here?

JGuinegagne · 2024-07-31T15:57:49Z

src/sagemaker/jumpstart/factory/model.py

    kwargs: Union[JumpStartModelInitKwargs, JumpStartModelDeployKwargs]
 ) -> JumpStartModelInitKwargs:
    """Sets session in kwargs based on default or override, returns full kwargs."""

    kwargs.sagemaker_session = (
        kwargs.sagemaker_session
        or get_default_jumpstart_session_with_user_agent_suffix(
-            kwargs.model_id, kwargs.model_version, kwargs.hub_arn
+            kwargs.model_id, kwargs.model_version, kwargs.config_name, kwargs.hub_arn


consider using named args here to avoid the risk of mixing them up.

JGuinegagne · 2024-07-31T16:00:11Z

src/sagemaker/jumpstart/factory/model.py

+        return kwargs
+    supported_instance_types = resolved_config.get("supported_inference_instance_types", [])
+    if kwargs.instance_type not in supported_instance_types:
+        JUMPSTART_LOGGER.warning("Overriding instance type to %s", kwargs.instance_type)


nit (I know it isn't from this PR): could we use f-string please, also update to Overriding instance type to %s even though it is not in the list of supported instances.

i think we did this cause there's some linters that complain

JGuinegagne · 2024-07-31T16:03:05Z

src/sagemaker/jumpstart/utils.py

    is_hub_content: Optional[bool] = False,
 ) -> Session:
    """Returns default JumpStart SageMaker Session with model-specific user agent suffix."""
    botocore_session = botocore.session.get_session()
    botocore_config = botocore.config.Config(
        user_agent_extra=get_jumpstart_user_agent_extra_suffix(
-            model_id, model_version, is_hub_content
+            model_id, model_version, config_name, is_hub_content


consider using named args here.

JGuinegagne · 2024-07-31T16:04:02Z

tests/integ/sagemaker/jumpstart/model/test_jumpstart_model.py

+    session = model.sagemaker_session
+
+    with mock.patch("botocore.client.BaseClient._make_request") as mock_make_request:
+        try:


Just for my understanding, what is this try/except ?

basically im trying to intercept the network call to make sure the user agent has the right tags applied

still, I'm not sure why you need a try / except, particularly since you are mocking this.

Consider either removing or adding an inline comment to explain that.

…ment-configs

JGuinegagne · 2024-08-05T16:53:27Z

tests/integ/sagemaker/jumpstart/model/test_jumpstart_model.py

+    session = model.sagemaker_session
+
+    with mock.patch("botocore.client.BaseClient._make_request") as mock_make_request:
+        try:


still, I'm not sure why you need a try / except, particularly since you are mocking this.

Consider either removing or adding an inline comment to explain that.

chore: telemetry for deployment configs

78874b5

evakravi requested a review from a team as a code owner July 26, 2024 13:26

evakravi requested a review from mufaddal-rohawala July 26, 2024 13:26

evakravi temporarily deployed to auto-approve July 26, 2024 13:26 — with GitHub Actions Inactive

chore: minor fixes

49a1c82

evakravi temporarily deployed to auto-approve July 26, 2024 13:30 — with GitHub Actions Inactive

chore: address minor issues

3fc60f2

evakravi temporarily deployed to auto-approve July 26, 2024 13:40 — with GitHub Actions Inactive

fix: flake8

df711e3

evakravi temporarily deployed to auto-approve July 26, 2024 14:10 — with GitHub Actions Inactive

fix: model type for estimator

8f886fe

evakravi temporarily deployed to auto-approve July 26, 2024 15:28 — with GitHub Actions Inactive

evakravi temporarily deployed to auto-approve July 26, 2024 18:07 — with GitHub Actions Inactive

Merge branch 'master' into chore/telemetry-for-deployment-configs

086b920

evakravi temporarily deployed to auto-approve July 26, 2024 20:33 — with GitHub Actions Inactive

Captainia reviewed Jul 26, 2024

View reviewed changes

Merge branch 'master' into chore/telemetry-for-deployment-configs

35b85c2

evakravi temporarily deployed to auto-approve July 27, 2024 21:55 — with GitHub Actions Inactive

whittech1 reviewed Jul 29, 2024

View reviewed changes

Merge branch 'master' into chore/telemetry-for-deployment-configs

5150ef2

evakravi temporarily deployed to auto-approve July 30, 2024 13:11 — with GitHub Actions Inactive

chore: add ranking name argument to get_top_ranked_config_name

e4fa7b0

evakravi temporarily deployed to auto-approve July 30, 2024 14:14 — with GitHub Actions Inactive

Merge branch 'master' into chore/telemetry-for-deployment-configs

e34936b

evakravi temporarily deployed to auto-approve July 31, 2024 15:47 — with GitHub Actions Inactive

Captainia approved these changes Jul 31, 2024

View reviewed changes

JGuinegagne reviewed Jul 31, 2024

View reviewed changes

chore: use named args

12f8f95

evakravi temporarily deployed to auto-approve July 31, 2024 16:27 — with GitHub Actions Inactive

fix: remove tuple from model type

3534b79

evakravi temporarily deployed to auto-approve August 5, 2024 13:28 — with GitHub Actions Inactive

Merge remote-tracking branch 'origin' into chore/telemetry-for-deploy…

9d635a4

…ment-configs

evakravi temporarily deployed to auto-approve August 5, 2024 13:51 — with GitHub Actions Inactive

Merge branch 'master' into chore/telemetry-for-deployment-configs

3852722

evakravi temporarily deployed to auto-approve August 5, 2024 16:46 — with GitHub Actions Inactive

JGuinegagne approved these changes Aug 5, 2024

View reviewed changes

JGuinegagne previously approved these changes Aug 5, 2024

View reviewed changes

chore: add comment explaining test

fdaa7fd

evakravi dismissed JGuinegagne’s stale review via fdaa7fd August 5, 2024 18:11

evakravi temporarily deployed to auto-approve August 5, 2024 18:11 — with GitHub Actions Inactive

Merge branch 'master' into chore/telemetry-for-deployment-configs

f490e46

evakravi temporarily deployed to auto-approve August 5, 2024 18:15 — with GitHub Actions Inactive

sage-maker approved these changes Aug 5, 2024

View reviewed changes

sage-maker merged commit ba7e6a9 into aws:master Aug 5, 2024
14 checks passed

chore: telemetry for deployment configs #4806

chore: telemetry for deployment configs #4806

Uh oh!

Conversation

evakravi commented Jul 26, 2024

Merge Checklist

General

Tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!