From 3872f79dad476dd399c3e16e7fd0b7e96d212806 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Saugat=20Pachhai=20=28=E0=A4=B8=E0=A5=8C=E0=A4=97=E0=A4=BE?= =?UTF-8?q?=E0=A4=A4=29?= Date: Tue, 17 May 2022 19:56:05 +0545 Subject: [PATCH] objects.fs: do not use format_link, DvcException and fix_env from DVC * Introduce ConfigError and AuthError in dvc.objects.fs and raise it instead of dvc.config.ConfigError. * Provide hints and links by catching those errors in dvc.cli.main, i.e decouple UI messages from filesystems for AuthError and ConfigError. * Copy flatten/unflatten utils to dvc.objects.fs.utils. --- dvc/cli/__init__.py | 17 +++++++++--- dvc/fs/__init__.py | 7 ++--- dvc/objects/fs/base.py | 18 +------------ dvc/objects/fs/errors.py | 29 ++++++++++++++++++++ dvc/objects/fs/implementations/azure.py | 13 +++------ dvc/objects/fs/implementations/gdrive.py | 34 +++++++----------------- dvc/objects/fs/implementations/hdfs.py | 17 +++++++++--- dvc/objects/fs/implementations/http.py | 3 +-- dvc/objects/fs/implementations/s3.py | 5 ++-- dvc/objects/fs/utils.py | 12 +++++++++ tests/unit/fs/test_s3.py | 3 +-- 11 files changed, 90 insertions(+), 68 deletions(-) create mode 100644 dvc/objects/fs/errors.py diff --git a/dvc/cli/__init__.py b/dvc/cli/__init__.py index 5c73acd21..a3f22f646 100644 --- a/dvc/cli/__init__.py +++ b/dvc/cli/__init__.py @@ -38,8 +38,7 @@ def parse_args(argv=None): def _log_exceptions(exc: Exception): """Try to log some known exceptions, that are not DVCExceptions.""" - from dvc.objects.cache import DiskError - from dvc.objects.fs.base import RemoteMissingDepsError + from dvc.fs import AuthError, ConfigError, RemoteMissingDepsError from dvc.utils import error_link, format_link if isinstance(exc, RemoteMissingDepsError): @@ -72,6 +71,17 @@ def _log_exceptions(exc: Exception): ) return + if isinstance(exc, (AuthError, ConfigError)): + link = format_link("https://man.dvc.org/remote/modify") + logger.exception("configuration error") + logger.exception( + f"{exc!s}\nLearn more about configuration settings at {link}.", + extra={"tb_only": True}, + ) + return 251 + + from dvc.objects.cache import DiskError + if isinstance(exc, DiskError): from dvc.utils import relpath @@ -172,8 +182,7 @@ def main(argv=None): # noqa: C901 ret = 254 except Exception as exc: # noqa, pylint: disable=broad-except # pylint: disable=no-member - _log_exceptions(exc) - ret = 255 + ret = _log_exceptions(exc) or 255 try: from dvc import analytics diff --git a/dvc/fs/__init__.py b/dvc/fs/__init__.py index 59b3f023b..312b254b7 100644 --- a/dvc/fs/__init__.py +++ b/dvc/fs/__init__.py @@ -24,9 +24,10 @@ from dvc.objects.fs import ( # noqa: F401 get_fs_config, system, ) -from dvc.objects.fs.base import ( # noqa: F401 - AnyFSPath, - FileSystem, +from dvc.objects.fs.base import AnyFSPath, FileSystem # noqa: F401 +from dvc.objects.fs.errors import ( # noqa: F401 + AuthError, + ConfigError, RemoteMissingDepsError, ) from dvc.objects.fs.implementations.azure import AzureAuthError # noqa: F401 diff --git a/dvc/objects/fs/base.py b/dvc/objects/fs/base.py index 1688029b6..ff39b9e47 100644 --- a/dvc/objects/fs/base.py +++ b/dvc/objects/fs/base.py @@ -21,6 +21,7 @@ from funcy import cached_property from ..executors import ThreadPoolExecutor from ._callback import DEFAULT_CALLBACK, FsspecCallback +from .errors import RemoteMissingDepsError if TYPE_CHECKING: from typing import BinaryIO, TextIO @@ -51,23 +52,6 @@ class LinkError(OSError): ) -class RemoteMissingDepsError(Exception): - def __init__( - self, - fs: "FileSystem", - protocol: str, - url: str, - missing: List[str] = None, - ) -> None: - self.protocol = protocol - self.fs = fs - self.url = url - self.missing_deps = missing or [] - super().__init__( - f"filesystem for '{protocol}': '{type(fs)}' missing dependencies" - ) - - class FileSystem: sep = "/" diff --git a/dvc/objects/fs/errors.py b/dvc/objects/fs/errors.py new file mode 100644 index 000000000..75852b15a --- /dev/null +++ b/dvc/objects/fs/errors.py @@ -0,0 +1,29 @@ +from typing import TYPE_CHECKING, List + +if TYPE_CHECKING: + from .base import FileSystem + + +class AuthError(Exception): + pass + + +class ConfigError(Exception): + pass + + +class RemoteMissingDepsError(Exception): + def __init__( + self, + fs: "FileSystem", + protocol: str, + url: str, + missing: List[str] = None, + ) -> None: + self.protocol = protocol + self.fs = fs + self.url = url + self.missing_deps = missing or [] + super().__init__( + f"filesystem for '{protocol}': '{type(fs)}' missing dependencies" + ) diff --git a/dvc/objects/fs/implementations/azure.py b/dvc/objects/fs/implementations/azure.py index f9cc4c1d6..ded99de07 100644 --- a/dvc/objects/fs/implementations/azure.py +++ b/dvc/objects/fs/implementations/azure.py @@ -6,10 +6,8 @@ from fsspec.asyn import fsspec_loop from fsspec.utils import infer_storage_options from funcy import cached_property, memoize, wrap_prop -from dvc.exceptions import DvcException -from dvc.utils import format_link - from ..base import ObjectFileSystem +from ..errors import AuthError logger = logging.getLogger(__name__) _DEFAULT_CREDS_STEPS = ( @@ -19,7 +17,7 @@ _DEFAULT_CREDS_STEPS = ( ) -class AzureAuthError(DvcException): +class AzureAuthError(AuthError): pass @@ -91,9 +89,7 @@ class AzureFileSystem(ObjectFileSystem): if not (login_info["account_name"] or login_info["connection_string"]): raise AzureAuthError( "Authentication to Azure Blob Storage requires either " - "account_name or connection_string.\nLearn more about " - "configuration settings at " - + format_link("https://man.dvc.org/remote/modify") + "account_name or connection_string." ) any_secondary = any( @@ -154,6 +150,5 @@ class AzureFileSystem(ObjectFileSystem): except (ValueError, AzureError) as e: raise AzureAuthError( f"Authentication to Azure Blob Storage via {self.login_method}" - " failed.\nLearn more about configuration settings at" - f" {format_link('https://man.dvc.org/remote/modify')}" + " failed." ) from e diff --git a/dvc/objects/fs/implementations/gdrive.py b/dvc/objects/fs/implementations/gdrive.py index 2c841535e..7c2e2a198 100644 --- a/dvc/objects/fs/implementations/gdrive.py +++ b/dvc/objects/fs/implementations/gdrive.py @@ -4,23 +4,21 @@ import threading from funcy import cached_property, wrap_prop -from dvc.exceptions import DvcException -from dvc.utils import format_link - from ..base import FileSystem +from ..errors import AuthError, ConfigError from ..utils import tmp_fname logger = logging.getLogger(__name__) FOLDER_MIME_TYPE = "application/vnd.google-apps.folder" -class GDriveAuthError(DvcException): +class GDriveAuthError(AuthError): def __init__(self, cred_location): if cred_location: message = ( "GDrive remote auth failed with credentials in '{}'.\n" - "Backup first, remove or fix them, and run DVC again.\n" + "Backup first, remove or fix them, and run again.\n" "It should do auth again and refresh the credentials.\n\n" "Details:".format(cred_location) ) @@ -52,12 +50,7 @@ class GDriveFileSystem(FileSystem): # pylint:disable=abstract-method opts = infer_storage_options(self.url) if not opts["host"]: - raise DvcException( - "Empty GDrive URL '{}'. Learn more at {}".format( - config["url"], - format_link("https://man.dvc.org/remote/add"), - ) - ) + raise ConfigError("Empty GDrive URL '{}'.".format(config["url"])) self._bucket = opts["host"] self._path = opts["path"].lstrip("/") @@ -110,24 +103,18 @@ class GDriveFileSystem(FileSystem): # pylint:disable=abstract-method and not self._service_account_json_file_path and GDriveFileSystem.GDRIVE_CREDENTIALS_DATA not in os.environ ): - raise DvcException( + raise ConfigError( "To use service account, set " "`gdrive_service_account_json_file_path`, and optionally" - "`gdrive_service_account_user_email` in DVC config\n" - "Learn more at {}".format( - format_link("https://man.dvc.org/remote/modify") - ) + "`gdrive_service_account_user_email` in DVC config." ) # Validate OAuth 2.0 Client ID configuration if not self._use_service_account: if bool(self._client_id) != bool(self._client_secret): - raise DvcException( + raise ConfigError( "Please specify GDrive's client ID and secret in " - "DVC config or omit both to use the defaults.\n" - "Learn more at {}".format( - format_link("https://man.dvc.org/remote/modify") - ) + "DVC config or omit both to use the defaults." ) @cached_property @@ -163,10 +150,7 @@ class GDriveFileSystem(FileSystem): # pylint:disable=abstract-method "Client ID and secret configured do not match the " "actual ones used\nto access the remote. Do you " "use multiple GDrive remotes and forgot to\nset " - "`gdrive_user_credentials_file` for one or more of them? " - "Learn more at\n{}.\n".format( - format_link("https://man.dvc.org/remote/modify") - ) + "`gdrive_user_credentials_file` for one or more of them?" ) @wrap_prop(threading.RLock()) diff --git a/dvc/objects/fs/implementations/hdfs.py b/dvc/objects/fs/implementations/hdfs.py index 399cc4966..8e794c9f8 100644 --- a/dvc/objects/fs/implementations/hdfs.py +++ b/dvc/objects/fs/implementations/hdfs.py @@ -1,17 +1,28 @@ import os import re import subprocess +import sys import threading from funcy import cached_property, wrap_prop -from dvc.utils import fix_env - from ..base import FileSystem CHECKSUM_REGEX = re.compile(r".*\t.*\t(?P.*)") +def fix_env(): + env = os.environ.copy() + if getattr(sys, "frozen", False): + lp_key = "LD_LIBRARY_PATH" + lp_orig = env.get(lp_key + "_ORIG", None) + if lp_orig is not None: + env[lp_key] = lp_orig + else: + env.pop(lp_key, None) + return env + + # pylint: disable=abstract-method class HDFSFileSystem(FileSystem): protocol = "hdfs" @@ -58,7 +69,7 @@ class HDFSFileSystem(FileSystem): result = self._run_command( f"checksum {url}", - env=fix_env(os.environ), + env=fix_env(), user=self.fs_args.get("user"), ) if result is None: diff --git a/dvc/objects/fs/implementations/http.py b/dvc/objects/fs/implementations/http.py index bdc456743..0b6732dd8 100644 --- a/dvc/objects/fs/implementations/http.py +++ b/dvc/objects/fs/implementations/http.py @@ -6,6 +6,7 @@ from funcy import cached_property, memoize, wrap_with from .._callback import DEFAULT_CALLBACK, FsspecCallback from ..base import AnyFSPath, FileSystem +from ..errors import ConfigError @wrap_with(threading.Lock()) @@ -42,8 +43,6 @@ class HTTPFileSystem(FileSystem): import aiohttp from fsspec.asyn import fsspec_loop - from dvc.config import ConfigError - credentials = {} client_kwargs = credentials.setdefault("client_kwargs", {}) diff --git a/dvc/objects/fs/implementations/s3.py b/dvc/objects/fs/implementations/s3.py index 2aa53f98d..c2f7360ae 100644 --- a/dvc/objects/fs/implementations/s3.py +++ b/dvc/objects/fs/implementations/s3.py @@ -5,6 +5,8 @@ from collections import defaultdict from funcy import cached_property, wrap_prop from ..base import ObjectFileSystem +from ..errors import ConfigError +from ..utils import flatten, unflatten _AWS_CONFIG_PATH = os.path.join(os.path.expanduser("~"), ".aws", "config") @@ -72,9 +74,6 @@ class S3FileSystem(ObjectFileSystem): return self._split_s3_config(s3_config) def _prepare_credentials(self, **config): - from dvc.config import ConfigError - from dvc.utils.flatten import flatten, unflatten - login_info = defaultdict(dict) # credentials diff --git a/dvc/objects/fs/utils.py b/dvc/objects/fs/utils.py index e387537fb..37e0625eb 100644 --- a/dvc/objects/fs/utils.py +++ b/dvc/objects/fs/utils.py @@ -236,3 +236,15 @@ def human_readable_to_bytes(value: str) -> int: multiplier = MULTIPLIERS.get(suffix, 1) return int(value) * multiplier + + +def flatten(d): + import flatten_dict + + return flatten_dict.flatten(d, reducer="dot") + + +def unflatten(d): + import flatten_dict + + return flatten_dict.unflatten(d, splitter="dot") diff --git a/tests/unit/fs/test_s3.py b/tests/unit/fs/test_s3.py index 193aa6cc8..cc24fee1f 100644 --- a/tests/unit/fs/test_s3.py +++ b/tests/unit/fs/test_s3.py @@ -2,8 +2,7 @@ import os import pytest -from dvc.config import ConfigError -from dvc.fs import S3FileSystem +from dvc.fs import ConfigError, S3FileSystem bucket_name = "bucket-name" prefix = "some/prefix"