From 2b66323873dcd77f96df68f6d611346afd53c212 Mon Sep 17 00:00:00 2001 From: Dimitri Yatsenko Date: Tue, 13 Oct 2026 15:33:09 -0500 Subject: [PATCH] fix(diagram): make rendered diagrams byte-reproducible across runs Python salts string hashes per process, so iterating a set of table names gives every interpreter a different order. Several places on the rendering path did that. The drawing graph was built with networkx `subgraph()`, which iterates a node subset smaller than half the graph in set order however the nodes were passed in. The entity clusters were emitted by looping over a set of part names. And `to_pydot()` wrote each node's primary-key set into the dot source with `str()`. Two identical runs therefore emitted nodes, edges and clusters in different orders: a committed SVG changed on every regeneration, and a drift check that compares a regenerated figure with the committed one could not tell a real change from reshuffling. The drawing graph is now built directly, with nodes and edges in order of full table name and the primary key stored sorted, and every other set iterated while rendering is sorted first. The notation is unchanged; only the emission order is now fixed, so committed figures need one regeneration. A new test renders the same diagrams in separate interpreters with different PYTHONHASHSEED values and asserts that the dot source, SVG and Mermaid output are identical, on both backends. Closes #1551. --- src/datajoint/diagram.py | 45 ++-- .../integration/test_diagram_reproducible.py | 202 ++++++++++++++++++ 2 files changed, 234 insertions(+), 13 deletions(-) create mode 100644 tests/integration/test_diagram_reproducible.py diff --git a/src/datajoint/diagram.py b/src/datajoint/diagram.py index 6ff60d061..59670bbee 100644 --- a/src/datajoint/diagram.py +++ b/src/datajoint/diagram.py @@ -1206,8 +1206,12 @@ def _make_graph(self) -> nx.MultiDiGraph: # primary-key attribute of their own beyond what they inherit through # foreign keys. These are drawn with an underlined label. ("Schema # dimension" / "axis" is the documented term for such a table.) - # Filter nodes_to_show to only include nodes that exist in the graph - valid_nodes = self.nodes_to_show.intersection(set(self.nodes())) + # Filter nodes_to_show to only include nodes that exist in the graph. + # Sorted, because the rendered output follows this order: table names + # are strings, whose hashes are salted per process, so iterating the set + # would emit nodes, edges and clusters in a different order on every + # run (#1551). + valid_nodes = sorted(self.nodes_to_show.intersection(set(self.nodes()))) for name in valid_nodes: foreign_attributes = set( attr for p in self.in_edges(name, data=True) for attr in p[2]["attr_map"] if p[2]["primary"] @@ -1215,12 +1219,25 @@ def _make_graph(self) -> nx.MultiDiGraph: self.nodes[name]["introduces_dimension"] = ( "primary_key" in self.nodes[name] and foreign_attributes < self.nodes[name]["primary_key"] ) - # construct subgraph and rename nodes to class names. A MultiDiGraph is - # used so parallel FKs between the same pair of tables (e.g. a renamed - # FK alongside a direct one) survive to the renderer as distinct edges. - # Subgraph a plain MultiDiGraph copy — subgraphing/copying Diagram itself - # would try to instantiate Diagram() (which requires a `source`). - graph = nx.MultiDiGraph(nx.MultiDiGraph(self).subgraph(valid_nodes)) + # Construct the drawing graph and rename nodes to class names. A + # MultiDiGraph is used so parallel FKs between the same pair of tables + # (e.g. a renamed FK alongside a direct one) survive to the renderer as + # distinct edges. It is built here in sorted order rather than with + # subgraph(): a subgraph view holding fewer than half of the nodes + # iterates them in set order. to_pydot() writes every node attribute + # into the dot source with str(), so the primary-key set is stored sorted. + graph = nx.MultiDiGraph() + for name in valid_nodes: + attrs = dict(self.nodes[name]) + if "primary_key" in attrs: + attrs["primary_key"] = sorted(attrs["primary_key"]) + graph.add_node(name, **attrs) + graph.add_edges_from( + sorted( + ((u, v, k, d) for u, v, k, d in self.edges(valid_nodes, keys=True, data=True) if v in graph), + key=lambda edge: (edge[0], edge[1], str(edge[2])), + ) + ) nx.set_node_attributes(graph, name="node_type", values={n: _get_tier(n) for n in graph}) # relabel nodes to class names mapping = {node: lookup_class_name(node, self.context) or node for node in graph.nodes()} @@ -1270,7 +1287,7 @@ def _apply_collapse(self, graph: nx.MultiDiGraph) -> tuple[nx.MultiDiGraph, dict # Group collapsed nodes by schema collapsed_by_schema = {} # schema_name -> list of class_names - for class_name in nodes_to_collapse: + for class_name in sorted(nodes_to_collapse): full_name = class_to_full.get(class_name) if full_name: schema_name, _ = self._connection.adapter.split_full_table_name(full_name) @@ -1501,7 +1518,7 @@ def make_dot(self, theme=None): schema_map = {} # class_name -> schema_name schema_modules = {} # schema_name -> set of module names - for full_name in self.nodes_to_show: + for full_name in sorted(self.nodes_to_show): schema_name, _ = self._connection.adapter.split_full_table_name(full_name) if schema_name: class_name = lookup_class_name(full_name, self.context) or full_name @@ -1745,9 +1762,11 @@ def _master_of(part_stripped): fontname="Helvetica", # schema label in a sans font, not Graphviz's Times default ) node_by_name = {n.get_name().strip('"'): n for n in nodes} - # masters in this schema that have at least one part present + # masters in this schema that have at least one part present, + # in sorted order so the entity clusters are emitted in a fixed + # order (#1551) masters_here = {} - for pn in part_names: + for pn in sorted(part_names): mn = part_master[pn] if pn in node_by_name and mn in node_by_name: masters_here.setdefault(mn, []).append(pn) @@ -1870,7 +1889,7 @@ def make_mermaid(self) -> str: schema_map = {} # class_name -> schema_name schema_modules = {} # schema_name -> set of module names - for full_name in self.nodes_to_show: + for full_name in sorted(self.nodes_to_show): schema_name, _ = self._connection.adapter.split_full_table_name(full_name) if schema_name: class_name = lookup_class_name(full_name, self.context) or full_name diff --git a/tests/integration/test_diagram_reproducible.py b/tests/integration/test_diagram_reproducible.py new file mode 100644 index 000000000..3eb139941 --- /dev/null +++ b/tests/integration/test_diagram_reproducible.py @@ -0,0 +1,202 @@ +""" +Byte-reproducibility of rendered diagrams across processes (#1551). + +Python salts ``str.__hash__`` per process (PEP 456), so iterating a set of table +names yields a different order in every interpreter. Wherever such an order +reaches the renderer, two identical runs emit nodes, edges and clusters in a +different sequence and the output differs byte for byte, although the picture +is the same. That defeats committing generated figures and checking them for +drift. An in-process test cannot catch this, so the same diagram is rendered in +separate interpreters with different ``PYTHONHASHSEED`` values and compared. +""" + +import json +import os +import subprocess +import sys +import time + +import pytest + +import datajoint as dj + +# Declares a pipeline with several master-part groups and renders it three ways: +# the whole schema, a neighborhood that holds fewer than half of the loaded +# tables (networkx then iterates its subgraph in set order), and a partly +# collapsed diagram. Writes the dot source, SVG and Mermaid of each as JSON. +_SCRIPT = ''' +import json +import sys + +import datajoint as dj + +cfg = json.loads(sys.argv[1]) +dj.config["database.backend"] = cfg["backend"] +dj.config["database.host"] = cfg["host"] +dj.config["database.port"] = cfg["port"] +dj.config["database.user"] = cfg["user"] +dj.config["database.password"] = cfg["password"] + +schema = dj.Schema(cfg["schema"]) + + +@schema +class Subject(dj.Manual): + definition = """ + subject_id : int32 + """ + + +@schema +class Rig(dj.Lookup): + definition = """ + rig : varchar(16) + """ + + +@schema +class Session(dj.Manual): + definition = """ + -> Subject + session_id : int32 + --- + -> Rig + """ + + class Note(dj.Part): + definition = """ + -> master + note_id : int32 + """ + + class Stimulus(dj.Part): + definition = """ + -> master + stim_id : int32 + """ + + +@schema +class Scan(dj.Imported): + definition = """ + -> Session + """ + + class Field(dj.Part): + definition = """ + -> master + field_id : int32 + """ + + class Channel(dj.Part): + definition = """ + -> master + channel : int32 + """ + + +@schema +class Segmentation(dj.Computed): + definition = """ + -> Scan + """ + + class Mask(dj.Part): + definition = """ + -> master + -> Scan.Field + mask_id : int32 + """ + + +@schema +class Fluorescence(dj.Computed): + definition = """ + -> Segmentation + """ + + class Trace(dj.Part): + definition = """ + -> master + -> Segmentation.Mask + """ + + +@schema +class Pairing(dj.Manual): + definition = """ + -> Session + -> Session.proj(other_subject_id="subject_id", other_session_id="session_id") + """ + + +diagrams = { + "schema": dj.Diagram(schema), + "neighborhood": dj.Diagram(Scan) + 1, + "collapsed": dj.Diagram(schema).collapse() + dj.Diagram(Segmentation), +} +rendered = { + name: {"dot": d.make_dot().to_string(), "svg": d.svg_string(), "mermaid": d.make_mermaid()} + for name, d in diagrams.items() +} +with open(cfg["out"], "w") as f: + json.dump(rendered, f) +''' + +# Seeds chosen so that, before the fix, set iteration order differed between them. +_SEEDS = ("0", "1", "12345") + + +@pytest.fixture(scope="function") +def schema_name_by_backend(connection_by_backend, db_creds_by_backend): + backend = db_creds_by_backend["backend"] + test_id = str(int(time.time() * 1000))[-8:] + schema_name = f"djtest_repro_{backend}_{test_id}"[:64] + quoted = connection_by_backend.adapter.quote_identifier(schema_name) + if connection_by_backend.is_connected: + try: + connection_by_backend.query(f"DROP DATABASE IF EXISTS {quoted}") + except Exception: + pass + dj.Schema(schema_name, connection=connection_by_backend) + yield schema_name + if connection_by_backend.is_connected: + try: + connection_by_backend.query(f"DROP DATABASE IF EXISTS {quoted}") + except Exception: + pass + + +def _render(seed, cfg, tmp_path): + out = tmp_path / f"render_{seed}.json" + proc = subprocess.run( + [sys.executable, "-c", _SCRIPT, json.dumps(dict(cfg, out=str(out)))], + env=dict(os.environ, PYTHONHASHSEED=seed), + cwd=tmp_path, + capture_output=True, + text=True, + timeout=300, + ) + assert proc.returncode == 0, f"render with PYTHONHASHSEED={seed} failed:\n{proc.stderr}" + return json.loads(out.read_text()) + + +def test_render_is_independent_of_hash_seed(schema_name_by_backend, db_creds_by_backend, tmp_path): + host, port = db_creds_by_backend["host"].rsplit(":", 1) + cfg = dict( + backend=db_creds_by_backend["backend"], + host=host, + port=int(port), + user=db_creds_by_backend["user"], + password=db_creds_by_backend["password"], + schema=schema_name_by_backend, + ) + reference, *others = (_render(seed, cfg, tmp_path) for seed in _SEEDS) + mismatches = [ + f"{name} diagram, {fmt} output, PYTHONHASHSEED={_SEEDS[0]} vs {seed}" + for seed, rendered in zip(_SEEDS[1:], others) + for name, outputs in reference.items() + for fmt, text in outputs.items() + if rendered[name][fmt] != text + ] + assert not mismatches, "rendering depends on the hash seed:\n" + "\n".join(mismatches)