Add telemetry metrics on usage of default images for ModelBuilder #4430
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| @@ -57,6 +57,8 @@ def __init__(self): | |||
| self.mode = None | |||
| self.model_server = None | |||
| self.image_uri = None | |||
| self._is_custom_image_uri = False | |||
| self.vpc_config = None | |||
There was a problem hiding this comment.
What's the added VPC config for?
There was a problem hiding this comment.
It's for feature parity. This config has already exposed to djl/tgi/torchserve/tri
| @@ -67,6 +68,7 @@ def wrapper(self, *args, **kwargs): | |||
| 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={_get_image_uri_option(self.image_uri, self._is_custom_image_uri)}" | |||
There was a problem hiding this comment.
Need to be careful with the substitution here, there was an issue previously with enums not being converted to string properly. See here for example: https://github.com/aws/sagemaker-python-sdk/blob/master/src/sagemaker/serve/utils/telemetry_logger.py#L38-L51.
There was a problem hiding this comment.
I don't think that's an issue here. See
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
return value is marked as enum.value
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #4430 +/- ##
==========================================
- Coverage 86.82% 86.81% -0.02%
==========================================
Files 386 386
Lines 35710 35731 +21
==========================================
+ Hits 31006 31020 +14
- Misses 4704 4711 +7 ☔ View full report in Codecov by Sentry. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| 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.
curious, is "default" important to the naming here or can we just go with "imageUsage"
There was a problem hiding this comment.
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.
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
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
…s#4430) * Initial commit * Fix formatting and add uts * try to fix ut failure * fix missing attribute in telemetry ut * fix docstyle * fix pylint
…s#4430) * Initial commit * Fix formatting and add uts * try to fix ut failure * fix missing attribute in telemetry ut * fix docstyle * fix pylint
…s#4430) * Initial commit * Fix formatting and add uts * try to fix ut failure * fix missing attribute in telemetry ut * fix docstyle * fix pylint
Issue #, if available:
Description of changes:
Testing done:
Merge Checklist
Put an
xin 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
Tests
unique_name_from_baseto 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.