From f53a68378de21fc42136761ff46a1eae243361d5 Mon Sep 17 00:00:00 2001 From: osobky Date: Mon, 26 Jul 2021 12:05:28 +0200 Subject: [PATCH 1/9] Solved the access error Signed-off-by: osobky --- mlflow/projects/docker.py | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/mlflow/projects/docker.py b/mlflow/projects/docker.py index 19411a106f9fc..43e0a6e8254c9 100644 --- a/mlflow/projects/docker.py +++ b/mlflow/projects/docker.py @@ -99,6 +99,25 @@ def _get_docker_image_uri(repository_uri, work_dir): return repository_uri + version_string +def onerror(func, path, exc_info): + """ + Error handler for ``shutil.rmtree``. + + If the error is due to an access error (read only file) + it attempts to add write permission and then retries. + + If the error is for another reason it re-raises the error. + + Usage : ``shutil.rmtree(path, onerror=onerror)`` + """ + import stat + if not os.access(path, os.W_OK): + # Is the error an access error ? + os.chmod(path, stat.S_IWUSR) + func(path) + else: + raise + def _create_docker_build_ctx(work_dir, dockerfile_contents): """ Creates build context tarfile containing Dockerfile and project code, returning path to tarfile @@ -114,7 +133,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=onerror) return result_path From 3a472a7a7f864a677c1acee8d5b6cba7f24d11b5 Mon Sep 17 00:00:00 2001 From: harupy Date: Sun, 5 Dec 2021 12:49:02 +0900 Subject: [PATCH 2/9] use more conservative function Signed-off-by: harupy --- mlflow/projects/docker.py | 37 +++++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/mlflow/projects/docker.py b/mlflow/projects/docker.py index 43e0a6e8254c9..8b33017072f5d 100644 --- a/mlflow/projects/docker.py +++ b/mlflow/projects/docker.py @@ -1,5 +1,6 @@ import logging import os +import stat import posixpath import shutil import tempfile @@ -99,24 +100,28 @@ def _get_docker_image_uri(repository_uri, work_dir): return repository_uri + version_string -def onerror(func, path, exc_info): +def handle_readonly(func, path, exc_info): """ - Error handler for ``shutil.rmtree``. + Clear the readonly bit and reattempt the removal. - If the error is due to an access error (read only file) - it attempts to add write permission and then retries. - - If the error is for another reason it re-raises the error. - - Usage : ``shutil.rmtree(path, onerror=onerror)`` + References: + - https://bugs.python.org/issue19643 + - https://bugs.python.org/issue43657 """ - import stat - if not os.access(path, os.W_OK): - # Is the error an access error ? - os.chmod(path, stat.S_IWUSR) - func(path) - else: - raise + exc_class, exc_instance = exc_info[:2] + should_reattempt = ( + func in (os.unlink, os.rmdir) + and issubclass(exc_class, PermissionError) + and ( + (os.name == "nt" and exc_instance.winerror == 5) + or (os.name != "nt" and exc_instance.errno == 13) + ) + ) + if not should_reattempt: + raise exc_instance + os.chmod(path, stat.S_IWRITE) + func(path) + def _create_docker_build_ctx(work_dir, dockerfile_contents): """ @@ -133,7 +138,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, onerror=onerror) + shutil.rmtree(directory, onerror=handle_readonly) return result_path From 6c767ff90f1565014bb179ca41561bf7fb29e5e7 Mon Sep 17 00:00:00 2001 From: harupy Date: Mon, 6 Dec 2021 15:33:48 +0900 Subject: [PATCH 3/9] comment Signed-off-by: harupy --- mlflow/projects/docker.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/mlflow/projects/docker.py b/mlflow/projects/docker.py index 8b33017072f5d..9a39fbe96aff7 100644 --- a/mlflow/projects/docker.py +++ b/mlflow/projects/docker.py @@ -113,8 +113,9 @@ def handle_readonly(func, path, exc_info): func in (os.unlink, os.rmdir) and issubclass(exc_class, PermissionError) and ( - (os.name == "nt" and exc_instance.winerror == 5) - or (os.name != "nt" and exc_instance.errno == 13) + # Check whether the error code indicates "Permission Denied" + (os.name != "nt" and exc_instance.errno == 13) + or (os.name == "nt" and exc_instance.winerror == 5) ) ) if not should_reattempt: From 23cb4d6b40e5e95b9617ae0102b177b8b15ca611 Mon Sep 17 00:00:00 2001 From: harupy Date: Mon, 6 Dec 2021 21:02:52 +0900 Subject: [PATCH 4/9] add test Signed-off-by: harupy --- mlflow/projects/docker.py | 27 ++----------------------- mlflow/utils/file_utils.py | 22 ++++++++++++++++++++ tests/projects/test_docker_projects.py | 4 ++-- tests/utils/test_file_utils.py | 28 +++++++++++++++++++++++++- 4 files changed, 53 insertions(+), 28 deletions(-) diff --git a/mlflow/projects/docker.py b/mlflow/projects/docker.py index 9a39fbe96aff7..1a6bb6f763334 100644 --- a/mlflow/projects/docker.py +++ b/mlflow/projects/docker.py @@ -16,6 +16,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__) @@ -100,30 +101,6 @@ def _get_docker_image_uri(repository_uri, work_dir): return repository_uri + version_string -def handle_readonly(func, path, exc_info): - """ - Clear the readonly bit and reattempt the removal. - - References: - - https://bugs.python.org/issue19643 - - https://bugs.python.org/issue43657 - """ - exc_class, exc_instance = exc_info[:2] - should_reattempt = ( - func in (os.unlink, os.rmdir) - and issubclass(exc_class, PermissionError) - and ( - # Check whether the error code indicates "Permission Denied" - (os.name != "nt" and exc_instance.errno == 13) - or (os.name == "nt" and exc_instance.winerror == 5) - ) - ) - if not should_reattempt: - raise exc_instance - os.chmod(path, stat.S_IWRITE) - func(path) - - def _create_docker_build_ctx(work_dir, dockerfile_contents): """ Creates build context tarfile containing Dockerfile and project code, returning path to tarfile @@ -139,7 +116,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, onerror=handle_readonly) + 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..8138e2008813e 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,24 @@ 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): + """ + Clear the readonly bit and reattempt the removal on Windows. + + References: + - https://bugs.python.org/issue19643 + - https://bugs.python.org/issue43657 + """ + exc_class, exc_instance = exc_info[:2] + should_reattempt = ( + os.name == "nt" + and func in (os.unlink, os.rmdir) + and issubclass(exc_class, PermissionError) + and exc_instance.winerror == 5 + ) + if not should_reattempt: + raise exc_instance + 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..5eb4df2789b7c 100644 --- a/tests/projects/test_docker_projects.py +++ b/tests/projects/test_docker_projects.py @@ -1,14 +1,14 @@ import os +import stat import pytest -import posixpath # pylint: disable=unused-import from unittest import mock from databricks_cli.configure.provider import DatabricksConfig import mlflow from mlflow.entities import ViewType -from mlflow.projects.docker import _get_docker_image_uri +from mlflow.projects.docker import _get_docker_image_uri, _handle_readonly_on_windows from mlflow.projects import ExecutionException from mlflow.projects.backend.local import _get_docker_command from mlflow.store.tracking import file_store diff --git a/tests/utils/test_file_utils.py b/tests/utils/test_file_utils.py index 223ac53774ec0..2c7cdd5cdbd62 100644 --- a/tests/utils/test_file_utils.py +++ b/tests/utils/test_file_utils.py @@ -8,7 +8,12 @@ import tarfile 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 +173,24 @@ 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): + import win32api + import win32con + + tmp_path = tmpdir.join("file").strpath + with open(tmp_path, "w"): + pass + + win32api.SetFileAttributes(tmp_path, win32con.FILE_ATTRIBUTE_READONLY) + 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) From 961517156b496f36bea387ce0ce8e7ffc447a285 Mon Sep 17 00:00:00 2001 From: harupy Date: Mon, 6 Dec 2021 21:07:31 +0900 Subject: [PATCH 5/9] fix test Signed-off-by: harupy --- tests/utils/test_file_utils.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/utils/test_file_utils.py b/tests/utils/test_file_utils.py index 2c7cdd5cdbd62..648278742a5ad 100644 --- a/tests/utils/test_file_utils.py +++ b/tests/utils/test_file_utils.py @@ -6,6 +6,7 @@ import shutil import pytest import tarfile +import stat from mlflow.utils import file_utils from mlflow.utils.file_utils import ( @@ -177,14 +178,13 @@ def test_dir_copy(): @pytest.mark.skipif(os.name != "nt", reason="requires Windows") def test_handle_readonly_on_windows(tmpdir): - import win32api - import win32con - tmp_path = tmpdir.join("file").strpath with open(tmp_path, "w"): pass - win32api.SetFileAttributes(tmp_path, win32con.FILE_ATTRIBUTE_READONLY) + # 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) From 3121176ed38b86166e450cea2d5abbbc1dcece9c Mon Sep 17 00:00:00 2001 From: harupy Date: Mon, 6 Dec 2021 21:10:23 +0900 Subject: [PATCH 6/9] rename variables Signed-off-by: harupy --- mlflow/utils/file_utils.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/mlflow/utils/file_utils.py b/mlflow/utils/file_utils.py index 8138e2008813e..b0ed11ebc74f9 100644 --- a/mlflow/utils/file_utils.py +++ b/mlflow/utils/file_utils.py @@ -470,14 +470,14 @@ def _handle_readonly_on_windows(func, path, exc_info): - https://bugs.python.org/issue19643 - https://bugs.python.org/issue43657 """ - exc_class, exc_instance = exc_info[:2] + exc_type, exc_value = exc_info[:2] should_reattempt = ( os.name == "nt" and func in (os.unlink, os.rmdir) - and issubclass(exc_class, PermissionError) - and exc_instance.winerror == 5 + and issubclass(exc_type, PermissionError) + and exc_value.winerror == 5 ) if not should_reattempt: - raise exc_instance + raise exc_value os.chmod(path, stat.S_IWRITE) func(path) From c3e0c3d3a2577e44c9494ac896a2df9b802af6f7 Mon Sep 17 00:00:00 2001 From: harupy Date: Mon, 6 Dec 2021 21:19:23 +0900 Subject: [PATCH 7/9] remove unused imports Signed-off-by: harupy --- tests/projects/test_docker_projects.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/projects/test_docker_projects.py b/tests/projects/test_docker_projects.py index 5eb4df2789b7c..52123997396a9 100644 --- a/tests/projects/test_docker_projects.py +++ b/tests/projects/test_docker_projects.py @@ -1,5 +1,4 @@ import os -import stat import pytest from unittest import mock @@ -8,7 +7,7 @@ import mlflow from mlflow.entities import ViewType -from mlflow.projects.docker import _get_docker_image_uri, _handle_readonly_on_windows +from mlflow.projects.docker import _get_docker_image_uri from mlflow.projects import ExecutionException from mlflow.projects.backend.local import _get_docker_command from mlflow.store.tracking import file_store From 33c200563bb729f030ab3b904758718d4d40ee10 Mon Sep 17 00:00:00 2001 From: harupy Date: Mon, 6 Dec 2021 21:28:25 +0900 Subject: [PATCH 8/9] lint Signed-off-by: harupy --- mlflow/projects/docker.py | 1 - 1 file changed, 1 deletion(-) diff --git a/mlflow/projects/docker.py b/mlflow/projects/docker.py index 1a6bb6f763334..df3e5f27bcb98 100644 --- a/mlflow/projects/docker.py +++ b/mlflow/projects/docker.py @@ -1,6 +1,5 @@ import logging import os -import stat import posixpath import shutil import tempfile From a8a1ef0e3313e615a04cbc7230cd5113fc8f7020 Mon Sep 17 00:00:00 2001 From: harupy Date: Mon, 6 Dec 2021 22:03:52 +0900 Subject: [PATCH 9/9] update docstring Signed-off-by: harupy --- mlflow/utils/file_utils.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/mlflow/utils/file_utils.py b/mlflow/utils/file_utils.py index b0ed11ebc74f9..2ba9ad6e745d0 100644 --- a/mlflow/utils/file_utils.py +++ b/mlflow/utils/file_utils.py @@ -464,7 +464,9 @@ def download_file_using_http_uri(http_uri, download_path, chunk_size=100000000): def _handle_readonly_on_windows(func, path, exc_info): """ - Clear the readonly bit and reattempt the removal on Windows. + 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