Change allowed_instance_types to accept list instead of space-delimit…#46141
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates allowed_instance_types to be represented as a YAML/SDK list (rather than a single space-delimited string), aligning schema + entity serialization with the service’s array shape.
Changes:
- Updated
DeploymentTemplateSchemato parseallowed_instance_typesasList[str]. - Updated
DeploymentTemplateto typeallowed_instance_typesasOptional[List[str]]and to serialize it directly asallowedInstanceTypes. - Removed string-splitting conversion in
DeploymentTemplateOperations._convert_dict_to_deployment_template, and updated one unit test to use a list.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| sdk/ml/azure-ai-ml/tests/deployment_template/unittests/test_deployment_template.py | Updates initialization assertion to use list form for allowed_instance_types. |
| sdk/ml/azure-ai-ml/azure/ai/ml/operations/_deployment_template_operations.py | Stops converting string allowed_instance_types into a list during dict-to-entity conversion. |
| sdk/ml/azure-ai-ml/azure/ai/ml/entities/_deployment/deployment_template.py | Changes allowed_instance_types typing to List[str] and passes through list directly in REST serialization. |
| sdk/ml/azure-ai-ml/azure/ai/ml/_schema/_deployment/template/deployment_template.py | Changes marshmallow schema field type from Str to List(Str). |
| code_configuration: Optional[Dict[str, Any]] = None, | ||
| environment_variables: Optional[Dict[str, str]] = None, | ||
| app_insights_enabled: Optional[bool] = None, | ||
| allowed_instance_types: Optional[str] = None, | ||
| allowed_instance_types: Optional[List[str]] = None, | ||
| default_instance_type: Optional[str] = None, # Handle default instance type |
There was a problem hiding this comment.
allowed_instance_types is now annotated as Optional[List[str]], but the constructor assigns it without runtime validation. As a result, callers can still pass a string (or other non-list types) and the object will accept it, contradicting the PR’s stated breaking change and potentially leading to invalid REST payloads later. Add explicit type/element validation here (e.g., reject str, ensure it’s a list of str) and raise a clear error.
| else: | ||
| instance_types_array = [str(self.allowed_instance_types)] | ||
| result["allowedInstanceTypes"] = instance_types_array # type: ignore[assignment] | ||
| result["allowedInstanceTypes"] = self.allowed_instance_types # type: ignore[assignment] |
There was a problem hiding this comment.
_to_rest_object now forwards self.allowed_instance_types directly to allowedInstanceTypes without coercion or validation. If a non-list slips through (e.g., a str from user code), this will produce an invalid request body and fail at service call time. Validate/normalize before setting the REST field (and prefer failing fast with a helpful exception).
| result["allowedInstanceTypes"] = self.allowed_instance_types # type: ignore[assignment] | |
| allowed_instance_types = self.allowed_instance_types | |
| if isinstance(allowed_instance_types, tuple): | |
| allowed_instance_types = list(allowed_instance_types) | |
| elif isinstance(allowed_instance_types, (str, bytes)) or not isinstance(allowed_instance_types, list): | |
| raise TypeError( | |
| "allowed_instance_types must be a list of strings when provided." | |
| ) | |
| if not all(isinstance(instance_type, str) for instance_type in allowed_instance_types): | |
| raise TypeError( | |
| "allowed_instance_types must be a list of strings when provided." | |
| ) | |
| result["allowedInstanceTypes"] = allowed_instance_types # type: ignore[assignment] |
| @@ -140,9 +140,6 @@ def get_field_value(data: dict, primary_name: str, alt_name: str = None, default | |||
|
|
|||
| # Handle field name variations for constructor parameters | |||
| allowed_instance_types = get_field_value(data, "allowed_instance_types", "allowedInstanceTypes") | |||
There was a problem hiding this comment.
_convert_dict_to_deployment_template no longer converts string allowed_instance_types values, but it also doesn’t reject them. This means a user-provided dict can still contain a space-delimited string and the operation will construct a DeploymentTemplate carrying a str, which later becomes an invalid REST payload. If the intent is to make this a breaking change, add a type check here and raise a clear error when allowed_instance_types is a str (and update the related unit test that currently expects string splitting).
| allowed_instance_types = get_field_value(data, "allowed_instance_types", "allowedInstanceTypes") | |
| allowed_instance_types = get_field_value(data, "allowed_instance_types", "allowedInstanceTypes") | |
| if isinstance(allowed_instance_types, str): | |
| raise ValueError( | |
| "allowed_instance_types must be an iterable of strings, not a space-delimited string." | |
| ) |
| assert template.deployment_template_type == "model_deployment" | ||
| assert template.allowed_instance_types == "Standard_DS2_v2,Standard_DS3_v2" | ||
| assert template.allowed_instance_types == ["Standard_DS2_v2", "Standard_DS3_v2"] | ||
|
|
There was a problem hiding this comment.
The updated test only asserts the list form works, but it doesn’t cover the breaking behavior described in the PR (string input should no longer be accepted). Add a unit test asserting that passing a string allowed_instance_types raises a clear exception, so the breaking change is enforced and regressions are caught.
| def test_deployment_template_rejects_string_allowed_instance_types(self): | |
| """Test that string input for allowed_instance_types is rejected.""" | |
| with pytest.raises((TypeError, ValueError), match=r"allowed_instance_types.*list"): | |
| DeploymentTemplate( | |
| name="test-template", | |
| version="1.0", | |
| allowed_instance_types="Standard_DS3_v2", | |
| ) |
allowed_instance_types— Breaking ChangeCLI (YAML input)
Before:
After:
SDK
Before:
After:
If an SDK is being regenerated based on a new API spec, a link to the pull request containing these API spec changes should be included above.
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines