Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug Fix Issue #4603 Access problem #4604

Merged
merged 10 commits into from Dec 7, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion mlflow/projects/docker.py
Expand Up @@ -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__)

Expand Down Expand Up @@ -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


Expand Down
24 changes: 24 additions & 0 deletions mlflow/utils/file_utils.py
Expand Up @@ -7,6 +7,7 @@
import sys
import tarfile
import tempfile
import stat

import urllib.parse
import urllib.request
Expand Down Expand Up @@ -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)
1 change: 0 additions & 1 deletion 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
Expand Down
28 changes: 27 additions & 1 deletion tests/utils/test_file_utils.py
Expand Up @@ -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
Expand Down Expand Up @@ -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)