-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: minor jumpstart dev ex improvements #4279
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
fix: minor jumpstart dev ex improvements #4279
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
…rializablePayload
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
Nice PR!
A quick question, I may have gotten confused by the wording of Ensure logging wildcard warning happens a single time for gated models
. IIUC this means we want to log only once? If that's the case, could you point me to the change that enables a single log? Thanks!
if ( | ||
getattr(self, "model_id", None) in {"", None} | ||
and instance_type | ||
and instance_type.startswith("ml.inf") |
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.
nit: Consider a const
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.
can you create a separate utility for this + unit test it please?
src/sagemaker/model.py
Outdated
@@ -635,7 +648,7 @@ def prepare_container_def( | |||
self.repacked_model_data or self.model_data, | |||
deploy_env, | |||
image_config=self.image_config, | |||
accept_eula=getattr(self, "accept_eula", None), | |||
accept_eula=accept_eula or getattr(self, "accept_eula", None), |
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.
question: Why not adhere the changes to create()
to the current pattern outlined in deploy()? I'd recommend we stick to either passing in the value or through using getattr
, but not both. wdyt?
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.
imo we should pass it in and modify deploy()
to match but would definitely want to have the group weigh in on this
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 is possible to call prepare_container_def
outside of deploy()
, so we need to add this support.
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.
question: what happens if accept_eula
is False
? Does it use the attribute?
Could you add a unit test for this case please?
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.
Yeah, totally agree @evakravi. Doesn't have to be this PR but can we put a backlog item to refactor deploy()
to match and remove getattr
& self.accept_eula
?
instance_type=None, | ||
accelerator_type=None, | ||
serverless_inference_config=None, | ||
accept_eula=None, |
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.
Another plus to adhering to getattr
is now you won't need to make every modification here
@@ -115,80 +115,6 @@ def _construct_payload( | |||
return payload_to_use | |||
|
|||
|
|||
def _extract_generated_text_from_response( |
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.
nit: Could you specify why you are removing this in the PR description?
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.
sorry, will do
@@ -371,7 +370,6 @@ def from_json(self, json_obj: Optional[Dict[str, Any]]) -> None: | |||
self.content_type = json_obj["content_type"] | |||
self.body = json_obj["body"] | |||
accept = json_obj.get("accept") | |||
self.generated_text_response_key = json_obj.get("generated_text_response_key") |
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.
nit: same as above
@@ -109,7 +109,8 @@ def get_jumpstart_gated_content_bucket( | |||
accessors.JumpStartModelsAccessor.set_jumpstart_gated_content_bucket(gated_bucket_to_return) | |||
|
|||
if gated_bucket_to_return != old_gated_content_bucket: | |||
accessors.JumpStartModelsAccessor.reset_cache() | |||
if old_gated_content_bucket is not None: |
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.
nit: This seems to be copied from get_jumpstart_content_bucket(). Can we pass in these bucket names in and merge these two functions?
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.
for now, let's keep as is. i agree, there is code duplication that should be removed.
btw, this change is for ensuring the wildcard logging happens a single time. basically, what is happening is that when a gated model is initialized, the cache got reset, causing the wildcard warning to get emitted twice.
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.
Ah I see, that makes a lot of sense. Nice catch!
@mock.patch("sagemaker.model.LOGGER.warning") | ||
@mock.patch("sagemaker.utils.sagemaker_timestamp") | ||
@mock.patch("sagemaker.session.Session.endpoint_from_production_variants") | ||
@mock.patch("sagemaker.session.Session.create_model") |
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.
nit: Can we mock these functions from the mock_session
patch?
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.
this is a different mocked session object we need to include. It's messy i know lol
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 |
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.
Please review with tech writer and create additional utilities.
Also, you are removing an internal function, could you confirm no other team is using it?
"Please try targeting a higher version of the model or using a " | ||
"different model." | ||
"Please try targeting a higher version of the model, upgrading " | ||
"SageMaker Python SDK version or using a different model." |
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.
could you check with @judyheflin please? I think they generally don't like using Please
.
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.
Recommendation:
"Version '{version}' of JumpStart model '{model_id}' is deprecated. We recommend that you specify a more recent model version or choose a different model. To access the latest models and model versions, be sure to upgrade to the latest version of the SageMaker Python SDK."
src/sagemaker/model.py
Outdated
@@ -635,7 +648,7 @@ def prepare_container_def( | |||
self.repacked_model_data or self.model_data, | |||
deploy_env, | |||
image_config=self.image_config, | |||
accept_eula=getattr(self, "accept_eula", None), | |||
accept_eula=accept_eula or getattr(self, "accept_eula", None), |
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.
question: what happens if accept_eula
is False
? Does it use the attribute?
Could you add a unit test for this case please?
if ( | ||
getattr(self, "model_id", None) in {"", None} | ||
and instance_type | ||
and instance_type.startswith("ml.inf") |
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.
can you create a separate utility for this + unit test it please?
INSTANCE_TYPE, | ||
accelerator_type=accelerator_type, | ||
serverless_inference_config=None, | ||
accept_eula=True, |
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.
add a unit test for False
case please
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.
question: what happens if accept_eula is False ? Does it use the attribute?
-- Good catch, I will fix this and add the test
can you create a separate utility for this + unit test it please?
-- separate utility for what exactly? there already is a unit test that the log does not get emitted for JS models
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 #4279 +/- ##
==========================================
- Coverage 86.72% 86.57% -0.15%
==========================================
Files 1197 380 -817
Lines 105633 34810 -70823
==========================================
- Hits 91609 30137 -61472
+ Misses 14024 4673 -9351 ☔ 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 |
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 |
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 |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue #, if available:
Description of changes:
sagemaker.model.Model.create()
support foraccept_eula
argumentgenerated_text_response_key
, since it is not being used in favor of another metadata fieldnotebook_utils
(and ability to disable all JS logging)info
logging messageTesting done:
Unit tests added and passing
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
Tests
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.