Fix/stderr (#410)

* implemented context manager for printing to stderr * modified cli to print errors to stderr * updated the runner to split output to stdout and stderr * updated cli tests to read from result.stdout and result.stderr as opposed to result.output * added tests to ensure exceptions and mlem errors are printed to stderr * typo correction - thrown to throws * added test for stderr_echo context manager to fix warning from codecov * fix dvc test * fix dvc test Co-authored-by: Alexander Guschin <[email protected]>
iterative · Sep 19, 2022 · 0dbe116 · 0dbe116
1 parent 51f328a
commit 0dbe116
Show file tree

Hide file tree

Showing 17 changed files with 243 additions and 50 deletions.
diff --git a/mlem/cli/main.py b/mlem/cli/main.py
@@ -23,7 +23,15 @@
 from mlem.core.metadata import load_meta
 from mlem.core.objects import MlemObject
 from mlem.telemetry import telemetry
-from mlem.ui import EMOJI_FAIL, EMOJI_MLEM, bold, cli_echo, color, echo
+from mlem.ui import (
+    EMOJI_FAIL,
+    EMOJI_MLEM,
+    bold,
+    cli_echo,
+    color,
+    echo,
+    stderr_echo,
+)
 
 
 class MlemFormatter(HelpFormatter):
@@ -289,22 +297,22 @@ def inner(ctx, *iargs, **ikwargs):
                 error = f"{e.__class__.__module__}.{e.__class__.__name__}"
                 if ctx.obj["traceback"]:
                     raise
-                with cli_echo():
+                with stderr_echo():
                     echo(EMOJI_FAIL + color(str(e), col=typer.colors.RED))
                 raise typer.Exit(1)
             except ValidationError as e:
                 error = f"{e.__class__.__module__}.{e.__class__.__name__}"
                 if ctx.obj["traceback"]:
                     raise
                 msgs = "\n".join(_format_validation_error(e))
-                with cli_echo():
+                with stderr_echo():
                     echo(EMOJI_FAIL + color("Error:\n", "red") + msgs)
                 raise typer.Exit(1)
             except Exception as e:  # pylint: disable=broad-except
                 error = f"{e.__class__.__module__}.{e.__class__.__name__}"
                 if ctx.obj["traceback"]:
                     raise
-                with cli_echo():
+                with stderr_echo():
                     echo(
                         EMOJI_FAIL
                         + color(

diff --git a/mlem/ui.py b/mlem/ui.py
@@ -10,6 +10,7 @@
 from mlem.config import LOCAL_CONFIG
 
 console = Console()
+error_console = Console(stderr=True)
 
 _echo_func: Optional[Callable] = None
 _offset: int = 0
@@ -46,6 +47,12 @@ def cli_echo():
         yield
 
 
+@contextlib.contextmanager
+def stderr_echo():
+    with set_echo(error_console.print):
+        yield
+
+
 @contextlib.contextmanager
 def no_echo():
     with set_echo(None):

diff --git a/setup.py b/setup.py
@@ -50,6 +50,7 @@
     "pyarrow",
     "skl2onnx",
     "dvc[s3]",
+    "importlib_metadata",
 ]
 
 extras = {

diff --git a/tests/cli/conftest.py b/tests/cli/conftest.py
@@ -7,7 +7,7 @@
 
 class Runner:
     def __init__(self):
-        self._runner = CliRunner()
+        self._runner = CliRunner(mix_stderr=False)
 
     def invoke(self, *args, **kwargs) -> Result:
         return self._runner.invoke(app, *args, **kwargs)

diff --git a/tests/cli/test_apply.py b/tests/cli/test_apply.py
@@ -38,7 +38,11 @@ def test_apply(runner, model_path, data_path):
                 "--no-index",
             ],
         )
-        assert result.exit_code == 0, (result.output, result.exception)
+        assert result.exit_code == 0, (
+            result.stdout,
+            result.stderr,
+            result.exception,
+        )
         predictions = load(path)
         assert isinstance(predictions, ndarray)
 
@@ -85,7 +89,11 @@ def test_apply_batch(runner, model_path_batch, data_path_batch):
                 "5",
             ],
         )
-        assert result.exit_code == 0, (result.output, result.exception)
+        assert result.exit_code == 0, (
+            result.stdout,
+            result.stderr,
+            result.exception,
+        )
         predictions_meta = load_meta(
             path, load_value=True, force_type=MlemData
         )
