Repository navigation
fix(serve): handle partial base model metadata - #6466
lucasjia-aws wants to merge 1 commit into
Conversation
Normalize optional BaseModel fields, preserve latest Hub content lookup, and safely resolve hosting configurations through an explicit recipe override or unambiguous inference. Prevent unset model metadata from leaking into generated resource tags. Fixes aws#5667
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 AI code review
This PR hardens ModelBuilder against partially populated generated BaseModel metadata: it normalizes Unassigned/empty strings via _optional_base_model_string, omits an absent HubContentVersion so the latest is chosen, adds a public recipe_name constructor override, reworks _extract_hosting_configs_from_hub to prefer top-level configs and infer a unique recipe (raising on ambiguity), and avoids emitting invalid resource tags. The implementation is coherent and well covered by nine new unit tests. A couple of minor points below; nothing blocking.
Reviewed commit de649e1. Automated review; verify before acting.
| base_model = model_package.inference_specification.containers[0].base_model | ||
| recipe_name = self._effective_recipe_name(base_model) | ||
| if recipe_name is None: | ||
| raise ValueError( |
There was a problem hiding this comment.
🟠 Medium: fetch_endpoint_names_for_base_model now raises ValueError when _effective_recipe_name returns None, which is new behavior (previously it read base_model.recipe_name directly). This is a reasonable change, but none of the nine added tests exercise this new raise path, and the method had existing unit coverage (test_model_builder.py::test_fetch_endpoint_names_for_base_model). Please add a test asserting the ValueError is raised when no recipe is resolvable so this contract is pinned.
| return None | ||
| return value | ||
|
|
||
| def _effective_recipe_name(self, base_model: Any) -> Optional[str]: |
There was a problem hiding this comment.
🟡 Low: _effective_recipe_name prefers the public self.recipe_name override over the BaseModel's own recipe_name in every call site, including PEFT detection (_fetch_peft, _deploy_model_customization) and _is_nova_model, not just hosting-config disambiguation. The docstring/field help describes recipe_name as a disambiguation override, so a user who sets it to pick a hosting config would also silently change PEFT-type ("lora" in recipe_name) and Nova detection. Consider restricting the override to the hosting-config resolution paths, or documenting that it influences PEFT/Nova inference as well.
Summary
Generated BaseModel fields may contain
Unassignedwhen Hub content version or recipe metadata is omitted. BecauseUnassignedis truthy, ModelBuilder can treat missing metadata as real values, fail recipe matching, or emit invalid resource tags.This change normalizes absent BaseModel strings, omits missing Hub content versions so the latest version is selected, and adds an optional
recipe_nameconstructor override. When recipe metadata is absent, ModelBuilder first uses top-level HostingConfigs, otherwise infers a recipe only when exactly one deployable candidate exists, and reports all candidates when explicit disambiguation is required.Unset model and recipe metadata is omitted from generated tags. Existing named-recipe and top-level fallback behavior remains supported.
Testing
git diff --checkpassed.sagemaker-servewheel and source distribution successfully.Fixes #5667