Model integration with DT and online-deployment message #44261
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Copilot finished reviewing on behalf of
kshitij-microsoft
December 4, 2025 08:26
Contributor
There was a problem hiding this comment.
Pull request overview
This PR integrates default deployment template support for models in Azure ML SDK and adds informational messaging when deploying models with configured deployment templates. The changes include:
- Adding
defaultDeploymentTemplatefield to Model entities with proper serialization - Creating new
DefaultDeploymentTemplateclass for managing deployment template references - Updating deployment operations to check for and inform users about default deployment templates
- Modifying REST client code generation for proper field naming (camelCase in JSON)
- Updating swagger configuration and auto-generated REST client operations
Key Changes
- New
DefaultDeploymentTemplateentity class with schema support - Model entity extended with
default_deployment_templatefield - Deployment operations enhanced to display informational messages about default deployment templates
- Sample file demonstrating usage patterns
- Swagger configuration updated with new API version (v2025-04-01-preview)
- REST client operations auto-regenerated with style improvements
Reviewed changes
Copilot reviewed 45 out of 47 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| swagger/.../readme.md | Updated swagger configuration, fixed tag conditions, added v2025-04-01-preview |
| swagger/.../mfe.json | Changed field name from DefaultDeploymentTemplate to defaultDeploymentTemplate (camelCase) |
| samples/model_with_deployment_template_example.py | Example code demonstrating DefaultDeploymentTemplate usage (contains hardcoded credentials) |
| operations/_online_deployment_operations.py | Added credential parameter to upload_dependencies call |
| operations/_model_operations.py | Updated return type and imports for v2021_10_01_dataplanepreview models |
| operations/_deployment_template_operations.py | Temporarily changed endpoint to test environment |
| operations/_batch_deployment_operations.py | Added credential parameter to upload_dependencies call |
| entities/.../default_deployment_template.py | New entity class for deployment template references |
| entities/.../model.py | Added default_deployment_template field with serialization support |
| entities/init.py | Exported DefaultDeploymentTemplate class |
| _utils/_endpoint_utils.py | Added logic to check and log messages about default deployment templates |
| _schema/assets/model.py | Added schema field for default_deployment_template |
| _schema/assets/default_deployment_template.py | New schema for DefaultDeploymentTemplate |
| _restclient/v2021_10_01_dataplanepreview/operations/*.py | Auto-generated REST client operations with style improvements |
API Change CheckAPIView identified API level changes in this PR and created the following API reviews |
Member
|
Please update changelog |
Member
|
Please update samples as per code change. this sample code will be published in the ms public documentation: https://github.com/Azure/azure-sdk-for-python/tree/main/sdk/ml/azure-ai-ml/samples |
Member
|
consider to add/update test cases wherever possible |
jayesh-tanna
reviewed
Dec 5, 2025
| # If we can't fetch the model, continue without the check | ||
| pass | ||
| except Exception: # pylint: disable=broad-except | ||
| # If parsing fails, continue without the check |
Member
There was a problem hiding this comment.
this is really long method. can you refactor this
|
|
||
| def upload_dependencies(deployment: Deployment, orchestrators: OperationOrchestrator) -> None: | ||
| def upload_dependencies( | ||
| deployment: Deployment, orchestrators: OperationOrchestrator, credential: Optional[Any] = None |
Member
There was a problem hiding this comment.
instead of Optional[Any] consider to use Optional[TokenCredential]
| match = re.match(REGISTRY_VERSION_PATTERN, deployment.model, re.IGNORECASE) | ||
| if match: | ||
| registry_name = match.group(1) # Extract registry name from URI | ||
| # asset_type = match.group(2) # e.g., "models" |
Member
There was a problem hiding this comment.
if its comment then can we remove this?
| model_version = match.group(4) | ||
|
|
||
| try: | ||
| # Use the v2021_10_01_dataplanepreview REST client to query the registry |
| service_client_10_2021_dataplanepreview, # service_client with correct base_url | ||
| resource_group_name, # resource_group_name | ||
| _, # subscription_id (not needed) | ||
| _, # service_model_client (not needed here) |
Member
There was a problem hiding this comment.
plz correct the comments here also
| ) | ||
|
|
||
| # Check if model from registry has default_deployment_template | ||
| if credential and deployment.model and is_registry_id_for_resource(deployment.model): |
Member
There was a problem hiding this comment.
consider to add test case for this long added code
| properties=self.properties, | ||
| flavors=( | ||
| {key: FlavorData(data=dict(value)) for key, value in self.flavors.items()} if self.flavors else None | ||
| ), # flatten OrderedDict to dict |
| return model | ||
|
|
||
| def _to_rest_object(self) -> ModelVersion: | ||
| def _to_rest_object(self) -> Union[ModelVersionData, ModelVersion]: |
Member
There was a problem hiding this comment.
is there test case available for this method? if not , should we add one as we have added business logic
| name=name, workspace_name=self._workspace_name, **self._scope_kwargs | ||
| return self._model_container_operation.get(name=name, registry_name=self._registry_name, **self._scope_kwargs) | ||
|
|
||
| def _get_with_workspace(self, name: str, version: Optional[str] = None) -> ModelVersion: # name:latest |
Member
There was a problem hiding this comment.
consider to add or update test case for this method
| ) | ||
|
|
||
| def _get(self, name: str, version: Optional[str] = None) -> ModelVersion: # name:latest | ||
| def _get_with_registry(self, name: str, version: Optional[str] = None) -> ModelVersionData: # name:latest |
Member
There was a problem hiding this comment.
consider to add or update test case for this method
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Please add an informative description that covers that changes made by the pull request and link all relevant issues.
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