-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: jumpstart model id suggestions #2899
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
598562b
c51d9bd
b591959
2e017c4
3c5ea3a
9d84e2e
cede5fa
a765512
5150498
dded640
5370f76
b0a761b
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 |
---|---|---|
|
@@ -13,6 +13,7 @@ | |
"""This module defines the JumpStartModelsCache class.""" | ||
from __future__ import absolute_import | ||
import datetime | ||
from difflib import get_close_matches | ||
from typing import List, Optional | ||
import json | ||
import boto3 | ||
|
@@ -204,14 +205,34 @@ def _get_manifest_key_from_model_id_semantic_version( | |
sm_version_to_use = sm_version_to_use_list[0] | ||
|
||
error_msg = ( | ||
f"Unable to find model manifest for {model_id} with version {version} " | ||
f"compatible with your SageMaker version ({sm_version}). " | ||
f"Unable to find model manifest for '{model_id}' with version '{version}' " | ||
f"compatible with your SageMaker version ('{sm_version}'). " | ||
f"Consider upgrading your SageMaker library to at least version " | ||
f"{sm_version_to_use} so you can use version " | ||
f"{model_version_to_use_incompatible_with_sagemaker} of {model_id}." | ||
f"'{sm_version_to_use}' so you can use version " | ||
f"'{model_version_to_use_incompatible_with_sagemaker}' of '{model_id}'." | ||
) | ||
raise KeyError(error_msg) | ||
error_msg = f"Unable to find model manifest for {model_id} with version {version}." | ||
|
||
error_msg = f"Unable to find model manifest for '{model_id}' with version '{version}'. " | ||
error_msg += ( | ||
"Visit https://sagemaker.readthedocs.io/en/stable/doc_utils/jumpstart.html" | ||
" for updated list of models. " | ||
) | ||
|
||
other_model_id_version = self._select_version( | ||
"*", versions_incompatible_with_sagemaker | ||
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. can you please add a comment as to why you use this variable. 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. reiterate: please explain why we use a variable called |
||
) # all versions here are incompatible with sagemaker | ||
if other_model_id_version is not None: | ||
error_msg += ( | ||
f"Consider using model ID '{model_id}' with version " | ||
f"'{other_model_id_version}'." | ||
) | ||
|
||
else: | ||
possible_model_ids = [header.model_id for header in manifest.values()] | ||
closest_model_id = get_close_matches(model_id, possible_model_ids, n=1, cutoff=0)[0] | ||
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. question: should you show only one? or say for example: at least 1, up to 3 when score > xx ? 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. Let's just do 1 and make it simple. If we want to make this really good, we're better off just having a separate utility for searching model ids. 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. another question, can you get an If that's possible, could you handle this edge case and add a unit test for it? 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. No, because the cutoff is 0, there will always be a match. 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. Thanks. I know this is super edge case, but could there be a case where 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. Only if the manifest is empty |
||
error_msg += f"Did you mean to use model ID '{closest_model_id}'?" | ||
|
||
raise KeyError(error_msg) | ||
|
||
def _get_file_from_s3( | ||
|
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 find!