fix: propagate deploy tags on the model-customization endpoint paths - #6179
fix: propagate deploy tags on the model-customization endpoint paths#6179rohangujarathi wants to merge 1 commit into
Conversation
|
Claude finished @rohangujarathi's task in 2m 40s —— View job PR Review: propagate deploy tags on model-customization endpoint paths
(The inline-comment tool wasn't available in this run, so findings are consolidated here with The overall approach is sound — centralizing tag normalization in 🔴 High —
|
Tags passed to ModelBuilder.deploy() were accepted but never reached the
endpoints created on the model-customization paths, so they could not be used
for tag-based resource association.
deploy() branches into _deploy_model_customization() for fine-tuned and Nova
models and returns before reaching _deploy_core_endpoint(), which is the only
place that applies the deploy-time tags kwarg. Those paths built their endpoint
tags from self._tags alone, so they saw only the tags ModelBuilder had added
itself. Resolve the endpoint tags once on entry from both sources and use them
for whichever path creates the endpoint.
The restricted-model-package path created its endpoint with no tags at all; it
now receives them like the other two.
Adds _tags_as_key_value_list() to normalize tags into the lowercase key/value
list the sagemaker.core.resources create() calls are typed for. self._tags can
hold either casing, Tag objects, or a bare {key: value} mapping, and the
previous inline reads indexed tag["Key"] directly after format_tags(), which
returns a list unchanged whatever its casing -- so a lowercase list raised
KeyError. That was already reachable through build(tags=...) and add_tags().
The merge deliberately does not go through add_tags(): tag_exists() compares
tags as tag["Key"] == curr_tag["Key"], so writing the lowercase form these
create() calls require back onto the accumulator raises KeyError as soon as
self._tags is non-empty, which is the normal state after build(). Resolving
into a local list instead also keeps deploy() from mutating builder state as a
side effect. SageMaker rejects duplicate tag keys, so on a collision the tag
ModelBuilder manages is kept and the caller's duplicate is dropped rather than
displacing a tag the SDK relies on.
23cab69 to
37d2b09
Compare
|
Claude finished @rohangujarathi's task in 1m 27s —— View job PR Review: propagate deploy tags on model-customization endpoint paths (updated revision)
This revision resolves the 🔴 blocker from the previous review. The deploy-time merge no longer routes through What I verified
Minor (non-blocking)
Looks good to me — the correctness fix is sound and well-tested. Nice work addressing the earlier feedback. |
1 participant
Tags passed to ModelBuilder.deploy() were accepted but never reached the endpoints created on the model-customization paths, so they could not be used for tag-based resource association.
deploy() branches into _deploy_model_customization() for fine-tuned and Nova models and returns before reaching _deploy_core_endpoint(), which is the only place that merges the deploy-time tags kwarg into self._tags. Those paths build their endpoint tags from self._tags, so they saw only the tags ModelBuilder had added itself. Merge the kwarg into the accumulator on entry so all three endpoint-creation paths below pick it up.
The restricted-model-package path created its endpoint with no tags at all; it now receives them like the other two.
Adds _tags_as_key_value_list() to normalize tags into the lowercase key/value list the sagemaker.core.resources create() calls are typed for. self._tags can hold either casing or a bare {key: value} mapping, and the previous inline reads indexed tag["Key"] directly after format_tags(), which returns a list unchanged whatever its casing -- so a lowercase list raised KeyError. That was already reachable through build(tags=...) and add_tags().
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.