feat: tagging jumpstart models#2
feat: tagging jumpstart models#2evakravi wants to merge 9 commits intofeat/script_mode_for_model_classfrom
Conversation
evakravi
force-pushed
the
feat/tagging-jumpstart-models
branch
from
0ee6065 to
15f730f
Compare
JGuinegagne
left a comment
JGuinegagne
left a comment
There was a problem hiding this comment.
Few comments:
- tag names
- edge case handling
src/sagemaker/jumpstart/constants.py
Outdated
|
|
||
|
|
||
| class JumpStartTag(str, Enum): | ||
| """Enum class for tags to apply to JumpStart models.""" |
There was a problem hiding this comment.
Is that the tag key? If yes, can you update the docstring to make it clear please?
src/sagemaker/jumpstart/utils.py
Outdated
| inference_script_uri: Optional[str], | ||
| ) -> List[Dict[str, str]]: | ||
| """Adds tags for JumpStart models. Returns original tags for non-JumpStart | ||
| models. |
There was a problem hiding this comment.
nit:
"""Add custom tags to JumpStart models, return the updated tags.
No-op if this is not a JumpStart model related resource.
...
"""| { | ||
| constants.JumpStartTag.INFERENCE_SCRIPT_URI.value: inference_script_uri, | ||
| } | ||
| ) |
There was a problem hiding this comment.
I know this is an edge case, but could you check that these tags do not already exist, and not add them if that's the case? You would get a validation error if you tried to add two tags with the same key.
| JumpStartTag.INFERENCE_MODEL_URI.value: inference_model_uri, | ||
| }, | ||
| {JumpStartTag.INFERENCE_SCRIPT_URI.value: inference_script_uri}, | ||
| ] |
There was a problem hiding this comment.
could you please add a unit test for the edge case where these tag keys are requested by the customer?
src/sagemaker/jumpstart/constants.py
Outdated
| """Enum class for tags to apply to JumpStart models.""" | ||
|
|
||
| INFERENCE_MODEL_URI = "jumpstart-inference-model-uri" | ||
| INFERENCE_SCRIPT_URI = "jumpstart-inference-script-uri" |
There was a problem hiding this comment.
could you use something like: aws-jumpstart- or aws-smjs- prefix to make it clear this was added by AWS?
System tags look like aws: so I would like to be close to this, even though these are not system tags (see tagging).
evakravi
force-pushed
the
feat/tagging-jumpstart-models
branch
from
2f3efa2 to
5cba331
Compare
…gging-jumpstart-models
2 participants
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.