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

ui: simplify/shorten error messages #3730

Merged
merged 2 commits into from
May 4, 2020
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion dvc/command/run.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ def run(self):
name=self.args.name,
)
except DvcException:
logger.exception("failed to run command")
logger.exception("")
return 1

return 0
Expand Down
38 changes: 18 additions & 20 deletions dvc/exceptions.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,16 @@
"""Exceptions raised by the dvc."""
from funcy import first

from dvc.utils import relpath, format_link


class DvcException(Exception):
"""Base class for all dvc exceptions."""

def __init__(self, msg, *args):
assert msg
super().__init__(msg, *args)


class InvalidArgumentError(ValueError, DvcException):
"""Thrown if arguments are invalid."""
Expand All @@ -23,13 +28,15 @@ class OutputDuplicationError(DvcException):
def __init__(self, output, stages):
assert isinstance(output, str)
assert all(hasattr(stage, "relpath") for stage in stages)
msg = (
"file/directory '{}' is specified as an output in more than one "
"stage: \n{}\n"
"This is not allowed. Consider using a different output name."
).format(
output, "\n".join("\t{}".format(s.addressing) for s in stages)
)
if len(stages) == 1:
msg = "output '{}' is already specified in {}.".format(
output, first(stages)
)
else:
msg = "output '{}' is already specified in stages:\n{}".format(
output,
"\n".join("\t- {}".format(s.addressing) for s in stages),
)
super().__init__(msg)
self.stages = stages
self.output = output
Expand Down Expand Up @@ -81,10 +88,7 @@ class CircularDependencyError(DvcException):
def __init__(self, dependency):
assert isinstance(dependency, str)

msg = (
"file/directory '{}' is specified as an output and as a "
"dependency."
)
msg = "'{}' is specified as an output and as a dependency."
super().__init__(msg.format(dependency))


Expand Down Expand Up @@ -133,22 +137,16 @@ def __init__(self):
class CyclicGraphError(DvcException):
def __init__(self, stages):
assert isinstance(stages, list)
stages = "\n".join(
"\t- {}".format(stage.addressing) for stage in stages
)
msg = (
"you've introduced a cycle in your pipeline that involves "
"the following stages:"
"\n"
"{stages}".format(stages=stages)
msg = "Pipeline has a cycle involving: {}.".format(
", ".join(s.addressing for s in stages)
)
super().__init__(msg)


class ConfirmRemoveError(DvcException):
def __init__(self, path):
super().__init__(
"unable to remove '{}' without a confirmation from the user. Use "
"unable to remove '{}' without a confirmation. Use "
"`-f` to force.".format(path)
)

Expand Down
5 changes: 4 additions & 1 deletion dvc/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from dvc import analytics
from dvc.cli import parse_args
from dvc.config import ConfigError
from dvc.exceptions import DvcParserError
from dvc.exceptions import DvcParserError, DvcException
from dvc.exceptions import NotDvcRepoError
from dvc.external_repo import clean_repos
from dvc.logger import disable_other_loggers, FOOTER
Expand Down Expand Up @@ -58,6 +58,9 @@ def main(argv=None):
ret = 253
except DvcParserError:
ret = 254
except DvcException:
ret = 255
logger.exception("")
Comment on lines +61 to +63
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DvcException won't have unexpected error prepended. Other exceptions and assertion errors will still have that prefixed, which is a good compromise, I'd say.

except Exception as exc: # pylint: disable=broad-except
if isinstance(exc, OSError) and exc.errno == errno.EMFILE:
logger.exception(
Expand Down
5 changes: 3 additions & 2 deletions dvc/stage/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -608,8 +608,9 @@ def _run(self):
if old_handler:
signal.signal(signal.SIGINT, old_handler)

if (p is None) or (p.returncode != 0):
raise StageCmdFailedError(self)
retcode = None if not p else p.returncode
if retcode != 0:
raise StageCmdFailedError(self, retcode)

@rwlocked(read=["deps"], write=["outs"])
def run(
Expand Down
12 changes: 6 additions & 6 deletions dvc/stage/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@


class StageCmdFailedError(DvcException):
def __init__(self, stage):
msg = "{} cmd '{}' failed".format(stage, stage.cmd)
def __init__(self, stage, status=None):
msg = "failed to run: {}".format(stage.cmd)
if status is not None:
msg += ", exited with {}".format(status)
super().__init__(msg)


Expand Down Expand Up @@ -88,9 +90,7 @@ def __init__(self, missing_files):
class StageNotFound(KeyError, DvcException):
def __init__(self, file, name):
super().__init__(
"Stage with '{}' name not found inside '{}' file".format(
name, file.relpath
)
"Stage '{}' not found inside '{}' file".format(name, file.relpath)
)


Expand All @@ -105,7 +105,7 @@ def __init__(self, file):
class DuplicateStageName(DvcException):
def __init__(self, name, file):
super().__init__(
"Stage with name '{name}' already exists in '{relpath}'.".format(
"Stage '{name}' already exists in '{relpath}'.".format(
name=name, relpath=file.relpath
)
)
Expand Down