Azure / azure-sdk-for-python

This repository is for active development of the Azure SDK for Python. For consumers of the SDK we recommend visiting our public developer docs at https://learn.microsoft.com/python/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-python.
MIT License
4.54k stars 2.77k forks source link

azureml-mlflow: Cannot download model artifacts with the same filename and directory predicate #29652

Open ssabdb opened 1 year ago

ssabdb commented 1 year ago

Describe the bug Cannot log models with filenname with an overlapping predicate with a directory. For example, retrieving a model with a code directory: src/experiment/test.py src/experiment_tracking.py

Will result in an error.

Full stack trace ``` --------------------------------------------------------------------------- MlflowException Traceback (most recent call last) Cell In[12], line 1 ----> 1 mlflow.pyfunc.load_model(f"runs:/{m.run_id}/testmodel") File /anaconda/envs/azureml_py38/lib/python3.8/site-packages/mlflow/pyfunc/__init__.py:486, in load_model(model_uri, suppress_warnings, dst_path) 458 def load_model( 459 model_uri: str, 460 suppress_warnings: bool = False, 461 dst_path: str = None, 462 ) -> PyFuncModel: 463 """ 464 Load a model stored in Python function format. 465 (...) 484 path will be created. 485 """ --> 486 local_path = _download_artifact_from_uri(artifact_uri=model_uri, output_path=dst_path) 488 if not suppress_warnings: 489 _warn_dependency_requirement_mismatches(local_path) File /anaconda/envs/azureml_py38/lib/python3.8/site-packages/mlflow/tracking/artifact_utils.py:100, in _download_artifact_from_uri(artifact_uri, output_path) 94 """ 95 :param artifact_uri: The *absolute* URI of the artifact to download. 96 :param output_path: The local filesystem path to which to download the artifact. If unspecified, 97 a local output path will be created. 98 """ 99 root_uri, artifact_path = _get_root_uri_and_artifact_path(artifact_uri) --> 100 return get_artifact_repository(artifact_uri=root_uri).download_artifacts( 101 artifact_path=artifact_path, dst_path=output_path 102 ) File /anaconda/envs/azureml_py38/lib/python3.8/site-packages/mlflow/store/artifact/runs_artifact_repo.py:125, in RunsArtifactRepository.download_artifacts(self, artifact_path, dst_path) 110 def download_artifacts(self, artifact_path, dst_path=None): 111 """ 112 Download an artifact file or directory to a local directory if applicable, and return a 113 local path for it. (...) 123 :return: Absolute path of the local filesystem location containing the desired artifacts. 124 """ --> 125 return self.repo.download_artifacts(artifact_path, dst_path) File /anaconda/envs/azureml_py38/lib/python3.8/site-packages/azureml/mlflow/_store/artifact/artifact_repo.py:139, in AzureMLflowArtifactRepository.download_artifacts(self, artifact_path, dst_path) 136 @wraps(ArtifactRepository.download_artifacts) 137 def download_artifacts(self, artifact_path, dst_path=None): 138 artifact_path = self._get_full_artifact_path(artifact_path) --> 139 return super(AzureMLflowArtifactRepository, self).download_artifacts(artifact_path, dst_path=dst_path) File /anaconda/envs/azureml_py38/lib/python3.8/site-packages/mlflow/store/artifact/artifact_repo.py:263, in ArtifactRepository.download_artifacts(self, artifact_path, dst_path) 260 failed_downloads[inflight_download.src_artifact_path] = repr(e) 262 if len(failed_downloads) > 0: --> 263 raise MlflowException( 264 message=( 265 "The following failures occurred while downloading one or more" 266 " artifacts from {artifact_root}: {failures}".format( 267 artifact_root=self.artifact_uri, 268 failures=failed_downloads, 269 ) 270 ) 271 ) 272 return dst_local_path MlflowException: The following failures occurred while downloading one or more artifacts from azureml://experiments/Default/runs/bf5227a3-8ef6-451b-b1bf-1bc9bece896c/artifacts: {'testmodel/code/src/experiment_tracking.py': "IsADirectoryError(21, 'Is a directory')"} ```

To Reproduce Steps to reproduce the behavior:

  1. Create the directory structure:
    mkdir -p src/experiment
    echo "hello" > src/experiment_tracking.py
    echo "hello" > src/experiment/test.py
  2. Upload and download the model
    
    import mlflow

m = mlflow.pyfunc.log_model( loader_module="test", artifact_path="testmodel", code_path=["src"] )

mlflow.pyfunc.load_model(f"runs:/{m.run_id}/testmodel")



**Expected behavior**
The model should download as expected.

**Additional context**
(I'd be happy to submit a PR to fix this, but I can't find the file in github!)

In `azureml/mlflow/_store/artifact/artifact_repo.py`, the predicate `file_path[:len(path)] == path` makes unsafe assumptions to determine if `file_path` is inside the `path` directory on line 111.

In this example, `file_path = "src/experiment_tracking.py"` and `path = "src/experiment"` results in a comparison of `"src/experiment" == "src/experiment"`. This is then labelled as an empty directory which is created.

A workaround is to make sure there are no overlapping directories and paths, e.g. rename the `src/experiment` directory, but this is clearly sub-optimal.
pvaneck commented 1 year ago

Thanks for the feedback. Not sure where azureml-mlflow is maintained, but seems like it is not in this repository. @azureml-github, can you take a look?

