-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add telemetry metrics on usage of default images for ModelBuilder #4430
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,8 @@ | |
from sagemaker import Session, exceptions | ||
from sagemaker.serve.mode.function_pointers import Mode | ||
from sagemaker.serve.utils.exceptions import ModelBuilderException | ||
from sagemaker.serve.utils.types import ModelServer | ||
from sagemaker.serve.utils.types import ModelServer, ImageUriOption | ||
from sagemaker.serve.validations.check_image_uri import is_1p_image_uri | ||
from sagemaker.user_agent import SDK_VERSION | ||
|
||
logger = logging.getLogger(__name__) | ||
|
@@ -62,11 +63,13 @@ def wrapper(self, *args, **kwargs): | |
caught_ex = None | ||
|
||
image_uri_tail = self.image_uri.split("/")[1] | ||
image_uri_option = _get_image_uri_option(self.image_uri, self._is_custom_image_uri) | ||
extra = ( | ||
f"{func_name}" | ||
f"&x-modelServer={MODEL_SERVER_TO_CODE[str(self.model_server)]}" | ||
f"&x-imageTag={image_uri_tail}" | ||
f"&x-sdkVersion={SDK_VERSION}" | ||
f"&x-defaultImageUsage={image_uri_option}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. curious, is "default" important to the naming here or can we just go with "imageUsage" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or "imageType" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think so, this metric is to capture whether customers provide their own image or not, the further differentiation on custom image or custom 1p image is not that important. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ack sound good. posed the question from a string len perspective. 1024 is max i believe. So if there are ways to cut down on length without losing the meaningfulness we should try and do so |
||
) | ||
|
||
if self.model_server == ModelServer.DJL_SERVING or self.model_server == ModelServer.TGI: | ||
|
@@ -201,3 +204,22 @@ def _get_region_or_default(session): | |
return session.boto_session.region_name | ||
except Exception: # pylint: disable=W0703 | ||
return "us-west-2" | ||
|
||
|
||
def _get_image_uri_option(image_uri: str, is_custom_image: bool) -> int: | ||
"""Detect whether default values are used for ModelBuilder | ||
|
||
Args: | ||
image_uri (str): Image uri used by ModelBuilder. | ||
is_custom_image: (bool): Boolean indicating whether customer provides with custom image. | ||
Returns: | ||
bool: Integer code of image option types. | ||
""" | ||
|
||
if not is_custom_image: | ||
return ImageUriOption.DEFAULT_IMAGE.value | ||
|
||
if is_1p_image_uri(image_uri): | ||
return ImageUriOption.CUSTOM_1P_IMAGE.value | ||
|
||
return ImageUriOption.CUSTOM_IMAGE.value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the added VPC config for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's for feature parity. This config has already exposed to djl/tgi/torchserve/tri