From 8bc2daa236f23156bb6bb10861a2c4a3d280f6f2 Mon Sep 17 00:00:00 2001 From: Jimmy Kirk Date: Sun, 16 Aug 2026 17:39:42 -0500 Subject: [PATCH] refactor(diagrams): apply the large-graph layout policy at the render point Follow-up to #2, addressing the review: the helper had to be called by every builder, so a new diagram type would silently miss it. 'render_dot_to_svg' is already the codebase's single point for turning a graphviz object into SVG -- its docstring says every render must go through it, and it already carries one cross-cutting guarantee (a render failure must never break the embedding page). The size policy belongs in the same place, so this moves it there and drops the seven per-builder calls. Deciding centrally means the exceptions have to be explicit rather than implied by which builders remembered to opt in: * graphs that already chose a force-directed engine are left alone ('render_calls' uses fdp, 'render_heatmap' sfdp) * graphs whose ranks carry meaning opt out by name via RANKED_BY_MEANING -- currently the Hasse diagram, where the ranks are the partial order and a force-directed layout would destroy what it exists to show Also fixes a real bug in the merged version: it set overlap="prism", which is exactly the unsupported default 'render_dot_to_svg' warns about -- prism needs a triangulation library many graphviz builds omit, and would have failed the same way the docstring describes. Now overlap="false", matching 'render_calls'. Tests: the builder-level cases now exercise fit_layout_to_size directly, plus new cases for the two opt-outs and one asserting prism is never requested. FakeDot grows the attributes the render path now inspects. 26 passed against 23 on main for the same selection; the 17 errors are pre-existing here (fixtures shelling out to cabal). --- cli/lib/src/pb/lib/diagram_builder.py | 40 ++++++++++++++++------ cli/lib/tests/test_core_diagram_builder.py | 39 ++++++++++++++++----- cli/pipeline/src/pb/pipeline/diagrams.py | 12 ++++++- cli/pipeline/tests/test_shell_diagrams.py | 9 +++++ 4 files changed, 80 insertions(+), 20 deletions(-) diff --git a/cli/lib/src/pb/lib/diagram_builder.py b/cli/lib/src/pb/lib/diagram_builder.py index cf79ad68..80ec2f9d 100644 --- a/cli/lib/src/pb/lib/diagram_builder.py +++ b/cli/lib/src/pb/lib/diagram_builder.py @@ -64,13 +64,24 @@ def apply_defaults(dot, node_extra=None, edge_extra=None) -> None: # `dot`'s ranking is more informative than a force-directed blob. LARGE_GRAPH_NODES = 60 +# Graphs whose ranked layout carries meaning rather than being incidental. A +# Hasse diagram's ranks ARE its partial order, so re-laying it out +# force-directed would destroy the thing it exists to show. Keyed on the +# graph's own `name`, which every builder here already sets. +RANKED_BY_MEANING = frozenset({"lattice"}) + def _node_count(dot) -> int: return sum(1 for line in dot.body if "->" not in line and "[" in line) -def _relayout_if_large(dot): - """Re-lays out a graph that has outgrown `dot`, in place. +def fit_layout_to_size(dot): + """Re-lay out a graph that has outgrown `dot`, in place. + + Applied once, centrally, by `pb.pipeline.diagrams.render_dot_to_svg` -- + the single point every graphviz render in the codebase goes through. It + is deliberately not called by the individual builders: a new diagram type + should get this for free rather than having to remember to opt in. `dot` ranks nodes, which is right for a call graph and degenerate for the bipartite ones: once hundreds of nodes share a rank the drawing is a @@ -82,16 +93,23 @@ def _relayout_if_large(dot): `sfdp` is designed for this shape and gives 1.29, 1.27 and 1.31 respectively on the same data. `render_calls` already avoids the problem - by using `fdp`, and lands at 1.49. + by using `fdp` and lands at 1.49 -- graphs that already chose a + force-directed engine are left alone here. `splines="ortho"` has to go when switching: ortho routing aborts under sfdp with `multispline.c:153: findMap: Assertion 'ip' failed`, so straight - edges are used instead. + edges are used instead. `overlap` is `"false"`, not `"prism"` -- prism + needs a triangulation library many graphviz builds omit, exactly the + unsupported-default trap `render_dot_to_svg` documents. """ + if getattr(dot, "engine", "dot") != "dot": + return dot + if getattr(dot, "name", None) in RANKED_BY_MEANING: + return dot if _node_count(dot) < LARGE_GRAPH_NODES: return dot dot.engine = "sfdp" - dot.attr(splines="line", overlap="prism", sep="+8") + dot.attr(splines="line", overlap="false", sep="+8") return dot @@ -143,7 +161,7 @@ def render_inheritance( URL=f"pb://object/{root}#kind={kind}", ) - return _relayout_if_large(dot) + return dot def render_calls( @@ -240,7 +258,7 @@ def render_dw_tables( for dw, tbl in rows: dot.edge(f"dw_{dw}", f"t_{tbl}", color="#56A85D88", arrowsize="0.5", penwidth="0.7") - return _relayout_if_large(dot) + return dot def render_heatmap( @@ -346,7 +364,7 @@ def render_sql_lineage( if not rows: dot.node("empty", label="No PowerScript SQL statements found", shape="plaintext", fontcolor="#5c5f72") - return _relayout_if_large(dot) + return dot def render_table_lineage( @@ -387,7 +405,7 @@ def render_table_lineage( if not rows: dot.node("empty", label=f"No references found for table: {table_name}", shape="plaintext", fontcolor="#5c5f72") - return _relayout_if_large(dot) + return dot def render_proc_tables( @@ -429,7 +447,7 @@ def render_proc_tables( msg += f" for table: {table_name}" dot.node("empty", label=msg, shape="plaintext", fontcolor="#5c5f72") - return _relayout_if_large(dot) + return dot _FK_CATEGORY_STYLE = { @@ -471,7 +489,7 @@ def render_fk_graph( if not edges: dot.node("empty", label="No FK relationships found", shape="plaintext", fontcolor="#5c5f72") - return _relayout_if_large(dot) + return dot def render_lattice( diff --git a/cli/lib/tests/test_core_diagram_builder.py b/cli/lib/tests/test_core_diagram_builder.py index 57442980..b2e31d76 100644 --- a/cli/lib/tests/test_core_diagram_builder.py +++ b/cli/lib/tests/test_core_diagram_builder.py @@ -1,5 +1,6 @@ from pb.lib.diagram_builder import ( LARGE_GRAPH_NODES, + fit_layout_to_size, complexity_color, kind_color, render_calls, @@ -7,6 +8,7 @@ render_fk_graph, render_heatmap, render_inheritance, + render_lattice, render_proc_tables, render_sql_lineage, render_table_lineage, @@ -103,25 +105,46 @@ def _big_inheritance(n): return render_inheritance(edges, {}, None) +def test_builders_do_not_relayout_themselves(): + # The policy is applied centrally by render_dot_to_svg, not per builder, + # so a new diagram type gets it without opting in. + dot = _big_inheritance(LARGE_GRAPH_NODES + 10) + assert dot.engine == "dot" + + def test_small_graph_keeps_dot_and_ortho_splines(): - dot = _big_inheritance(5) + dot = fit_layout_to_size(_big_inheritance(5)) assert dot.engine == "dot" - assert 'splines=ortho' in "".join(dot.body) + assert "splines=ortho" in "".join(dot.body) def test_large_graph_switches_to_sfdp(): - dot = _big_inheritance(LARGE_GRAPH_NODES + 10) + dot = fit_layout_to_size(_big_inheritance(LARGE_GRAPH_NODES + 10)) assert dot.engine == "sfdp" def test_large_graph_drops_ortho_splines(): # ortho routing aborts under sfdp, so it must not survive the switch. - body = "".join(_big_inheritance(LARGE_GRAPH_NODES + 10).body) - assert 'splines=line' in body + body = "".join(fit_layout_to_size(_big_inheritance(LARGE_GRAPH_NODES + 10)).body) + assert "splines=line" in body -def test_calls_graph_is_left_alone(): - # render_calls already uses fdp and is not one of the degenerate shapes. +def test_never_requests_prism_overlap(): + # prism needs a triangulation library many graphviz builds omit. + body = "".join(fit_layout_to_size(_big_inheritance(LARGE_GRAPH_NODES + 10)).body) + assert "prism" not in body + + +def test_force_directed_graph_is_left_alone(): + # render_calls already uses fdp; re-laying it out would be wrong. nodes = {f"o_{i}" for i in range(LARGE_GRAPH_NODES + 10)} - dot = render_calls(nodes, [], {}, "o_0") + dot = fit_layout_to_size(render_calls(nodes, [], {}, "o_0")) assert dot.engine == "fdp" + + +def test_ranked_by_meaning_graph_is_left_alone(): + # A Hasse diagram's ranks are its partial order, not incidental layout. + concepts = [{"extent": [f"w{i}"], "intent": [f"t{i}"]} for i in range(LARGE_GRAPH_NODES + 10)] + covers = [{"upper": i + 1, "lower": i} for i in range(len(concepts) - 1)] + dot = fit_layout_to_size(render_lattice(concepts, covers)) + assert dot.engine == "dot" diff --git a/cli/pipeline/src/pb/pipeline/diagrams.py b/cli/pipeline/src/pb/pipeline/diagrams.py index d752d35d..a3e3e843 100644 --- a/cli/pipeline/src/pb/pipeline/diagrams.py +++ b/cli/pipeline/src/pb/pipeline/diagrams.py @@ -11,6 +11,7 @@ import graphviz import networkx as nx from pb.lib.diagram_builder import ( + fit_layout_to_size, render_calls, render_dw_tables, render_fk_graph, @@ -67,6 +68,15 @@ def render_dot_to_svg(dot) -> str: report a clear 503 instead of silently hiding a missing dependency behind a placeholder image. + It also applies `pb.lib.diagram_builder.fit_layout_to_size`, which + switches a graph that has outgrown `dot`'s ranked layout onto `sfdp`. + That lives here rather than in the builders so a new diagram type gets it + without having to remember to ask: the size at which a ranked layout + degenerates is a property of graphviz, not of any one diagram. Builders + that need their ranks preserved (a Hasse diagram, where the ranks are the + partial order) say so via `RANKED_BY_MEANING`, and graphs that already + chose a force-directed engine are left alone. + Public (not `_`-prefixed): every graphviz.Digraph render in the codebase must go through this one guarantee. `pb.api.services.diagrams.get_cfg_diagram` builds its own `cfg_to_dot(...)` graph outside the `kind`-based builders @@ -76,7 +86,7 @@ def render_dot_to_svg(dot) -> str: took that endpoint down uncaught. """ try: - return dot.pipe(format="svg").decode("utf-8") + return fit_layout_to_size(dot).pipe(format="svg").decode("utf-8") except graphviz.backend.execute.ExecutableNotFound: raise except Exception: diff --git a/cli/pipeline/tests/test_shell_diagrams.py b/cli/pipeline/tests/test_shell_diagrams.py index 0ad40cd2..cc433fd1 100644 --- a/cli/pipeline/tests/test_shell_diagrams.py +++ b/cli/pipeline/tests/test_shell_diagrams.py @@ -324,6 +324,15 @@ def __init__(self, exc: Exception | None): """exc=None means .pipe() succeeds; otherwise every call raises exc.""" self.exc = exc self.calls = 0 + # render_dot_to_svg also runs the graph through fit_layout_to_size, + # which inspects these. An empty body is below LARGE_GRAPH_NODES, so + # the layout is left alone and these tests stay about failure handling. + self.body: list[str] = [] + self.engine = "dot" + self.name = "fake" + + def attr(self, **_kwargs) -> None: + pass def pipe(self, format: str) -> bytes: # noqa: A002 self.calls += 1