@@ -116,7 +124,11 @@ def test_apply_with_import(runner, model_meta_saved_single, tmp_path_factory):
                 "pandas[csv]",
             ],
         )
-        assert result.exit_code == 0, (result.output, result.exception)
+        assert result.exit_code == 0, (
+            result.stdout,
+            result.stderr,
+            result.exception,
+        )
         predictions = load(path)
         assert isinstance(predictions, ndarray)
 
@@ -146,19 +158,27 @@ def test_apply_batch_with_import(
                 "2",
             ],
         )
-        assert result.exit_code == 1, (result.output, result.exception)
+        assert result.exit_code == 1, (
+            result.stdout,
+            result.stderr,
+            result.exception,
+        )
         assert (
             "Batch data loading is currently not supported for loading data on-the-fly"
-            in result.output
+            in result.stderr
         )
 
 
 def test_apply_no_output(runner, model_path, data_path):
     result = runner.invoke(
         ["apply", model_path, data_path, "-m", "predict", "--no-index"],
     )
-    assert result.exit_code == 0, (result.output, result.exception)
-    assert len(result.output) > 0
+    assert result.exit_code == 0, (
+        result.stdout,
+        result.stderr,
+        result.exception,
+    )
+    assert len(result.stdout) > 0
 
 
 def test_apply_fails_without_mlem_dir(runner, model_path, data_path):
@@ -176,7 +196,11 @@ def test_apply_fails_without_mlem_dir(runner, model_path, data_path):
                 "--index",
             ],
         )
-        assert result.exit_code == 1, (result.output, result.exception)
+        assert result.exit_code == 1, (
+            result.stdout,
+            result.stderr,
+            result.exception,
+        )
         assert isinstance(result.exception, MlemProjectNotFound)
 
 
@@ -206,7 +230,11 @@ def test_apply_from_remote(runner, current_test_branch, s3_tmp_path):
             "--no-index",
         ],
     )
-    assert result.exit_code == 0, (result.output, result.exception)
+    assert result.exit_code == 0, (
+        result.stdout,
+        result.stderr,
+        result.exception,
+    )
     predictions = load(out)
     assert isinstance(predictions, ndarray)
 
@@ -227,6 +255,10 @@ def test_apply_remote(mlem_client, runner, data_path):
                 path,
             ],
         )
-        assert result.exit_code == 0, (result.output, result.exception)
+        assert result.exit_code == 0, (
+            result.stdout,
+            result.stderr,
+            result.exception,
+        )
         predictions = load(path)
         assert isinstance(predictions, ndarray)
diff --git a/tests/cli/test_build.py b/tests/cli/test_build.py
@@ -21,7 +21,11 @@ def test_build(runner: Runner, model_meta_saved_single, tmp_path):
         f"build {make_posix(model_meta_saved_single.loc.uri)} -c target={make_posix(path)} mock"
     )
 
-    assert result.exit_code == 0, (result.exception, result.output)
+    assert result.exit_code == 0, (
+        result.stdout,
+        result.stderr,
+        result.exception,
+    )
 
     with open(path, encoding="utf8") as f:
         assert f.read().strip() == model_meta_saved_single.loc.path
diff --git a/tests/cli/test_checkenv.py b/tests/cli/test_checkenv.py
@@ -7,7 +7,11 @@ def test_checkenv(runner, model_path_mlem_project):
     result = runner.invoke(
         ["checkenv", model_path],
     )
-    assert result.exit_code == 0, (result.output, result.exception)
+    assert result.exit_code == 0, (
+        result.stdout,
+        result.stderr,
+        result.exception,
+    )
 
     meta = load_meta(model_path, load_value=False, force_type=MlemModel)
     meta.requirements.__root__[0].version = "asdad"
@@ -16,4 +20,8 @@ def test_checkenv(runner, model_path_mlem_project):
     result = runner.invoke(
         ["checkenv", model_path],
     )
-    assert result.exit_code == 1, (result.output, result.exception)
+    assert result.exit_code == 1, (
+        result.stdout,
+        result.stderr,
+        result.exception,
+    )
diff --git a/tests/cli/test_clone.py b/tests/cli/test_clone.py
@@ -9,5 +9,9 @@ def test_model_cloning(runner: Runner, model_path):
     with tempfile.TemporaryDirectory() as path:
         path = posixpath.join(path, "cloned")
         result = runner.invoke(["clone", model_path, path, "--no-index"])
