From 88061e1d3f37b5c96e8eb3c587b66b527c224451 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Mon, 21 Sep 2026 15:06:24 -0400 Subject: [PATCH 1/4] Drop the LNG_BOUNDARY_LEAF_REUSE escape hatch; setting it is now an error The flag existed to ask one question -- does refusing to reuse a node the root complex can reach lose anything? -- and that question is answered: held-out +173 conditioned, p<1e-4, false change 1437->1150 (deltasignal specs/018). A flag whose question is settled is a second code path nobody measures. Removing it outright would make a stale LNG_BOUNDARY_LEAF_REUSE=any in an environment or a benchmark script silently measure the default while the operator believed they were measuring the old mode -- exactly the silent-input bug class this project has hit four times. So the name is kept as a rejection: setting it to anything raises, naming the spec. The two mode tests are replaced by one that asserts that error for three values, including the two that used to be valid. Co-Authored-By: Claude Opus 5 (1M context) --- src/logic_network_generator.py | 27 +++++++++++++++------------ tests/test_boundary_leaf_reuse.py | 26 ++++++++------------------ 2 files changed, 23 insertions(+), 30 deletions(-) diff --git a/src/logic_network_generator.py b/src/logic_network_generator.py index 9656e0b..fbfa2ca 100755 --- a/src/logic_network_generator.py +++ b/src/logic_network_generator.py @@ -1712,11 +1712,18 @@ def _emit_boundary_decomposition_edges( # takes TP53's strongly connected component from 836 nodes to 56 and DSB's # from 1,127 to ~290 (deltasignal specs/018). Reachability is computed over # every edge emitted so far (bridges and depletion edges included). - # LNG_BOUNDARY_LEAF_REUSE=any restores the old behaviour. + # + # This is unconditional. The LNG_BOUNDARY_LEAF_REUSE escape hatch that + # restored the welding behaviour was removed once the question it existed + # to ask was answered (held-out +173, p<1e-4). Setting it is an error + # rather than a no-op, so a stale value in an environment or a script is + # not silently ignored while the run measures something else. from collections import defaultdict - reuse_mode = os.environ.get("LNG_BOUNDARY_LEAF_REUSE", "downstream_free") - if reuse_mode not in ("downstream_free", "any"): - raise ValueError(f"LNG_BOUNDARY_LEAF_REUSE={reuse_mode!r}: expected 'downstream_free' or 'any'") + if "LNG_BOUNDARY_LEAF_REUSE" in os.environ: + raise ValueError( + "LNG_BOUNDARY_LEAF_REUSE was removed: boundary leaves never reuse a node " + "the root complex can reach. See deltasignal specs/018-derived-edge-loops." + ) _succ: Dict[str, List[str]] = defaultdict(list) for e in pathway_logic_network_data: _succ[str(e.get("source_id"))].append(str(e.get("target_id"))) @@ -1739,14 +1746,10 @@ def _downstream_of(root_uuid: str) -> Set[str]: def _leaf_uuid(leaf_stid: str, root_uuid: str) -> str: candidates = stid_to_existing_uuids.get(leaf_stid, []) - if reuse_mode == "any": - if candidates: - return candidates[0] - else: - downstream = _downstream_of(root_uuid) - for cand in candidates: - if cand not in downstream: - return cand + downstream = _downstream_of(root_uuid) + for cand in candidates: + if cand not in downstream: + return cand if leaf_stid not in leaf_uuid_registry: leaf_uuid_registry[leaf_stid] = str(uuid.uuid4()) reactome_id_to_uuid[leaf_uuid_registry[leaf_stid]] = leaf_stid diff --git a/tests/test_boundary_leaf_reuse.py b/tests/test_boundary_leaf_reuse.py index 47a64cd..756e3df 100644 --- a/tests/test_boundary_leaf_reuse.py +++ b/tests/test_boundary_leaf_reuse.py @@ -5,7 +5,6 @@ v97 catalog; deltasignal specs/018). A produced node that is NOT downstream is a real feed-forward link (hasComponent with no reaction) and must still be reused -- severing those cost Mitotic G1 28 cases in the first version of this fix.""" -import os import pytest import src.neo4j_connector as nc import src.logic_network_generator as lng @@ -40,7 +39,6 @@ def assembly_sources(data): def test_produced_node_is_not_reused_as_a_boundary_leaf(stub, monkeypatch): - monkeypatch.delenv("LNG_BOUNDARY_LEAF_REUSE", raising=False) data, r2u = network() _emit_boundary_decomposition_edges(data, r2u) asm = assembly_sources(data) @@ -60,7 +58,6 @@ def test_produced_node_is_not_reused_as_a_boundary_leaf(stub, monkeypatch): def test_a_copy_reached_only_through_a_bridge_is_downstream_too(stub, monkeypatch): # P has two copies: u_Pout (reaction output, downstream of C) and u_Pin (an input copy whose # only incoming edge is a diagram bridge from u_Pout -- also downstream). Neither may be reused. - monkeypatch.delenv("LNG_BOUNDARY_LEAF_REUSE", raising=False) data, r2u = network() data.append({"source_id": "u_Pout", "target_id": "u_Pin", "pos_neg": "pos", "and_or": "and", "edge_type": "diagram_bridge", "stoichiometry": 1}) data.append({"source_id": "u_Pin", "target_id": "r3", "pos_neg": "pos", "and_or": "and", "edge_type": "input", "stoichiometry": 1}) @@ -75,7 +72,6 @@ def test_a_produced_copy_that_is_not_downstream_is_reused(stub, monkeypatch): # The Mitotic G1 shape: P's only copy is produced by a reaction the root complex C does NOT # reach (r2, fed by Q). Reusing it adds no cycle and keeps the feed-forward link # P -> (assembly) -> C that Reactome joins only by hasComponent. - monkeypatch.delenv("LNG_BOUNDARY_LEAF_REUSE", raising=False) data = [ {"source_id": "u_C", "target_id": "r1", "pos_neg": "pos", "and_or": "and", "edge_type": "input", "stoichiometry": 1}, {"source_id": "r1", "target_id": "u_W", "pos_neg": "pos", "and_or": "or", "edge_type": "output", "stoichiometry": 1}, @@ -93,7 +89,6 @@ def test_a_produced_copy_that_is_not_downstream_is_reused(stub, monkeypatch): def test_two_root_complexes_sharing_a_subunit_each_get_an_acyclic_leaf(stub, monkeypatch): # C reaches P's produced copy (u_Pout) -> C gets a fresh leaf. C2 does NOT reach it -> # C2 reuses u_Pout (a real feed-forward link). Neither assembly edge closes a cycle. - monkeypatch.delenv("LNG_BOUNDARY_LEAF_REUSE", raising=False) data, r2u = network() data.append({"source_id": "u_C2", "target_id": "r4", "pos_neg": "pos", "and_or": "and", "edge_type": "input", "stoichiometry": 1}) data.append({"source_id": "r4", "target_id": "u_Z", "pos_neg": "pos", "and_or": "or", "edge_type": "output", "stoichiometry": 1}) @@ -118,16 +113,11 @@ def reach(u): assert not (srcs & reach(cpx)) -def test_legacy_reuse_is_available_and_welds_the_cycle(stub, monkeypatch): - monkeypatch.setenv("LNG_BOUNDARY_LEAF_REUSE", "any") - data, r2u = network() - _emit_boundary_decomposition_edges(data, r2u) - asm = assembly_sources(data) - assert "u_Pout" in asm # the old behaviour: the produced copy is reused -> C -> r1 -> u_Pout -> C - - -def test_bad_mode_is_an_error(stub, monkeypatch): - monkeypatch.setenv("LNG_BOUNDARY_LEAF_REUSE", "unproduced") - data, r2u = network() - with pytest.raises(ValueError): - _emit_boundary_decomposition_edges(data, r2u) +def test_removed_flag_is_an_error_not_a_noop(stub, monkeypatch): + # The escape hatch was removed; a stale value must fail loudly rather than + # letting a run silently measure the default while claiming the old mode. + for val in ("any", "downstream_free", "unproduced"): + monkeypatch.setenv("LNG_BOUNDARY_LEAF_REUSE", val) + data, r2u = network() + with pytest.raises(ValueError, match="was removed"): + _emit_boundary_decomposition_edges(data, r2u) From e58bd8def81e6a263c6cb2a3f8fccbc7b3954131 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Mon, 21 Sep 2026 15:15:49 -0400 Subject: [PATCH 2/4] Variant-node sharing is now unconditional; close the cache-fingerprint gap Phase 1 keyed entity uuids on the VIRTUAL reaction, so a participant common to every variant of a Reactome reaction was photocopied once per set-member combination. Keying on the Reactome reaction instead is what the data says; the flag existed only to measure that the correction costs nothing. Measured on the 92-pathway catalog (deltasignal specs/020): nodes 108,229 -> 70,738 (-34.6%) edges 285,497 -> 224,189 (-21.5%) pathways gaining cyclic nodes 0 (does not undo specs/018) predictions changed 29 of 24,100 (0.12%, 3 pathways) held-out net, both axes 0 (zero discordant cases) false change 1,150 -> 1,150 Verified that the new default reproduces the measured catalog exactly: HDR 799 nodes / 3,123 edges and DNA Repair 4,756 / 14,487, identical to the flagged run. Setting LNG_SHARE_VARIANT_NODES now raises rather than being ignored, so a stale value in a benchmark script cannot silently measure something else. Also adds LNG_BOUNDARY_EXPANSION, LNG_COMPOSITION_EDGES and LNG_EMIT_ONE_SIDED to _FINGERPRINTED_ENV. All three change emitted content, and the list comment already promised that flipping any fingerprinted flag invalidates stale caches "silently reusing a network built under the old setting" -- which is exactly what these three could do. Docstring fix: the BCDX2 example is 6 reactions, not 5. Co-Authored-By: Claude Opus 5 (1M context) --- src/logic_network_generator.py | 54 +++++++++++++++++++----------- src/pathway_generator.py | 3 ++ tests/test_variant_node_sharing.py | 49 ++++++++++++++++++--------- 3 files changed, 70 insertions(+), 36 deletions(-) diff --git a/src/logic_network_generator.py b/src/logic_network_generator.py index 9656e0b..b041ba8 100755 --- a/src/logic_network_generator.py +++ b/src/logic_network_generator.py @@ -489,27 +489,35 @@ def _register_phase1( terminal_output_eids: Set[str], terminal_output_uuid_cache: Dict[str, str], vr_to_reaction: Optional[Dict[str, str]] = None, - share_variants: bool = False, ) -> Dict[str, int]: - """Phase 1 of UUID assignment: one key per (entity, virtual reaction, role). + """Phase 1 of UUID assignment: one key per (entity, REACTOME reaction, role). A Reactome reaction with EntitySet participants becomes several VIRTUAL reactions, one per member combination, and every one of them registers its own UUID for every participant. An entity common to all variants -- BCDX2 in HDR's strand-invasion reactions -- therefore exists as one node PER VARIANT: - 33 copies from 5 reactions, each a full copy with the same downstream edges. + 33 copies from 6 reactions, each a full copy with the same downstream edges. Nothing downstream can tell them apart, and any solver rule over the copies is wrong in one direction or the other: min over copies caps the container by whichever copy a perturbation missed (missed change); max lets one elevated copy lift it (false change). Both were measured (deltasignal specs/016): -83 and -117 held-out. - `share_variants` collapses exactly that: the SAME entity in the SAME role - across the variants of ONE Reactome reaction shares one UUID. Variants that - differ in WHICH set member they use have different entity ids and are not - conflated. Copies across DIFFERENT reactions stay positional, as designed - (bridging those was measured harmful four times). Boundary entities already - share per stId through their caches and are left to them. + So this keys on the REACTOME reaction, not the virtual one: the SAME entity + in the SAME role across the variants of ONE Reactome reaction gets one UUID. + Variants that differ in WHICH set member they use have different entity ids + and are not conflated. Copies across DIFFERENT reactions stay positional, as + designed (bridging those was measured harmful four times). Boundary entities + already share per stId through their caches and are left to them. + + This is unconditional; the LNG_SHARE_VARIANT_NODES flag that gated it was + removed once measured (deltasignal specs/020): nodes -34.6%, edges -21.5%, + no pathway gains cyclic nodes, and 29 of 24,100 predictions change with + held-out net ZERO on both ground-truth axes. Note most of the Phase-1 + photocopying is already undone downstream -- Phase 2 unions producer-output + with consumer-input across variant pairs, and the boundary caches share per + stId -- so only 1,431 of 29,655 (entity, reaction, role) contexts actually + carry more than one node, and catalyst/regulator contexts carry none. Returns a small stats dict for the log. """ @@ -519,7 +527,7 @@ def _register_phase1( unmapped = 0 for vr_uid, (input_ids, output_ids, *_) in vr_entities.items(): rxn = vr_to_reaction.get(str(vr_uid)) - if share_variants and rxn in (None, "", "nan", "None"): + if rxn in (None, "", "nan", "None"): # `reactome_id` read back from a cached CSV with no dtype turns a # missing id into the STRING "nan"; treated as a real key it would # collapse every such variant across reactions onto one uuid. @@ -530,7 +538,7 @@ def _register_phase1( ): for eid in ids: key = (eid, vr_uid, role) - if (share_variants and rxn not in (None, "", "nan", "None") + if (rxn not in (None, "", "nan", "None") and eid not in boundary_eids and key not in entity_uuid_registry): vkey = (eid, rxn, role) @@ -544,12 +552,11 @@ def _register_phase1( continue _register_entity_uuid(eid, vr_uid, role, entity_uuid_registry, boundary_eids, boundary_cache) - if share_variants: - logger.info(f"Variant-node sharing: {shared} (entity, reaction, role) registrations " - f"reused a sibling variant's UUID ({len(variant_cache)} distinct)") - if unmapped: - logger.warning(f"Variant-node sharing: {unmapped} virtual reactions have no Reactome " - f"reaction id and were registered per-variant (sharing is inconsistent for them)") + logger.info(f"Variant-node sharing: {shared} (entity, reaction, role) registrations " + f"reused a sibling variant's UUID ({len(variant_cache)} distinct)") + if unmapped: + logger.warning(f"Variant-node sharing: {unmapped} virtual reactions have no Reactome " + f"reaction id and were registered per-variant (sharing is inconsistent for them)") return {"shared": shared, "distinct": len(variant_cache), "unmapped": unmapped} @@ -2147,15 +2154,22 @@ def create_pathway_logic_network( # Each entity gets a unique UUID per (entity, reaction, role) triple. # No cross-role keys are created (unlike the old self-loop approach). # Boundary entities (root inputs / terminal outputs) share one UUID per stId. - # Under LNG_SHARE_VARIANT_NODES the same entity in the same role across the - # VARIANTS of one Reactome reaction shares one UUID (see _register_phase1). + # The same entity in the same role across the VARIANTS of one Reactome + # reaction shares one UUID (see _register_phase1). This was the + # LNG_SHARE_VARIANT_NODES flag; it is now unconditional, and setting the + # variable is an error rather than a no-op so a stale value in a script is + # not silently ignored. + if "LNG_SHARE_VARIANT_NODES" in os.environ: + raise ValueError( + "LNG_SHARE_VARIANT_NODES was removed: variant-node sharing is always on. " + "See deltasignal specs/020-variant-node-sharing." + ) _register_phase1( vr_entities, entity_uuid_registry, root_input_eids, root_input_uuid_cache, terminal_output_eids, terminal_output_uuid_cache, vr_to_reaction=dict(zip(reaction_id_map["uid"].astype(str), reaction_id_map["reactome_id"].astype(str))), - share_variants=os.environ.get("LNG_SHARE_VARIANT_NODES", "0") == "1", ) logger.debug(f"Phase 1 complete: {len(entity_uuid_registry)} registry entries") diff --git a/src/pathway_generator.py b/src/pathway_generator.py index 34f61ca..8b26591 100755 --- a/src/pathway_generator.py +++ b/src/pathway_generator.py @@ -36,6 +36,9 @@ "LNG_HANDOFF_HUB_MAX", "LNG_SET_MEMBERS_OR", "LNG_DIAGRAM_SET_MEMBER", + "LNG_BOUNDARY_EXPANSION", + "LNG_COMPOSITION_EDGES", + "LNG_EMIT_ONE_SIDED", # Determinism controls: node ids are uuid4 and several selections iterate # sets, so hash seeding changes emitted content (~5.8% of TP53 edges per # bin/create-pathways.py). A cache built unseeded is not comparable to a diff --git a/tests/test_variant_node_sharing.py b/tests/test_variant_node_sharing.py index 932bbc9..9b0ffa1 100644 --- a/tests/test_variant_node_sharing.py +++ b/tests/test_variant_node_sharing.py @@ -1,17 +1,23 @@ -"""LNG_SHARE_VARIANT_NODES: the same entity in the same role across the variants of +"""Variant-node sharing: the same entity in the same role across the variants of ONE Reactome reaction shares one UUID; across different reactions it does not. +Unconditional since deltasignal specs/020 (nodes -34.6%, edges -21.5%, no +pathway gains cyclic nodes, held-out net zero on both ground-truth axes). The +LNG_SHARE_VARIANT_NODES flag that gated it is gone, and setting it is an error. + Motivation (deltasignal specs/016): HDR's BCDX2 complex existed as 33 node copies -- one per variant reaction -- and no solver rule over copies is right: min over them measured -83 held-out, max -117. One node is the fix. """ +import pytest + from src.logic_network_generator import _register_phase1 -def phase1(vr_entities, vr_to_reaction, share, roots=frozenset(), terms=frozenset()): +def phase1(vr_entities, vr_to_reaction, roots=frozenset(), terms=frozenset()): reg = {} _register_phase1(vr_entities, reg, set(roots), {}, set(terms), {}, - vr_to_reaction=vr_to_reaction, share_variants=share) + vr_to_reaction=vr_to_reaction) return reg @@ -21,31 +27,42 @@ def phase1(vr_entities, vr_to_reaction, share, roots=frozenset(), terms=frozense V2R = {"v1": "R1", "v2": "R1", "v3": "R2"} -def test_off_every_variant_gets_its_own_uuid(): - reg = phase1(VR, V2R, share=False) - assert reg[("B", "v1", "output")] != reg[("B", "v2", "output")] - assert reg[("A", "v1", "input")] != reg[("A", "v2", "input")] +def test_removed_flag_is_an_error_not_a_noop(monkeypatch): + # Sharing is unconditional. A stale LNG_SHARE_VARIANT_NODES must fail loudly + # rather than let a run silently measure the default under the old name. + from src.logic_network_generator import create_pathway_logic_network + import inspect + src = inspect.getsource(create_pathway_logic_network) + assert "LNG_SHARE_VARIANT_NODES was removed" in src + assert 'os.environ.get("LNG_SHARE_VARIANT_NODES"' not in src + + +def test_sharing_is_on_with_no_flag_set(monkeypatch): + monkeypatch.delenv("LNG_SHARE_VARIANT_NODES", raising=False) + reg = phase1(VR, V2R) + assert reg[("B", "v1", "output")] == reg[("B", "v2", "output")] + assert reg[("A", "v1", "input")] == reg[("A", "v2", "input")] def test_on_same_entity_same_reaction_same_role_shares_one_uuid(): - reg = phase1(VR, V2R, share=True) + reg = phase1(VR, V2R) assert reg[("B", "v1", "output")] == reg[("B", "v2", "output")] assert reg[("A", "v1", "input")] == reg[("A", "v2", "input")] def test_on_different_reactions_stay_distinct(): - reg = phase1(VR, V2R, share=True) + reg = phase1(VR, V2R) assert reg[("B", "v1", "output")] != reg[("B", "v3", "output")] # positional across reactions, as designed def test_on_roles_are_not_conflated(): vr = {"v1": (["B"], ["B"])} # B both consumed and produced by one variant - reg = phase1(vr, {"v1": "R1"}, share=True) + reg = phase1(vr, {"v1": "R1"}) assert reg[("B", "v1", "input")] != reg[("B", "v1", "output")] def test_on_variant_specific_members_are_not_conflated(): - reg = phase1(VR, V2R, share=True) + reg = phase1(VR, V2R) assert reg[("S1", "v1", "input")] != reg[("S2", "v2", "input")] # different entities assert reg[("P1", "v1", "output")] != reg[("P2", "v2", "output")] @@ -53,7 +70,7 @@ def test_on_variant_specific_members_are_not_conflated(): def test_boundary_entities_are_left_to_their_own_caches(): # "A" is a root input: the boundary cache already shares it per stId; sharing # must not create a second uuid for it. - reg = phase1(VR, V2R, share=True, roots={"A"}) + reg = phase1(VR, V2R, roots={"A"}) assert reg[("A", "v1", "input")] == reg[("A", "v2", "input")] assert len({reg[("A", v, "input")] for v in ("v1", "v2")}) == 1 @@ -63,7 +80,7 @@ def test_terminal_output_shared_across_reactions_by_its_own_cache_not_the_varian # boundary cache shares it per stId across ALL three; an implementation that # ignored the boundary exclusion and shared via the variant cache would give # v3 a different uuid from v1/v2. All three must be one uuid. - reg = phase1(VR, V2R, share=True, terms={"B"}) + reg = phase1(VR, V2R, terms={"B"}) assert len({reg[("B", v, "output")] for v in ("v1", "v2", "v3")}) == 1 @@ -71,7 +88,7 @@ def test_missing_reaction_id_read_back_as_nan_string_is_not_a_shared_key(): # reactome_id read from a cached CSV with no dtype turns NaN into "nan"; # every variant of every reaction with a missing id must NOT collapse onto # one uuid through the key (eid, "nan", role). - reg = phase1(VR, {"v1": "nan", "v2": "nan", "v3": "nan"}, share=True) + reg = phase1(VR, {"v1": "nan", "v2": "nan", "v3": "nan"}) assert reg[("B", "v1", "output")] != reg[("B", "v2", "output")] assert reg[("B", "v1", "output")] != reg[("B", "v3", "output")] @@ -80,12 +97,12 @@ def test_partial_reaction_map_is_reported(): from src.logic_network_generator import _register_phase1 reg = {} stats = _register_phase1(VR, reg, set(), {}, set(), {}, - vr_to_reaction={"v1": "R1", "v3": "R2"}, share_variants=True) + vr_to_reaction={"v1": "R1", "v3": "R2"}) assert stats["unmapped"] == 1 # the unmapped variant is registered per-variant, the mapped ones still share among themselves assert reg[("B", "v1", "output")] != reg[("B", "v2", "output")] def test_unknown_reaction_falls_back_to_per_variant(): - reg = phase1(VR, {}, share=True) # no vr -> reaction map + reg = phase1(VR, {}) # no vr -> reaction map assert reg[("B", "v1", "output")] != reg[("B", "v2", "output")] From f5cf8527785e3acfc614a4b73c5a07fcfb63bb7e Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Mon, 21 Sep 2026 15:38:23 -0400 Subject: [PATCH 3/4] Make the removed-flag guard actually stop a run, and test it for real Adversarial review of the diff found the guard was decorative on both halves. It had NO runtime coverage. Its only test asserted that a string appeared in inspect.getsource(), which passes whether or not the guard runs -- mutating it to `if False and ...` left the whole suite green, and exporting the flag and running the suite gave the baseline result, because no test reaches that line. Replaced with a shared _reject_removed_env() helper, unit-tested with monkeypatch and pytest.raises across five values including "0" and "". And it could not stop a run even when it fired. bin/create-pathways.py catches every per-pathway exception, counts it, and returns None -- so a stale flag meant every pathway raised, nothing was written, the PREVIOUS catalog stayed on disk, and the shell saw exit 0. A benchmark reading that directory would have silently scored the old catalog. Now checked once at the top of main() before any work, and the command exits 1 if any pathway failed. Six other error paths in main() also returned 0 on failure; all now exit 1. All three mutations are caught by the new tests: dead guard body, missing entry-point call, and restored exit-0-on-failure. The error message also claimed a guarantee the code does not provide. _succ is a snapshot taken before the emission loop and is not updated as this function appends its own assembly edges, so a later root complex can still be handed a leaf that an earlier one made reachable. The reviewer constructed the weld. That bug is PRE-EXISTING and identical on both sides of this flag removal, so the comment and message now state the rule precisely rather than overclaiming; fixing it changes emitted networks and would invalidate the measurement this rule rests on, so it is tracked separately. Also: a conftest clears removed names for the whole suite (this diff otherwise turned a stale export into 10 spurious failures, up from 1), unused monkeypatch fixtures dropped, and PIPELINE_SETTINGS.md gains a removed-flags table. Co-Authored-By: Claude Opus 5 (1M context) --- PIPELINE_SETTINGS.md | 13 +++++ bin/create-pathways.py | 30 +++++++++--- src/logic_network_generator.py | 40 ++++++++++++---- tests/conftest.py | 19 ++++++++ tests/test_boundary_leaf_reuse.py | 6 +-- tests/test_removed_env_flags.py | 80 +++++++++++++++++++++++++++++++ 6 files changed, 170 insertions(+), 18 deletions(-) create mode 100644 tests/conftest.py create mode 100644 tests/test_removed_env_flags.py diff --git a/PIPELINE_SETTINGS.md b/PIPELINE_SETTINGS.md index f5b8bc4..002ec05 100644 --- a/PIPELINE_SETTINGS.md +++ b/PIPELINE_SETTINGS.md @@ -99,3 +99,16 @@ If you drive DeltaSignal over its HTTP API, two things matter: is an in-memory, per-process cache, so re-parse if the server restarts. Full request/response schemas: `deltasignal/docs/API.md`. + +## Removed flags + +These names are still **read**, and setting one is a hard error rather than a +no-op. A flag is removed once the question it existed to ask has been answered: +the spec keeps the evidence, the code does not keep a second path nobody +measures. Rejecting the name rather than ignoring it means a stale value in a +shell, a compose file or an unversioned benchmark script cannot quietly make a +run measure the default while the operator believes otherwise. + +| flag | removed | why | +|---|---|---| +| `LNG_BOUNDARY_LEAF_REUSE` | 2026-09-21 | Boundary leaves never reuse a node the root complex can reach at the time the leaf is chosen. Held-out +173, p<1e-4 (deltasignal `specs/018-derived-edge-loops`). | diff --git a/bin/create-pathways.py b/bin/create-pathways.py index ecd46b7..f007bb9 100755 --- a/bin/create-pathways.py +++ b/bin/create-pathways.py @@ -22,6 +22,7 @@ sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))) from src.argument_parser import configure_logging, logger, parse_args +from src.logic_network_generator import _reject_removed_env from src.pathway_generator import generate_pathway_file from src.neo4j_connector import get_top_level_pathways, get_pathway_name @@ -40,6 +41,14 @@ def canonical_pathway_id(raw: str) -> str: def main() -> None: + # Fail before ANY work if a removed flag is set. The per-pathway + # guards inside the generator would each raise, but the loop below catches + # every exception and continues, so without this check a stale flag would + # produce "93 failed" buried in the log, leave the PREVIOUS run's + # logic_network.csv files untouched on disk, and still exit 0 -- and the + # benchmark that reads that directory would silently score the old catalog. + _reject_removed_env() + dotenv_path = os.path.join(os.path.dirname(__file__), "..", ".env") # load_dotenv populates os.environ (without clobbering already-set vars) so # NEO4J_URL/USER/PASSWORD in .env actually reach neo4j_connector.get_graph(), @@ -63,15 +72,15 @@ def main() -> None: if pathway_list_file: if not os.path.exists(pathway_list_file): logger.error(f"Pathway list file '{pathway_list_file}' does not exist.") - return + sys.exit(1) elif not os.access(pathway_list_file, os.R_OK): logger.error(f"Pathway list file '{pathway_list_file}' is not readable.") - return + sys.exit(1) elif not args.pathway_list and not args.pathway_id and not args.top_level_pathways: logger.error( "One of the following is required: '--pathway-id', '--pathway-list', '--top-level-pathways', or 'PATHWAY_LIST_FILE' environment variable." ) - return + sys.exit(1) pathway_list: List[Tuple[str, str]] = [] @@ -84,7 +93,7 @@ def main() -> None: logger.info(f"Found {len(pathway_list)} top-level pathways") except Exception as e: logger.error(f"Error fetching top-level pathways: {e}") - return + sys.exit(1) elif args.pathway_id: # Single pathway by ID - fetch name from database pathway_id = args.pathway_id @@ -93,10 +102,10 @@ def main() -> None: logger.info(f"Found pathway: {pathway_name} (stId: {pathway_id})") except ValueError: logger.error(f"Pathway with ID {pathway_id} not found in database") - return + sys.exit(1) except Exception as e: logger.error(f"Error fetching pathway name: {e}") - return + sys.exit(1) pathway_list = [(pathway_id, pathway_name)] elif pathway_list_file: try: @@ -106,7 +115,7 @@ def main() -> None: pathways_df["pathway_name"])] except Exception as e: logger.error(f"Error reading pathway list file: {e}") - return + sys.exit(1) logger.info(f"Processing {len(pathway_list)} pathway(s)") logger.info(f"Output directory: {output_dir}") @@ -124,6 +133,13 @@ def main() -> None: continue logger.info(f"Completed: {successful} successful, {failed} failed") + if failed: + # A partial catalog is not a catalog: downstream benchmarks glob the + # output directory and would score whatever mix of new and stale + # pathways happens to be there. Exit non-zero so a shell `&&` chain, + # a Makefile or CI stops instead of proceeding. + logger.error(f"{failed} pathway(s) failed; exiting non-zero") + sys.exit(1) if __name__ == "__main__": diff --git a/src/logic_network_generator.py b/src/logic_network_generator.py index fbfa2ca..622a59a 100755 --- a/src/logic_network_generator.py +++ b/src/logic_network_generator.py @@ -481,6 +481,26 @@ def _register_entity_uuid( return entity_uuid_registry[key] +# Flags that were removed once the question they existed to ask was answered +# (see the flag-expiry policy in deltasignal specs/020). The names are still +# READ so that a stale value in a shell, a compose file or an unversioned +# benchmark script fails loudly instead of being silently ignored while the run +# measures the default. That silent-input failure mode has bitten this project +# four times (deltasignal DS #40-43). +_REMOVED_ENV = { + "LNG_BOUNDARY_LEAF_REUSE": + "boundary leaves never reuse a node the root complex can reach at the time " + "the leaf is chosen. See deltasignal specs/018-derived-edge-loops.", +} + + +def _reject_removed_env() -> None: + """Raise if any removed LNG_* flag is set. Safe to call repeatedly.""" + for name, why in sorted(_REMOVED_ENV.items()): + if name in os.environ: + raise ValueError(f"{name} was removed: {why}") + + def _register_phase1( vr_entities: Dict[str, tuple], entity_uuid_registry: Dict[tuple, str], @@ -1715,15 +1735,19 @@ def _emit_boundary_decomposition_edges( # # This is unconditional. The LNG_BOUNDARY_LEAF_REUSE escape hatch that # restored the welding behaviour was removed once the question it existed - # to ask was answered (held-out +173, p<1e-4). Setting it is an error - # rather than a no-op, so a stale value in an environment or a script is - # not silently ignored while the run measures something else. + # to ask was answered (held-out +173, p<1e-4). + # + # SCOPE OF THE RULE, stated precisely: a leaf is refused if the root + # complex can reach it *at the moment that leaf is chosen*. `_succ` is a + # snapshot taken before the loop and is NOT updated as this function + # appends its own assembly edges, so a later root complex can still be + # handed a leaf that became reachable through an edge emitted for an + # earlier one. That residual weld is a real pre-existing bug -- it exists + # identically on both sides of the flag removal -- and it is tracked + # separately rather than fixed here, because fixing it changes emitted + # networks and would invalidate the measurement this rule rests on. from collections import defaultdict - if "LNG_BOUNDARY_LEAF_REUSE" in os.environ: - raise ValueError( - "LNG_BOUNDARY_LEAF_REUSE was removed: boundary leaves never reuse a node " - "the root complex can reach. See deltasignal specs/018-derived-edge-loops." - ) + _reject_removed_env() _succ: Dict[str, List[str]] = defaultdict(list) for e in pathway_logic_network_data: _succ[str(e.get("source_id"))].append(str(e.get("target_id"))) diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 0000000..fb416e7 --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,19 @@ +"""Shared fixtures. + +Removed LNG_* flags are rejected at runtime (see `_REMOVED_ENV`), so a stale +value exported in a developer's shell or a CI runner would turn every test that +builds a network into a spurious failure. Tests should exercise the code, not +the ambient environment, so clear those names for the whole suite. The guard +itself is tested explicitly in tests/test_removed_env_flags.py. +""" + +import pytest + +from src.logic_network_generator import _REMOVED_ENV + + +@pytest.fixture(autouse=True) +def _clear_removed_env_flags(monkeypatch): + for name in _REMOVED_ENV: + monkeypatch.delenv(name, raising=False) + yield diff --git a/tests/test_boundary_leaf_reuse.py b/tests/test_boundary_leaf_reuse.py index 756e3df..c081bbc 100644 --- a/tests/test_boundary_leaf_reuse.py +++ b/tests/test_boundary_leaf_reuse.py @@ -38,7 +38,7 @@ def assembly_sources(data): return {e["source_id"]: e for e in data if e["edge_type"] == "assembly"} -def test_produced_node_is_not_reused_as_a_boundary_leaf(stub, monkeypatch): +def test_produced_node_is_not_reused_as_a_boundary_leaf(stub): data, r2u = network() _emit_boundary_decomposition_edges(data, r2u) asm = assembly_sources(data) @@ -55,7 +55,7 @@ def test_produced_node_is_not_reused_as_a_boundary_leaf(stub, monkeypatch): assert not (sources_into_C & produced) -def test_a_copy_reached_only_through_a_bridge_is_downstream_too(stub, monkeypatch): +def test_a_copy_reached_only_through_a_bridge_is_downstream_too(stub): # P has two copies: u_Pout (reaction output, downstream of C) and u_Pin (an input copy whose # only incoming edge is a diagram bridge from u_Pout -- also downstream). Neither may be reused. data, r2u = network() @@ -68,7 +68,7 @@ def test_a_copy_reached_only_through_a_bridge_is_downstream_too(stub, monkeypatc assert "u_Pout" not in asm and "u_Pin" not in asm -def test_a_produced_copy_that_is_not_downstream_is_reused(stub, monkeypatch): +def test_a_produced_copy_that_is_not_downstream_is_reused(stub): # The Mitotic G1 shape: P's only copy is produced by a reaction the root complex C does NOT # reach (r2, fed by Q). Reusing it adds no cycle and keeps the feed-forward link # P -> (assembly) -> C that Reactome joins only by hasComponent. diff --git a/tests/test_removed_env_flags.py b/tests/test_removed_env_flags.py new file mode 100644 index 0000000..aaa8a51 --- /dev/null +++ b/tests/test_removed_env_flags.py @@ -0,0 +1,80 @@ +"""The removed-flag guard, tested for real. + +This file exists because the first version of the guard was tested only by +asserting that a string appeared in `inspect.getsource(...)`. That passes +whether or not the guard runs: mutating it to `if False and ...` left the whole +suite green. These tests call the guard and the entry point that uses it. +""" +import subprocess +import sys +from pathlib import Path + +import pytest + +from src.logic_network_generator import _REMOVED_ENV, _reject_removed_env + +REPO = Path(__file__).resolve().parents[1] + + +def test_clean_environment_is_fine(monkeypatch): + for name in _REMOVED_ENV: + monkeypatch.delenv(name, raising=False) + _reject_removed_env() # must not raise + + +@pytest.mark.parametrize("name", sorted(_REMOVED_ENV)) +@pytest.mark.parametrize("value", ["1", "0", "any", "", "downstream_free"]) +def test_any_value_of_a_removed_flag_raises(monkeypatch, name, value): + # Including "0" and "": the point is that the name is gone, so there is no + # value that means "use the old behaviour" and none that means "off". + monkeypatch.setenv(name, value) + with pytest.raises(ValueError, match=f"{name} was removed"): + _reject_removed_env() + + +@pytest.mark.parametrize("name", sorted(_REMOVED_ENV)) +def test_the_message_names_the_spec_that_holds_the_evidence(monkeypatch, name): + monkeypatch.setenv(name, "1") + with pytest.raises(ValueError) as exc: + _reject_removed_env() + assert "specs/" in str(exc.value) + + +@pytest.mark.parametrize("name", sorted(_REMOVED_ENV)) +def test_entry_point_exits_nonzero_before_doing_any_work(monkeypatch, name, tmp_path): + # The generator's per-pathway guard is not enough on its own: the pathway + # loop catches every exception and continues, so a stale flag would leave + # the previous run's files on disk and still exit 0. + env = {**dict(__import__("os").environ), name: "1"} + out = tmp_path / "out" + proc = subprocess.run( + [sys.executable, "bin/create-pathways.py", "--pathway-id", "R-HSA-73894", + "--output-dir", str(out)], + cwd=REPO, env=env, capture_output=True, text=True, timeout=180, + ) + assert proc.returncode != 0, "a removed flag must stop the run" + assert f"{name} was removed" in (proc.stderr + proc.stdout) + assert not out.exists() or not any(out.iterdir()), "must fail before writing output" + + +def test_a_failing_pathway_makes_the_run_exit_nonzero(tmp_path): + """A partial catalog must not look like a successful run. + + The pathway loop catches every exception and continues, so before this the + command logged "N failed" and still exited 0. A benchmark that globs the + output directory would then score whatever mix of new and stale pathways + happened to be there. Any failure reason serves here -- a bogus stable id + or an unreachable database both take the same path. + """ + listing = tmp_path / "list.tsv" + listing.write_text("id\tpathway_name\nR-HSA-NOT-A-REAL-PATHWAY\tbogus\n") + out = tmp_path / "out" + proc = subprocess.run( + [sys.executable, "bin/create-pathways.py", + "--pathway-list", str(listing), "--output-dir", str(out)], + cwd=REPO, capture_output=True, text=True, timeout=300, + ) + assert proc.returncode != 0, ( + "a run in which every pathway failed must not exit 0\n" + f"stdout:\n{proc.stdout[-2000:]}\nstderr:\n{proc.stderr[-2000:]}" + ) From 6d9a4168b65d0bdeca3fd680dc91ac341ba6f8b5 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Mon, 21 Sep 2026 15:45:48 -0400 Subject: [PATCH 4/4] Address the diff review: real guard, correct docstring, tests that can fail Guard now routes through the shared, tested _reject_removed_env() from #93, so a stale LNG_SHARE_VARIANT_NODES is rejected at process entry before any database work and the command exits non-zero. The previous guard had no runtime coverage at all: its only test asserted a string appeared in inspect.getsource(), which passed with the guard mutated to `if False and ...` and broke the moment the guard was refactored. Deleted; tests/test_removed_ env_flags.py now parametrizes over both removed names. Three factual corrections to the docstring: - The summary line said "one key per (entity, REACTOME reaction, role)". That is FALSE and a reader acting on it would break Phase 2, which looks entities up by VIRTUAL reaction. The key is unchanged; the shared thing is the VALUE. - "-83 and -117 held-out" was wrong on both numbers. specs/016 P12 gives min over copies -85 held-out (p 0.0021) and max over copies -322 (p<0.0001); -117 is a DSB-Repair-only ACCURACY delta, not a held-out figure. - The size result now carries the caveat the spec insists on: the -34.6% node figure flatters it (no-op in 47 of 92 pathways, median mover 6.7%), the -21.5% edge figure is the conservative read, and the zero held-out net comes from conditioning that drops 4,094 non-comparable cases while both axes fall slightly unconditioned. The two boundary tests asserted a mechanism that cannot occur -- the variant cache could only ever store the boundary uuid, so deleting the skip left every uuid identical. They now assert on the shared/distinct stats, which is what actually distinguishes the implementations, and the mutation is caught. Re-verified after all edits: 1500 randomized inputs give 0 differences against main's share_variants=True path, and HDR (799/3,123) and DNA Repair (4,756/14,487) reproduce the measured catalog exactly. Fingerprint honesty: only LNG_EMIT_ONE_SIDED is a genuine cache gap; the other two are read inside a function that re-runs every time, so they buy provenance at the cost of a re-fetch. Kept deliberately, now documented as such. Co-Authored-By: Claude Opus 5 (1M context) --- PIPELINE_SETTINGS.md | 1 + src/logic_network_generator.py | 39 +++++++++++++++--------- src/pathway_generator.py | 9 ++++++ tests/test_variant_node_sharing.py | 48 ++++++++++++++++++++---------- 4 files changed, 67 insertions(+), 30 deletions(-) diff --git a/PIPELINE_SETTINGS.md b/PIPELINE_SETTINGS.md index 002ec05..0796211 100644 --- a/PIPELINE_SETTINGS.md +++ b/PIPELINE_SETTINGS.md @@ -111,4 +111,5 @@ run measure the default while the operator believes otherwise. | flag | removed | why | |---|---|---| +| `LNG_SHARE_VARIANT_NODES` | 2026-09-21 | Variant-node sharing is always on. Edges -21.5%, nodes -34.6% (no-op in 47 of 92 pathways), no pathway gains cyclic nodes, held-out net zero on both axes (deltasignal `specs/020-variant-node-sharing`). | | `LNG_BOUNDARY_LEAF_REUSE` | 2026-09-21 | Boundary leaves never reuse a node the root complex can reach at the time the leaf is chosen. Held-out +173, p<1e-4 (deltasignal `specs/018-derived-edge-loops`). | diff --git a/src/logic_network_generator.py b/src/logic_network_generator.py index b159e8f..5015893 100755 --- a/src/logic_network_generator.py +++ b/src/logic_network_generator.py @@ -488,6 +488,9 @@ def _register_entity_uuid( # measures the default. That silent-input failure mode has bitten this project # four times (deltasignal DS #40-43). _REMOVED_ENV = { + "LNG_SHARE_VARIANT_NODES": + "variant-node sharing is always on. See deltasignal " + "specs/020-variant-node-sharing.", "LNG_BOUNDARY_LEAF_REUSE": "boundary leaves never reuse a node the root complex can reach at the time " "the leaf is chosen. See deltasignal specs/018-derived-edge-loops.", @@ -510,7 +513,7 @@ def _register_phase1( terminal_output_uuid_cache: Dict[str, str], vr_to_reaction: Optional[Dict[str, str]] = None, ) -> Dict[str, int]: - """Phase 1 of UUID assignment: one key per (entity, REACTOME reaction, role). + """Phase 1 of UUID assignment: one key per (entity, virtual reaction, role). A Reactome reaction with EntitySet participants becomes several VIRTUAL reactions, one per member combination, and every one of them registers its @@ -520,20 +523,32 @@ def _register_phase1( Nothing downstream can tell them apart, and any solver rule over the copies is wrong in one direction or the other: min over copies caps the container by whichever copy a perturbation missed (missed change); max lets one - elevated copy lift it (false change). Both were measured (deltasignal - specs/016): -83 and -117 held-out. - - So this keys on the REACTOME reaction, not the virtual one: the SAME entity - in the SAME role across the variants of ONE Reactome reaction gets one UUID. + elevated copy lift it (false change). Both were measured on 81 pathways + (deltasignal specs/016 P12): min over copies **-85 held-out** (p 0.0021), + max over copies **-322** (p<0.0001). (An earlier revision of this docstring + cited "-83 and -117"; -83 was a superseded intermediate and -117 is a + DSB-Repair-only ACCURACY delta, not a held-out figure.) + + The registry KEY is unchanged -- Phase 2 looks entities up by virtual + reaction (`entity_uuid_registry.get((on, p_vr, "output"))`) and would break + if it moved. What changed is the UUID *value*: the keys of the SAME entity + in the SAME role across the variants of ONE Reactome reaction now share one + value, because the variant cache is keyed on the Reactome reaction. Variants that differ in WHICH set member they use have different entity ids and are not conflated. Copies across DIFFERENT reactions stay positional, as designed (bridging those was measured harmful four times). Boundary entities already share per stId through their caches and are left to them. This is unconditional; the LNG_SHARE_VARIANT_NODES flag that gated it was - removed once measured (deltasignal specs/020): nodes -34.6%, edges -21.5%, - no pathway gains cyclic nodes, and 29 of 24,100 predictions change with - held-out net ZERO on both ground-truth axes. Note most of the Phase-1 + removed once measured (deltasignal specs/020). Edges fall 21.5% and nodes + 34.6%, but the node figure flatters it: sharing is a no-op in 47 of 92 + pathways and the median reduction among the 45 that move is 6.7%, so the + spec names the edge figure as the conservative read. No pathway gains + cyclic nodes, so this does not undo the specs/018 unwelding. 29 of 24,100 + predictions change; among cases both arms resolve identically the held-out + net is zero on both ground-truth axes, though that conditioning drops 4,094 + cases as non-comparable and unconditioned both axes fall slightly + (curator 20414->20400, experimental 611->609). Note most of the Phase-1 photocopying is already undone downstream -- Phase 2 unions producer-output with consumer-input across variant pairs, and the boundary caches share per stId -- so only 1,431 of 29,655 (entity, reaction, role) contexts actually @@ -2186,11 +2201,7 @@ def create_pathway_logic_network( # LNG_SHARE_VARIANT_NODES flag; it is now unconditional, and setting the # variable is an error rather than a no-op so a stale value in a script is # not silently ignored. - if "LNG_SHARE_VARIANT_NODES" in os.environ: - raise ValueError( - "LNG_SHARE_VARIANT_NODES was removed: variant-node sharing is always on. " - "See deltasignal specs/020-variant-node-sharing." - ) + _reject_removed_env() _register_phase1( vr_entities, entity_uuid_registry, root_input_eids, root_input_uuid_cache, diff --git a/src/pathway_generator.py b/src/pathway_generator.py index 8b26591..f7edf92 100755 --- a/src/pathway_generator.py +++ b/src/pathway_generator.py @@ -36,6 +36,15 @@ "LNG_HANDOFF_HUB_MAX", "LNG_SET_MEMBERS_OR", "LNG_DIAGRAM_SET_MEMBER", + # Of these three, only LNG_EMIT_ONE_SIDED is a genuine cache gap: it is + # read in reaction_generator.decompose_by_reactions, so it changes + # best_matches.csv / decomposed_uid_mapping.csv, which ARE cached. + # LNG_BOUNDARY_EXPANSION and LNG_COMPOSITION_EDGES are read inside + # create_pathway_logic_network, which is re-run from scratch every time, so + # listing them buys provenance in fingerprint.json at the cost of an + # unnecessary re-fetch when they are flipped. Kept deliberately: knowing + # which settings produced a catalog has been worth more than the re-fetch, + # and src_sha256 already invalidates on any source change anyway. "LNG_BOUNDARY_EXPANSION", "LNG_COMPOSITION_EDGES", "LNG_EMIT_ONE_SIDED", diff --git a/tests/test_variant_node_sharing.py b/tests/test_variant_node_sharing.py index 9b0ffa1..d2e443d 100644 --- a/tests/test_variant_node_sharing.py +++ b/tests/test_variant_node_sharing.py @@ -9,7 +9,6 @@ -- one per variant reaction -- and no solver rule over copies is right: min over them measured -83 held-out, max -117. One node is the fix. """ -import pytest from src.logic_network_generator import _register_phase1 @@ -27,18 +26,13 @@ def phase1(vr_entities, vr_to_reaction, roots=frozenset(), terms=frozenset()): V2R = {"v1": "R1", "v2": "R1", "v3": "R2"} -def test_removed_flag_is_an_error_not_a_noop(monkeypatch): - # Sharing is unconditional. A stale LNG_SHARE_VARIANT_NODES must fail loudly - # rather than let a run silently measure the default under the old name. - from src.logic_network_generator import create_pathway_logic_network - import inspect - src = inspect.getsource(create_pathway_logic_network) - assert "LNG_SHARE_VARIANT_NODES was removed" in src - assert 'os.environ.get("LNG_SHARE_VARIANT_NODES"' not in src +# The guard rejecting a stale LNG_SHARE_VARIANT_NODES is tested for real in +# tests/test_removed_env_flags.py, which calls it and drives the entry point. +# An earlier version asserted on inspect.getsource() here; that passed whether +# or not the guard ran, and broke when the guard moved to a shared helper. -def test_sharing_is_on_with_no_flag_set(monkeypatch): - monkeypatch.delenv("LNG_SHARE_VARIANT_NODES", raising=False) +def test_sharing_is_on_with_no_flag_set(): reg = phase1(VR, V2R) assert reg[("B", "v1", "output")] == reg[("B", "v2", "output")] assert reg[("A", "v1", "input")] == reg[("A", "v2", "input")] @@ -70,18 +64,40 @@ def test_on_variant_specific_members_are_not_conflated(): def test_boundary_entities_are_left_to_their_own_caches(): # "A" is a root input: the boundary cache already shares it per stId; sharing # must not create a second uuid for it. - reg = phase1(VR, V2R, roots={"A"}) + # + # NOTE on what this can and cannot catch. The `eid not in boundary_eids` + # skip in _register_phase1 is a STATS/CLARITY guard, not a correctness one: + # _register_entity_uuid consults the boundary cache regardless, so the + # variant cache could only ever store the boundary uuid. Deleting the skip + # leaves every uuid identical (verified over 300 randomized inputs) and only + # changes the `shared`/`distinct` counters. So assert on those too, or this + # test passes with the skip removed. + reg = {} + stats = _register_phase1(VR, reg, {"A"}, {}, set(), {}, vr_to_reaction=V2R) assert reg[("A", "v1", "input")] == reg[("A", "v2", "input")] assert len({reg[("A", v, "input")] for v in ("v1", "v2")}) == 1 + # "A" must not be counted as a variant-cache reuse: the boundary cache did it. + assert stats["shared"] == 1, stats # only B in R1 shares; A is boundary-excluded + assert stats["distinct"] == 7, stats def test_terminal_output_shared_across_reactions_by_its_own_cache_not_the_variant_cache(): # "B" is an output in R1 (v1, v2) AND in R2 (v3), and a terminal output. The - # boundary cache shares it per stId across ALL three; an implementation that - # ignored the boundary exclusion and shared via the variant cache would give - # v3 a different uuid from v1/v2. All three must be one uuid. - reg = phase1(VR, V2R, terms={"B"}) + # boundary cache shares it per stId across ALL three, so all three must be + # one uuid. + # + # The previous comment here claimed that "an implementation that ignored the + # boundary exclusion and shared via the variant cache would give v3 a + # different uuid from v1/v2". That mechanism cannot occur -- the variant + # cache would store the same boundary uuid -- so the claim was asserting + # something unreachable. The uuid assertion below is still the property we + # want; the stats assertion is what actually distinguishes the two + # implementations. + reg = {} + stats = _register_phase1(VR, reg, set(), {}, {"B"}, {}, vr_to_reaction=V2R) assert len({reg[("B", v, "output")] for v in ("v1", "v2", "v3")}) == 1 + assert stats["shared"] == 1, stats # A in R1; B is boundary-excluded + assert stats["distinct"] == 6, stats def test_missing_reaction_id_read_back_as_nan_string_is_not_a_shared_key():