From 3c4be19982ab6359ec383a5610f61dfd7cc98554 Mon Sep 17 00:00:00 2001 From: Omar Elsobky Date: Tue, 7 Dec 2021 12:14:56 +0100 Subject: [PATCH] Bug Fix Issue #4603 Access problem (#4604) * Solved the access error Signed-off-by: osobky * use more conservative function Signed-off-by: harupy * comment Signed-off-by: harupy * add test Signed-off-by: harupy * fix test Signed-off-by: harupy * rename variables Signed-off-by: harupy * remove unused imports Signed-off-by: harupy * lint Signed-off-by: harupy * update docstring Signed-off-by: harupy Co-authored-by: harupy --- mlflow/projects/docker.py | 3 ++- mlflow/utils/file_utils.py | 24 ++++++++++++++++++++++ tests/projects/test_docker_projects.py | 1 - tests/utils/test_file_utils.py | 28 +++++++++++++++++++++++++- 4 files changed, 53 insertions(+), 3 deletions(-) diff --git a/mlflow/projects/docker.py b/mlflow/projects/docker.py index 19411a106f9fc..df3e5f27bcb98 100644 --- a/mlflow/projects/docker.py +++ b/mlflow/projects/docker.py @@ -15,6 +15,7 @@ from mlflow.tracking.context.git_context import _get_git_commit from mlflow.utils import process, file_utils from mlflow.utils.mlflow_tags import MLFLOW_DOCKER_IMAGE_URI, MLFLOW_DOCKER_IMAGE_ID +from mlflow.utils.file_utils import _handle_readonly_on_windows _logger = logging.getLogger(__name__) @@ -114,7 +115,7 @@ def _create_docker_build_ctx(work_dir, dockerfile_contents): output_filename=result_path, source_dir=dst_path, archive_name=_PROJECT_TAR_ARCHIVE_NAME ) finally: - shutil.rmtree(directory) + shutil.rmtree(directory, onerror=_handle_readonly_on_windows) return result_path diff --git a/mlflow/utils/file_utils.py b/mlflow/utils/file_utils.py index fc16cf90704de..2ba9ad6e745d0 100644 --- a/mlflow/utils/file_utils.py +++ b/mlflow/utils/file_utils.py @@ -7,6 +7,7 @@ import sys import tarfile import tempfile +import stat import urllib.parse import urllib.request @@ -459,3 +460,26 @@ def download_file_using_http_uri(http_uri, download_path, chunk_size=100000000): if not chunk: break output_file.write(chunk) + + +def _handle_readonly_on_windows(func, path, exc_info): + """ + This function should not be called directly but should be passed to `onerror` of + `shutil.rmtree` in order to reattempt the removal of a read-only file after making + it writable on Windows. + + References: + - https://bugs.python.org/issue19643 + - https://bugs.python.org/issue43657 + """ + exc_type, exc_value = exc_info[:2] + should_reattempt = ( + os.name == "nt" + and func in (os.unlink, os.rmdir) + and issubclass(exc_type, PermissionError) + and exc_value.winerror == 5 + ) + if not should_reattempt: + raise exc_value + os.chmod(path, stat.S_IWRITE) + func(path) diff --git a/tests/projects/test_docker_projects.py b/tests/projects/test_docker_projects.py index d696261820ef9..52123997396a9 100644 --- a/tests/projects/test_docker_projects.py +++ b/tests/projects/test_docker_projects.py @@ -1,7 +1,6 @@ import os import pytest -import posixpath # pylint: disable=unused-import from unittest import mock from databricks_cli.configure.provider import DatabricksConfig diff --git a/tests/utils/test_file_utils.py b/tests/utils/test_file_utils.py index 223ac53774ec0..648278742a5ad 100644 --- a/tests/utils/test_file_utils.py +++ b/tests/utils/test_file_utils.py @@ -6,9 +6,15 @@ import shutil import pytest import tarfile +import stat from mlflow.utils import file_utils -from mlflow.utils.file_utils import get_parent_dir, _copy_file_or_tree, TempDir +from mlflow.utils.file_utils import ( + get_parent_dir, + _copy_file_or_tree, + TempDir, + _handle_readonly_on_windows, +) from tests.projects.utils import TEST_PROJECT_DIR from tests.helper_functions import random_int, random_file, safe_edit_yaml @@ -168,3 +174,23 @@ def test_dir_copy(): f.write("testing") _copy_file_or_tree(dir_path, copy_path, "") assert filecmp.dircmp(dir_path, copy_path) + + +@pytest.mark.skipif(os.name != "nt", reason="requires Windows") +def test_handle_readonly_on_windows(tmpdir): + tmp_path = tmpdir.join("file").strpath + with open(tmp_path, "w"): + pass + + # Make the file read-only + os.chmod(tmp_path, stat.S_IREAD | stat.S_IRGRP | stat.S_IROTH) + # Ensure the file can't be removed + with pytest.raises(PermissionError, match="Access is denied") as exc: + os.unlink(tmp_path) + + _handle_readonly_on_windows( + os.unlink, + tmp_path, + (exc.type, exc.value, exc.traceback), + ) + assert not os.path.exists(tmp_path)