-        assert result.exit_code == 0, (result.exception, result.output)
+        assert result.exit_code == 0, (
+            result.stdout,
+            result.stderr,
+            result.exception,
+        )
         load_meta(path, load_value=False)
diff --git a/tests/cli/test_config.py b/tests/cli/test_config.py
@@ -50,7 +50,7 @@ def test_set_get_validation(runner: Runner, mlem_project):
     assert result.exit_code == 1
     assert (
         isinstance(result.exception, ValidationError)
-        or "extra fields not permitted" in result.output
+        or "extra fields not permitted" in result.stderr
     ), traceback.format_exception(
         type(result.exception),
         result.exception,
@@ -64,7 +64,7 @@ def test_set_get_validation(runner: Runner, mlem_project):
     assert result.exit_code == 1
     assert (
         isinstance(result.exception, MlemError)
-        or "[name] should contain at least one dot" in result.output
+        or "[name] should contain at least one dot" in result.stderr
     ), traceback.format_exception(
         type(result.exception),
         result.exception,

diff --git a/tests/cli/test_deployment.py b/tests/cli/test_deployment.py
@@ -84,7 +84,11 @@ def test_deploy_create_new(
     result = runner.invoke(
         f"deploy run {path} -m {model_meta_saved_single.loc.uri} -t {mock_env_path} -c param=aaa".split()
     )
-    assert result.exit_code == 0, result.output
+    assert result.exit_code == 0, (
+        result.stdout,
+        result.stderr,
+        result.exception,
+    )
     assert os.path.isfile(path + MLEM_EXT)
     meta = load_meta(path)
     assert isinstance(meta, MlemDeploymentMock)
@@ -94,7 +98,11 @@ def test_deploy_create_new(
 
 def test_deploy_create_existing(runner: Runner, mock_deploy_path):
     result = runner.invoke(f"deploy run {mock_deploy_path}".split())
-    assert result.exit_code == 0, result.output
+    assert result.exit_code == 0, (
+        result.stdout,
+        result.stderr,
+        result.exception,
+    )
     meta = load_meta(mock_deploy_path)
     assert isinstance(meta, MlemDeploymentMock)
     assert meta.param == "bbb"
@@ -103,13 +111,21 @@ def test_deploy_create_existing(runner: Runner, mock_deploy_path):
 
 def test_deploy_status(runner: Runner, mock_deploy_path):
     result = runner.invoke(f"deploy status {mock_deploy_path}".split())
-    assert result.exit_code == 0, result.output
-    assert result.output.strip() == DeployStatus.NOT_DEPLOYED.value
+    assert result.exit_code == 0, (
+        result.stdout,
+        result.stderr,
+        result.exception,
+    )
+    assert result.stdout.strip() == DeployStatus.NOT_DEPLOYED.value
 
 
 def test_deploy_remove(runner: Runner, mock_deploy_path):
     result = runner.invoke(f"deploy remove {mock_deploy_path}".split())
-    assert result.exit_code == 0, result.output
+    assert result.exit_code == 0, (
+        result.stdout,
+        result.stderr,
+        result.exception,
+    )
     meta = load_meta(mock_deploy_path)
     assert isinstance(meta, MlemDeploymentMock)
     assert meta.status == DeployStatus.STOPPED
@@ -126,7 +142,11 @@ def test_deploy_apply(
     result = runner.invoke(
         f"deploy apply {mock_deploy_path} {data_path} -o {path}".split()
     )
-    assert result.exit_code == 0, result.output
+    assert result.exit_code == 0, (
+        result.stdout,
+        result.stderr,
+        result.exception,
+    )
     meta = load_meta(mock_deploy_path)
     assert isinstance(meta, MlemDeploymentMock)
     assert meta.status == DeployStatus.NOT_DEPLOYED

diff --git a/tests/cli/test_import_path.py b/tests/cli/test_import_path.py
@@ -26,7 +26,11 @@ def test_import_model_pickle_copy(
     result = runner.invoke(
         ["import", path, out_path, "--type", type_, "--copy"],
     )
-    assert result.exit_code == 0, (result.output, result.exception)
+    assert result.exit_code == 0, (
+        result.stdout,
+        result.stderr,
+        result.exception,
+    )
 
     loaded = load(out_path)
     loaded.predict(train)