Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates azure-ai-ml to address new mypy 1.20.0 type-checking failures by applying targeted typing workarounds and adjusting internal guidance for fixing mypy issues.
Changes:
- Expanded a
# type: ignoreonget_model_artifacts()tuple-unpacking to suppress a new mypystr-unpackerror. - Added
cast(Dict[str, Any], ...)atget_storage_client(**datastore_info)call sites to satisfy mypy’s**kwargstyping. - Updated the internal “fix-mypy” skill doc to emphasize fixing only CI-blocking mypy errors and removed PR-creation instructions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
sdk/ml/azure-ai-ml/azure/ai/ml/operations/_local_deployment_helper.py |
Suppresses new mypy unpacking error with an additional ignore code. |
sdk/ml/azure-ai-ml/azure/ai/ml/_artifacts/_artifact_utilities.py |
Uses casts / getattr to satisfy mypy typing around datastore info and artifact paths. |
.github/skills/ml/fix-mypy/SKILL.md |
Tightens scope guidance to focus on CI-blocking mypy issues; reorganizes steps. |
sdk/ml/azure-ai-ml/azure/ai/ml/operations/_local_deployment_helper.py
Outdated
Show resolved
Hide resolved
sdk/ml/azure-ai-ml/azure/ai/ml/_artifacts/_artifact_utilities.py
Outdated
Show resolved
Hide resolved
| model_operations: ModelOperations, | ||
| download_path: str, | ||
| ) -> Union[str, Tuple]: | ||
| ) -> Tuple: |
There was a problem hiding this comment.
will it not break the flow because we are removing one type from it
There was a problem hiding this comment.
all code paths actually return a tuple, so it will not be a breaking change.
| credential=None, | ||
| **kwargs, | ||
| ) -> Dict[Literal["storage_type", "storage_account", "account_url", "container_name", "credential"], str]: | ||
| ) -> Dict[str, Any]: |
There was a problem hiding this comment.
why are we replacing all literal with Any?
Instead of literal shoudn't be enum?
There was a problem hiding this comment.
The root cause of the mypy error was a type mismatch between the return annotation and the local variable. The return type declared [Dict[Literal[...], str]] but the local variable was typed as bare Dict (which mypy infers as Dict[Any, Any]). At call sites like download_artifact where datastore_info is an Optional[Dict] parameter that gets reassigned from get_datastore_info(), mypy sees a union of [Dict[Any, Any] | Dict[Literal[...], str]] — and mypy 1.20.0 rejects ** unpacking on a union of dict types.
To keep Literal keys, we'd also need to update the parameter types at all call sites (Optional[Dict] → [Optional[Dict[Literal[...], Any]]], which is verbose and fragile, and requires refactoring at 10+ places.
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