ghost commented 1 year ago

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc @azureml-github, @Azure/azure-ml-sdk.

Issue Details
- **Package Name**: azureml-mlflow - **Package Version**: 1.49.0 - **Operating System**: WSL, Ubuntu 20.04 LTS - **Python Version**: 3.10.9 **Describe the bug** Cannot log models with filenname with an overlapping predicate with a directory. For example, retrieving a model with a code directory: `src/experiment/test.py` `src/experiment_tracking.py` Will result in an error.
Full stack trace ``` --------------------------------------------------------------------------- MlflowException Traceback (most recent call last) Cell In[12], line 1 ----> 1 mlflow.pyfunc.load_model(f"runs:/{m.run_id}/testmodel") File /anaconda/envs/azureml_py38/lib/python3.8/site-packages/mlflow/pyfunc/__init__.py:486, in load_model(model_uri, suppress_warnings, dst_path) 458 def load_model( 459 model_uri: str, 460 suppress_warnings: bool = False, 461 dst_path: str = None, 462 ) -> PyFuncModel: 463 """ 464 Load a model stored in Python function format. 465 (...) 484 path will be created. 485 """ --> 486 local_path = _download_artifact_from_uri(artifact_uri=model_uri, output_path=dst_path) 488 if not suppress_warnings: 489 _warn_dependency_requirement_mismatches(local_path) File /anaconda/envs/azureml_py38/lib/python3.8/site-packages/mlflow/tracking/artifact_utils.py:100, in _download_artifact_from_uri(artifact_uri, output_path) 94 """ 95 :param artifact_uri: The *absolute* URI of the artifact to download. 96 :param output_path: The local filesystem path to which to download the artifact. If unspecified, 97 a local output path will be created. 98 """ 99 root_uri, artifact_path = _get_root_uri_and_artifact_path(artifact_uri) --> 100 return get_artifact_repository(artifact_uri=root_uri).download_artifacts( 101 artifact_path=artifact_path, dst_path=output_path 102 ) File /anaconda/envs/azureml_py38/lib/python3.8/site-packages/mlflow/store/artifact/runs_artifact_repo.py:125, in RunsArtifactRepository.download_artifacts(self, artifact_path, dst_path) 110 def download_artifacts(self, artifact_path, dst_path=None): 111 """ 112 Download an artifact file or directory to a local directory if applicable, and return a 113 local path for it. (...) 123 :return: Absolute path of the local filesystem location containing the desired artifacts. 124 """ --> 125 return self.repo.download_artifacts(artifact_path, dst_path) File /anaconda/envs/azureml_py38/lib/python3.8/site-packages/azureml/mlflow/_store/artifact/artifact_repo.py:139, in AzureMLflowArtifactRepository.download_artifacts(self, artifact_path, dst_path) 136 @wraps(ArtifactRepository.download_artifacts) 137 def download_artifacts(self, artifact_path, dst_path=None): 138 artifact_path = self._get_full_artifact_path(artifact_path) --> 139 return super(AzureMLflowArtifactRepository, self).download_artifacts(artifact_path, dst_path=dst_path) File /anaconda/envs/azureml_py38/lib/python3.8/site-packages/mlflow/store/artifact/artifact_repo.py:263, in ArtifactRepository.download_artifacts(self, artifact_path, dst_path) 260 failed_downloads[inflight_download.src_artifact_path] = repr(e) 262 if len(failed_downloads) > 0: --> 263 raise MlflowException( 264 message=( 265 "The following failures occurred while downloading one or more" 266 " artifacts from {artifact_root}: {failures}".format( 267 artifact_root=self.artifact_uri, 268 failures=failed_downloads, 269 ) 270 ) 271 ) 272 return dst_local_path MlflowException: The following failures occurred while downloading one or more artifacts from azureml://experiments/Default/runs/bf5227a3-8ef6-451b-b1bf-1bc9bece896c/artifacts: {'testmodel/code/src/experiment_tracking.py': "IsADirectoryError(21, 'Is a directory')"} ```
**To Reproduce** Steps to reproduce the behavior: 1. Create the directory structure: ``` mkdir -p src/experiment echo "hello" > src/experiment_tracking.py echo "hello" > src/experiment/test.py ``` 2. Upload and download the model ``` import mlflow m = mlflow.pyfunc.log_model( loader_module="test", artifact_path="testmodel", code_path=["src"] ) mlflow.pyfunc.load_model(f"runs:/{m.run_id}/testmodel") ``` **Expected behavior** The model should download as expected. **Additional context** (I'd be happy to submit a PR to fix this, but I can't find the file in github!) In `azureml/mlflow/_store/artifact/artifact_repo.py`, the predicate `file_path[:len(path)] == path` makes unsafe assumptions to determine if `file_path` is inside the `path` directory on line 111. In this example, `file_path = "src/experiment_tracking.py"` and `path = "src/experiment"` results in a comparison of `"src/experiment" == "src/experiment"`. This is then labelled as an empty directory which is created. A workaround is to make sure there are no overlapping directories and paths, e.g. rename the `src/experiment` directory, but this is clearly sub-optimal.
Author: ssabdb
Assignees: luigiw
Labels: `question`, `Machine Learning`, `Service Attention`, `customer-reported`
Milestone: -
ssabdb commented 1 year ago

Just checking in to see if this has reached the right team?