slightly improve error message (#2087)

Author: jeromedockes

skrub/_data_ops/_data_ops.py

@@ -733,9 +733,7 @@ def _repr_html_(self):
             graph = strip_xml_declaration(graph)
             has_graph = True
         except Exception:
-            graph = (
-                "Please install Pydot and GraphViz to display the computation graph."
-            )
+            graph = f"<p>{_utils.graphviz_error_message(html=True)}</p>"
             has_graph = False
         impl = self._skrub_impl
         if impl.preview_if_available() is NULL:

skrub/_data_ops/_inspection.py

@@ -145,7 +145,7 @@ def _make_full_report(
     overwrite=False,
     title=None,
 ):
-    _check_graphviz()
+    _utils.check_graphviz()
     output_dir = _get_output_dir(output_dir, overwrite)
     try:
         # TODO dump report in callback instead of evaluating full DataOps plan
@@ -330,28 +330,10 @@ def _dot_id(n):
     return f"node_{n}"
 
 
-def _has_graphviz():
-    try:
-        import pydot
-
-        g = pydot.Dot()
-        g.add_node(pydot.Node("node 0"))
-        g.create_svg()
-        return True
-    except Exception:
-        return False
-
-
-def _check_graphviz():
-    if _has_graphviz():
-        return
-    raise ImportError("Please install pydot and graphviz to draw data_op graphs.")
-
-
 def draw_data_op_graph(data_op, url=None, direction="TB"):
     # TODO if pydot or graphviz not available fallback on some other plotting
     # solution eg a vendored copy of mermaid? outputting html instead of svg
-    _check_graphviz()
+    _utils.check_graphviz()
 
     import pydot
 

skrub/_data_ops/_utils.py

@@ -125,3 +125,44 @@ def rename(name):
         return numbered
 
     return rename
+
+
+def graphviz_error_message(html=False):
+    if html:
+        return """\
+To display the DataOp graph, please install Pydot and Graphviz
+and make sure the dot command is in your <code>$PATH</code>.<br/>
+You may also need to run <code>dot -c</code> in bash or powershell
+to rebuild the plugin cache of Graphviz.<br/>
+Graphviz must be installed using your system's
+package manager rather than pip.<br/>
+<a href="https://pypi.org/project/pydot/">Pydot documentation</a><br/>
+<a href="https://graphviz.org/download/">Graphviz installation instructions</a><br/>
+"""
+    else:
+        return """\
+To display the DataOp graph,
+please install Pydot and Graphviz and make sure the 'dot' command is in your $PATH.
+You may also need to run 'dot -c' in bash or powershell
+to rebuild the plugin cache of Graphviz.
+Graphviz must be installed using your system's package manager rather than pip.
+https://pypi.org/project/pydot/
+https://graphviz.org/download/"""
+
+
+def has_graphviz():
+    try:
+        import pydot
+
+        g = pydot.Dot()
+        g.add_node(pydot.Node("node 0"))
+        g.create_svg()
+        return True
+    except Exception:
+        return False
+
+
+def check_graphviz():
+    if has_graphviz():
+        return
+    raise RuntimeError(graphviz_error_message())

skrub/_data_ops/tests/test_estimators.py

@@ -36,8 +36,8 @@
 from sklearn.utils.validation import check_is_fitted
 
 import skrub
+from skrub._data_ops import _utils
 from skrub._data_ops._estimator import _SharedDict
-from skrub._data_ops._inspection import _has_graphviz
 
 #
 # testing utils
@@ -1157,7 +1157,7 @@ def test_plot_results(randomized_search_backend):
         assert (fig is None) == (not plotly_installed)
 
 
-@pytest.mark.skipif(not _has_graphviz(), reason="full report requires graphviz")
+@pytest.mark.skipif(not _utils.has_graphviz(), reason="full report requires graphviz")
 def test_report(tmp_path):
     data_op, data = get_data_op_and_data("simple")
     pipe = data_op.skb.make_learner()

skrub/_data_ops/tests/test_inspection.py

@@ -11,10 +11,10 @@
 
 import skrub
 from skrub import datasets
-from skrub._data_ops import _inspection
+from skrub._data_ops import _inspection, _utils
 
 
-@pytest.mark.skipif(not _inspection._has_graphviz(), reason="report requires graphviz")
+@pytest.mark.skipif(not _utils.has_graphviz(), reason="report requires graphviz")
 def test_output_dir(tmp_path):
     e = skrub.X()
     assert e.skb.full_report(open=False)["report_path"].is_relative_to(
@@ -34,7 +34,7 @@ def test_output_dir(tmp_path):
     )
 
 
-@pytest.mark.skipif(not _inspection._has_graphviz(), reason="report requires graphviz")
+@pytest.mark.skipif(not _utils.has_graphviz(), reason="report requires graphviz")
 def test_full_report():
     # smoke test for the full report
     # TODO we should have a private function that returns the JSON data so we
@@ -60,7 +60,7 @@ def test_full_report():
     assert "This step did not run" in (out / "node_4.html").read_text("utf-8")
 
 
-@pytest.mark.skipif(not _inspection._has_graphviz(), reason="report requires graphviz")
+@pytest.mark.skipif(not _utils.has_graphviz(), reason="report requires graphviz")
 def test_full_report_title():
     # TODO we should have a private function that returns the JSON data so we
     #      can check the content before rendering with jinja
@@ -71,14 +71,14 @@ def test_full_report_title():
     assert title in report["report_path"].read_text("utf-8")
 
 
-@pytest.mark.skipif(not _inspection._has_graphviz(), reason="report requires graphviz")
+@pytest.mark.skipif(not _utils.has_graphviz(), reason="report requires graphviz")
 def test_preview_subsample():
     X = datasets.fetch_employee_salaries().X
     preview = skrub.X(X).skb.subsample(n=3)._repr_html_()
     assert "subsample" in preview
 
 
-@pytest.mark.skipif(not _inspection._has_graphviz(), reason="report requires graphviz")
+@pytest.mark.skipif(not _utils.has_graphviz(), reason="report requires graphviz")
 def test_full_report_failed_apply():
     # Somewhat contrived example for the corner case where an Apply does not
     # have an easily identifiable estimator.
@@ -96,7 +96,7 @@ def test_full_report_failed_apply():
     assert report["error"] is not None
 
 
-@pytest.mark.skipif(not _inspection._has_graphviz(), reason="report requires graphviz")
+@pytest.mark.skipif(not _utils.has_graphviz(), reason="report requires graphviz")
 def test_report_fit_mode():
     # non-regression: in fit mode the individual node pages used to show the
     # dataop itself as the output instead of the result of fit_transform for
@@ -132,15 +132,15 @@ def test_report_score_mode_with_scoring():
         learner.report(environment={}, mode="score")
 
 
-@pytest.mark.skipif(not _inspection._has_graphviz(), reason="report requires graphviz")
+@pytest.mark.skipif(not _utils.has_graphviz(), reason="report requires graphviz")
 def test_full_report_open(monkeypatch):
     mock = Mock()
     monkeypatch.setattr(webbrowser, "open", mock)
     skrub.as_data_op(0).skb.full_report()
     mock.assert_called_once()
 
 
-@pytest.mark.skipif(not _inspection._has_graphviz(), reason="report requires graphviz")
+@pytest.mark.skipif(not _utils.has_graphviz(), reason="report requires graphviz")
 def test_draw_graph():
     g = skrub.as_data_op(0).skb.draw_graph()
     assert repr(g) == "<GraphDrawing: use .open() to display>"
@@ -150,7 +150,7 @@ def test_draw_graph():
     assert g._repr_png_().startswith(b"\x89PNG")
 
 
-@pytest.mark.skipif(not _inspection._has_graphviz(), reason="report requires graphviz")
+@pytest.mark.skipif(not _utils.has_graphviz(), reason="report requires graphviz")
 def test_svg_anchor_google_colab(monkeypatch):
     """non-regression test for #1589"""
     monkeypatch.setitem(sys.modules, "google.colab", None)
@@ -168,18 +168,18 @@ def _import(name, *args, **kwargs):
         return builtin_import(name, *args, **kwargs)
 
     monkeypatch.setattr(builtins, "__import__", _import)
-    with pytest.raises(ImportError, match="Please install pydot"):
+    with pytest.raises(RuntimeError, match="please install Pydot and Graphviz"):
         skrub.as_data_op(0).skb.draw_graph()
 
 
 def test_no_graphviz(monkeypatch):
     pydot = pytest.importorskip("pydot")
     monkeypatch.setattr(pydot.Dot, "create_svg", Mock(side_effect=Exception()))
-    with pytest.raises(ImportError, match="Please install pydot and graphviz"):
+    with pytest.raises(RuntimeError, match="please install Pydot and Graphviz"):
         skrub.as_data_op(0).skb.draw_graph()
 
 
-@pytest.mark.skipif(not _inspection._has_graphviz(), reason="report requires graphviz")
+@pytest.mark.skipif(not _utils.has_graphviz(), reason="report requires graphviz")
 def test_draw_graph_open(monkeypatch):
     mock = Mock()
     monkeypatch.setattr(_inspection, "open_in_browser", mock)

skrub/tests/test_data_ops_stack_description.py

@@ -5,7 +5,7 @@
 from sklearn.preprocessing import FunctionTransformer
 
 import skrub
-from skrub._data_ops import _inspection
+from skrub._data_ops import _utils
 
 # Those tests have to be outside of the `_data_ops` module (and thus out of
 # _data_ops/tests/, as we place the tests inside the package), because to
@@ -81,7 +81,7 @@ def test_apply_eval_failure(eval_data_op):
         eval_data_op(e, {"a": 1.0, "b": 2.0})
 
 
-@pytest.mark.skipif(not _inspection._has_graphviz(), reason="report requires graphviz")
+@pytest.mark.skipif(not _utils.has_graphviz(), reason="report requires graphviz")
 def test_escaping_characters():
     # fmt: off
     out = skrub.as_data_op("") + "\n" + '"' + "\t" +'\\ ' \