-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: attach jumpstart estimator for gated model #4546
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: attach jumpstart estimator for gated model #4546
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4546 +/- ##
=======================================
Coverage 87.38% 87.38%
=======================================
Files 389 389
Lines 36776 36780 +4
=======================================
+ Hits 32135 32139 +4
Misses 4641 4641 ☔ View full report in Codecov by Sentry. |
| region=sagemaker_session.boto_region_name, | ||
| scope=JumpStartScriptScope.TRAINING, | ||
| tolerate_deprecated_model=True, # model is already trained, so tolerate if deprecated | ||
| tolerate_vulnerable_model=True, # model is already trained, so tolerate if vulnerable |
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: if a model is vulnerable, would this code raise an exception alerting the user:
my_estimator = estimator.attach()
my_model = my_estimator.deploy()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.
Nevertheless, it seems that you are only retrieving the specs to check if the model is gated or not, so that is non-blocking. Consider updating the comments to provide more details however.
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.
If the model was actually vulnerable or deprecated, you would get an exception when doing my_estimator.deploy(). As you mention, these tolerate flags are just to get the status of whether the model is gated or not
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue #, if available:
Description of changes:
Attaching a
JumpStartEstimatorfor a gated model results in the exception:ValueError: Need to define 'accept_eula'='true' within Environment. Model 'huggingface-llm-gemma-2b-instruct' requires accepting end-user license agreement (EULA). See https://jumpstart-cache-prod-us-west-2.s3.us-west-2.amazonaws.com/fmhMetadata/terms/gemmaTerms.txt for terms of use.This issue is now fixed so that an exception is not thrown.
Testing done:
Unit, integ tests added
Manual test
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.