From b916588962e25905dc79efcde0a960fc327c1fc8 Mon Sep 17 00:00:00 2001 From: Adam Wright <668955+adamjohnwright@users.noreply.github.com> Date: Fri, 2 Oct 2026 20:18:47 -0400 Subject: [PATCH 1/2] Code review 2026-10-02: a build fails loudly instead of shipping a silently wrong bundle - Export failures raise (F2). Every export after logic_network.csv was logged and skipped, including the orphan guard's deliberate ValueError, so containment.csv/cofactors.csv/node_resolution.csv went missing while the pathway counted as built and catalog.sh moved current. Regeneration also clears the previous run's bundle files. - LNG_* switches read through src/env_flags.py (F3): exactly 0 or 1, and an unknown LNG_* name is a startup error. LNG_HANDOFF_EDGES=false used to turn the harmful handoff edges ON; LNG_BOUNDARY_HIERARCHY=true turned the adopted hierarchy OFF; typos were never read. - Failed Neo4j lookups raise (F5, F11): the modifier-set fallback was cached for the whole run; the ubiquitin lookup dropped every Ub depletion edge; containment treated a failed lookup as 'contains only itself'; node_resolution dropped every set row. - PIPELINE_SETTINGS.md: LNG_DIAGRAM_BRIDGE default is 0; the stale DS defaults are replaced by a pointer (F13). Default output unchanged: GPVI, NODAL and RAF regenerated from this branch are identical to build 20260928-1110_06ccb63 by stable id, edge for edge, and every bundle file has the same rows. Runtime tests for each guard; the old code fails them. Co-Authored-By: Claude Opus 5.5 (1M context) --- PIPELINE_SETTINGS.md | 25 +++++------ src/diagram_connectivity.py | 3 +- src/env_flags.py | 64 +++++++++++++++++++++++++++ src/logic_network_generator.py | 52 +++++++++++----------- src/neo4j_connector.py | 3 +- src/pathway_generator.py | 36 ++++++++++++--- src/reaction_generator.py | 26 +++++------ tests/test_composition_edges.py | 5 ++- tests/test_diagram_set_member.py | 5 ++- tests/test_env_flags.py | 47 ++++++++++++++++++++ tests/test_export_failfast.py | 76 ++++++++++++++++++++++++++++++++ tests/test_lookup_failures.py | 32 ++++++++++++++ 12 files changed, 309 insertions(+), 65 deletions(-) create mode 100644 src/env_flags.py create mode 100644 tests/test_env_flags.py create mode 100644 tests/test_export_failfast.py create mode 100644 tests/test_lookup_failures.py diff --git a/PIPELINE_SETTINGS.md b/PIPELINE_SETTINGS.md index 0796211..0daae2d 100644 --- a/PIPELINE_SETTINGS.md +++ b/PIPELINE_SETTINGS.md @@ -67,23 +67,22 @@ what NOT to touch. Overriding is for A/B benchmarking only. | `LNG_COMPLEX_AS_NODE` | `1` | a Complex is one node (the key connectivity fix) | | `LNG_SET_EXPAND` | `1` | expand EntitySets to members at matching granularity | | `LNG_DIAGRAM_CONNECTIVITY` | `1` | union diagram-drawn reaction pairs into connectivity | -| `LNG_DIAGRAM_BRIDGE` | `1` | add (don't merge) diagram bridge edges | +| `LNG_DIAGRAM_BRIDGE` | `0` | legacy additive diagram bridges (measured harmful; off) | +| `LNG_BOUNDARY_HIERARCHY` | `1` | decompose a root complex one `hasComponent` level at a time (deltasignal specs/030) | +| `LNG_SET_POOL` | `1` | one OR node per set-valued catalyst or regulator (deltasignal specs/033) | +| `LNG_POOL_ACTIVE_VIA` | `made_from` | how a pool's active form is found (deltasignal specs/039) | | `LNG_CATALYST_BUNDLE` | `0` | legacy; subsumed by `LNG_COMPLEX_AS_NODE` | | `LNG_HANDOFF_EDGES` | `0` | precedingEvent hand-off bridges (net-negative, off) | -**Solver (DeltaSignal):** +A switch takes exactly `0` or `1`; any other value, or an `LNG_*` name the +generator does not read (a typo), stops the run at startup. The full list and +defaults are `BOOL_FLAGS` in `src/env_flags.py`. -| Variable | Default | Meaning | -|---|---|---| -| `DS_INHIBITION_MODE` | `divide` | de-repression-capable inhibition | -| `DS_AND_MODE` | `hill_log` | continuous log-fold AND aggregation | -| `DS_OR_MODE` | `mean` | OR alternatives each carry weight | -| `DS_ASSEMBLY_LIMITING` | `1` | a complex can't exceed its scarcest subunit | -| `DS_HILL_LOG_ZMAX` | `10.0` | AND saturation point | -| `DS_SCC_SOLVE` | `1` | SCC-condensation solve (handles feedback loops) | - -(There are more `DS_*` switches for other inhibition/AND modes, loop handling, -etc. — all default to the validated config. If you didn't set it, it's right.) +**Solver (DeltaSignal):** the defaults live in the deltasignal repo's +CLAUDE.md ("Solver Configuration"), which CI keeps in step with the code; they +are not repeated here, because this copy went stale (it gave `DS_AND_MODE` +`hill_log` and `DS_ASSEMBLY_LIMITING` `1`; the defaults are `hill_sat` and `0`). +If you didn't set a `DS_*` variable, it is at the validated default. ## 4. Running the solver diff --git a/src/diagram_connectivity.py b/src/diagram_connectivity.py index 0fa7ff2..b695095 100644 --- a/src/diagram_connectivity.py +++ b/src/diagram_connectivity.py @@ -18,6 +18,7 @@ """ import json import os +from src.env_flags import env_flag from collections import defaultdict from pathlib import Path from typing import Dict, List, Set, Tuple @@ -212,7 +213,7 @@ def augment_reaction_connections(pathway_id: str, event_status='Diagram Shared Product' for traceability. No-op (returns input unchanged) when disabled or no diagram is available. """ - if os.environ.get("LNG_DIAGRAM_CONNECTIVITY", "1") == "0": + if not env_flag("LNG_DIAGRAM_CONNECTIVITY"): return reaction_connections pairs = diagram_shared_product_pairs(pathway_id) diff --git a/src/env_flags.py b/src/env_flags.py new file mode 100644 index 0000000..7b96fa4 --- /dev/null +++ b/src/env_flags.py @@ -0,0 +1,64 @@ +"""One reader for the generator's LNG_* switches. + +Before this (code review 2026-10-02) each flag was compared to a literal at its +call site, in two opposite ways: `!= "0"` flags turned ON for any other value +(LNG_HANDOFF_EDGES=false enabled the measured-harmful handoff edges) and +`== "1"` flags turned OFF (LNG_BOUNDARY_HIERARCHY=true disabled the adopted +hierarchy). A misspelt name was simply never read, while catalog.sh recorded it +in BUILD.json as if applied. Now a switch takes exactly "0" or "1", and an +LNG_* name the generator does not read is an error at startup. +""" +import os + +# Boolean switches and their defaults. +BOOL_FLAGS = { + "LNG_BOUNDARY_EXPANSION": True, + "LNG_BOUNDARY_HIERARCHY": True, + "LNG_CAP_POOLS": False, + "LNG_CATALYST_BUNDLE": False, + "LNG_COMPLEX_AS_NODE": True, + "LNG_COMPOSITION_EDGES": False, + "LNG_DEBUG_TRACEBACKS": False, + "LNG_DIAGRAM_BRIDGE": False, + "LNG_DIAGRAM_CONNECTIVITY": True, + "LNG_DIAGRAM_SET_MEMBER": False, + "LNG_EMIT_ONE_SIDED": True, + "LNG_HANDOFF_EDGES": False, + "LNG_SET_EXPAND": True, + "LNG_SET_MEMBERS_OR": False, + "LNG_SET_POOL": True, +} + +# Read elsewhere with their own parsing and validation. +OTHER_FLAGS = frozenset({ + "LNG_ALLOW_NONDETERMINISM", # bin/create-pathways.py, before imports + "LNG_DIAGRAM_DIR", # a path + "LNG_HANDOFF_HUB_MAX", # _int_env + "LNG_MAX_VARIANTS", # _int_env + "LNG_POOL_ACTIVE_VIA", # pool_active_via() + "LNG_PYTHON", # catalog.sh: the interpreter, not a switch +}) + + +def env_flag(name: str) -> bool: + """The value of boolean switch `name`: unset or empty is its default, + "1" is on, "0" is off, anything else is an error.""" + default = BOOL_FLAGS[name] # KeyError: an unregistered switch + raw = os.environ.get(name, "").strip() + if raw == "": + return default + if raw not in ("0", "1"): + raise ValueError(f"{name} must be 0 or 1, got {raw!r}. Unset it for the default " + f"({int(default)}).") + return raw == "1" + + +def validate_env(removed=()) -> None: + """Every LNG_* in the environment is a known name with a valid value.""" + known = set(BOOL_FLAGS) | OTHER_FLAGS | set(removed) + unknown = sorted(n for n in os.environ if n.startswith("LNG_") and n not in known) + if unknown: + raise ValueError(f"Unknown LNG_* variable(s) {unknown}: the generator does not read " + "them, so they would have no effect. Check the spelling.") + for name in BOOL_FLAGS: + env_flag(name) diff --git a/src/logic_network_generator.py b/src/logic_network_generator.py index fa175e0..921706e 100755 --- a/src/logic_network_generator.py +++ b/src/logic_network_generator.py @@ -1,4 +1,5 @@ import os +from src.env_flags import env_flag, validate_env import uuid from collections import Counter from typing import Dict, List, Any, NamedTuple, Optional, Set, Tuple @@ -504,6 +505,7 @@ def _reject_removed_env() -> None: for name, why in sorted(_REMOVED_ENV.items()): if name in os.environ: raise ValueError(f"{name} was removed: {why}") + validate_env(removed=_REMOVED_ENV) pool_active_via() @@ -800,7 +802,7 @@ def _map_annotated_entity_to_nodes(entity_id: str, member_set: Set[str]) -> Set[ # (all plain stId), so a complex produced by one reaction and # catalysing/regulating another unifies instead of siloing into # disconnected ::variant:: nodes. See plan luminous-percolating-llama. - if os.environ.get("LNG_COMPLEX_AS_NODE", "1") != "0": + if env_flag("LNG_COMPLEX_AS_NODE"): return {str(entity_id)} # Set-variant node: OUTERMOST complex stId + a FLAT sorted list of the # variant's FULL terminal membership. We enumerate the complex's true @@ -835,7 +837,7 @@ def _map_annotated_entity_to_nodes(entity_id: str, member_set: Set[str]) -> Set[ # intersection came up empty and the set fell back to a bare, producer-less # node. Aligning the two granularities lets the set expand to its members # (sets should never survive as nodes — see docs/DESIGN_DECISIONS.md). - if os.environ.get("LNG_SET_EXPAND", "1") == "0": + if not env_flag("LNG_SET_EXPAND"): # Old behavior (leaf-granularity intersection) for A/B comparison. present = member_set & get_terminal_components(entity_id) else: @@ -891,7 +893,7 @@ def _annotated_stoich(reaction_id: str, io: str) -> Dict[str, int]: stoich_cache[key] = get_reaction_io_stoichiometry(reaction_id, io) return stoich_cache[key] - cap_pools = os.environ.get("LNG_CAP_POOLS", "0") == "1" + cap_pools = env_flag("LNG_CAP_POOLS") def _resolve_io(reaction_id: str, io: str, members: Set[str]) -> tuple: # Node identity comes from the reaction's annotated entities; the @@ -1352,7 +1354,10 @@ def _emit_substrate_depletion_edges( if subst_stids: ubiquitin_subst_by_rstid[rxn] = (subst_genes, subst_stids) except Exception as exc: - logger.warning(f"Ubiquitin topology lookup failed: {exc}") + # Raise: carrying on dropped every ubiquitin depletion edge from + # logic_network.csv with only a warning (code review 2026-10-02). + logger.error(f"Ubiquitin topology lookup failed: {exc}") + raise # Build network stid → list of UUIDs index from pathway_logic_network_data, # so we can target the substrate's NETWORK NODES (not just the leaf @@ -1922,7 +1927,7 @@ def _is_complex(entity_id: str) -> bool: # a second, causal encoding of something already available structurally. # # Default ON pending the measurement that decides it. - boundary_expansion = os.environ.get("LNG_BOUNDARY_EXPANSION", "1") == "1" + boundary_expansion = env_flag("LNG_BOUNDARY_EXPANSION") # LNG_BOUNDARY_HIERARCHY=1 (deltasignal specs/030): decompose a root complex # ONE hasComponent level at a time instead of straight to its base leaves. # A component that already exists as a node -- e.g. ISGF3 [cytosol], which @@ -1935,7 +1940,7 @@ def _is_complex(entity_id: str) -> bool: # root ISGF3:KPNA1:KPNB1 is translocated to the nucleus, and every IFN # alpha/beta perturbation upstream of ISGF3 was severed there (200 held-out # cases). The downstream-reuse rule (specs/018) applies unchanged. - hierarchy = os.environ.get("LNG_BOUNDARY_HIERARCHY", "1") == "1" # default since deltasignal specs/030 (held-out +197, re-measured) + hierarchy = env_flag("LNG_BOUNDARY_HIERARCHY") # default since deltasignal specs/030 (held-out +197, re-measured) from src.neo4j_connector import get_complex_components nested_registry: Dict[tuple, str] = {} seen_edges: Set[tuple] = set() @@ -2068,7 +2073,7 @@ def _decompose_hier(container_uuid: str, container_stid: str, root_uuid: str, de # Composition hierarchy (LNG_COMPOSITION_EDGES). Runs after boundary # expansion so dissociation sinks are known and excluded as sources. - if os.environ.get("LNG_COMPOSITION_EDGES", "0") == "1": + if env_flag("LNG_COMPOSITION_EDGES"): _emit_composition_edges(pathway_logic_network_data, reactome_id_to_uuid) @@ -2134,19 +2139,19 @@ def append_regulators( # neg) to its plain stId so it matches the produced/consumed occurrence's # node id and unifies instead of siloing. Subsumes the older opt-in # LNG_CATALYST_BUNDLE (pos-only). - complex_as_node = os.environ.get("LNG_COMPLEX_AS_NODE", "1") != "0" - bundle_on = os.environ.get("LNG_CATALYST_BUNDLE", "0") == "1" + complex_as_node = env_flag("LNG_COMPLEX_AS_NODE") + bundle_on = env_flag("LNG_CATALYST_BUNDLE") variant_decomposition = (pos_neg == "neg") and not complex_as_node bundle_complex = complex_as_node or ((pos_neg == "pos") and bundle_on) # Emit set-derived positive regulator members as OR alternatives # (see the and_or comment below). Opt-in while it is being A/B'd. - set_members_or = os.environ.get("LNG_SET_MEMBERS_OR", "0") == "1" + set_members_or = env_flag("LNG_SET_MEMBERS_OR") # deltasignal specs/033: a bare EntitySet regulator becomes ONE pool node # (the set itself) fed by its members, instead of every member wired # onto every reaction copy as a separate required (or blocking) term. # Default ON since specs/033's arms (held-out +6, tuning +10, exp +3 with # DS_SET_POOL_MODE=product); "0" restores the member fan-out. - set_pool = os.environ.get("LNG_SET_POOL", "1") == "1" + set_pool = env_flag("LNG_SET_POOL") from src.neo4j_connector import get_labels, get_set_members for _, row in map_df.iterrows(): @@ -2572,7 +2577,7 @@ def create_pathway_logic_network( # the consumption needs a depletion edge back onto the input, and depletion # is currently emitted for phosphatase reactions only. That is a separate # decision and is deliberately not bundled here. - emit_one_sided = os.environ.get("LNG_EMIT_ONE_SIDED", "1") == "1" + emit_one_sided = env_flag("LNG_EMIT_ONE_SIDED") for vr_uid, (input_ids, output_ids, input_stoich, output_stoich) in vr_entities.items(): if not input_ids and not output_ids: continue # genuinely empty: nothing to say about it @@ -2747,7 +2752,7 @@ def create_pathway_logic_network( # injects roughly as much spurious coupling as real signal it recovers (same # reason the member-exploded network tied set-variant at ~69%). Kept for # future work but OFF by default. See memory project_complex_as_node_result. - if os.environ.get("LNG_HANDOFF_EDGES", "0") != "0": + if env_flag("LNG_HANDOFF_EDGES"): _emit_precedingevent_handoff_edges( pathway_logic_network_data=pathway_logic_network_data, reaction_connections=reaction_connections, @@ -3281,10 +3286,10 @@ def export_containment(reactome_id_to_uuid: Dict[str, str], # positional copies would otherwise pay 40 times for the same answer. leaves_by_stid: Dict[str, Set[str]] = {} for stid in {str(v) for v in reactome_id_to_uuid.values() if v}: - try: - leaves = get_terminal_components(stid) - except Exception: # noqa: BLE001 - a missing entity must not fail the pathway - leaves = set() + # A failed lookup raises: treating it as "contains only itself" put a + # wrong row in containment.csv, which the consumer's default model + # reads (code review 2026-10-02). + leaves = get_terminal_components(stid) leaves_by_stid[stid] = {str(x) for x in leaves} | {stid} # Keyed by STABLE ID, not uuid. Containment is a property of the entity, not @@ -4071,15 +4076,10 @@ def add(stable_id: str, node_uuid: str, relation: str, depth: int, # 4. THE POINT: set -> the member nodes it was split into. excluded: List[Dict[str, str]] = [] - try: - participating = get_pathway_participating_entities(pathway_id) - except Exception: - logger.warning("could not list participating entities; set rows omitted") - participating = set() - try: - atomic = set(get_modifier_isoform_entity_set_ids()) - except Exception: - atomic = set() + # Both raise on failure: an empty answer silently omitted every set row + # from node_resolution.csv (code review 2026-10-02). + participating = get_pathway_participating_entities(pathway_id) + atomic = set(get_modifier_isoform_entity_set_ids()) resolve = make_neo4j_resolver(get_set_members, get_labels) for stable_id in sorted(participating): diff --git a/src/neo4j_connector.py b/src/neo4j_connector.py index 11177e6..7f40ac2 100755 --- a/src/neo4j_connector.py +++ b/src/neo4j_connector.py @@ -1,4 +1,5 @@ import os +from src.env_flags import env_flag from src.credential_redaction import scrub from typing import Any, Dict, List, Optional, Set, Tuple, Union @@ -105,7 +106,7 @@ def _traceback_kwargs() -> Dict[str, Any]: debugging, off by default so nothing credential-bearing reaches debug_log.txt (mode 0664) or stdout. """ - return {"exc_info": os.getenv("LNG_DEBUG_TRACEBACKS") == "1"} + return {"exc_info": env_flag("LNG_DEBUG_TRACEBACKS")} # Module-level caches for bulk pre-fetched data diff --git a/src/pathway_generator.py b/src/pathway_generator.py index fd186e9..0fa3b85 100755 --- a/src/pathway_generator.py +++ b/src/pathway_generator.py @@ -1,6 +1,7 @@ import hashlib import json import os +from src.env_flags import env_flag from pathlib import Path from typing import Any, Dict @@ -211,6 +212,15 @@ def _write_cache_fingerprint( +# Every file a pathway bundle is made of. Cleared before a regeneration. +BUNDLE_FILES = ( + "logic_network.csv", "stid_to_uuid_mapping.csv", "entity_reaction_proxy_mapping.csv", + "nodes.csv", "node_reaction_context.csv", "node_resolution.csv", "node_exclusions.csv", + "cofactors.csv", "containment.csv", "drugs.csv", + "pools.csv", "pool_transitions.csv", "pool_carriers.csv", +) + + def generate_pathway_file( pathway_id: str, pathway_name: str, @@ -267,6 +277,12 @@ def generate_pathway_file( ) pathway_output_dir = base_output_dir / folder_name pathway_output_dir.mkdir(exist_ok=True) + # Regenerating into an existing directory must not leave a previous run's + # bundle files behind: a file this run fails to write would otherwise + # survive with node ids that no longer exist (code review 2026-10-02). + # The cache/ subdirectory is handled by its own fingerprint check. + for stale in BUNDLE_FILES: + (pathway_output_dir / stale).unlink(missing_ok=True) # Create cache subdirectory for intermediate files cache_dir = pathway_output_dir / "cache" @@ -373,7 +389,7 @@ def generate_pathway_file( diagram_shared_product_pairs, ) diagram_bridge_pairs = None - if os.environ.get("LNG_DIAGRAM_BRIDGE", "0") == "1": + if env_flag("LNG_DIAGRAM_BRIDGE"): connectivity = reaction_connections diagram_bridge_pairs = diagram_shared_product_pairs(pathway_id) else: @@ -398,8 +414,8 @@ def generate_pathway_file( # catalog produced mixed and/or clusters. This produces them. The two # therefore have to be enabled and measured TOGETHER, and neither alone. set_member_pairs = None - if (os.environ.get("LNG_DIAGRAM_SET_MEMBER", "0") == "1" - and os.environ.get("LNG_DIAGRAM_CONNECTIVITY", "1") != "0"): + if (env_flag("LNG_DIAGRAM_SET_MEMBER") + and env_flag("LNG_DIAGRAM_CONNECTIVITY")): try: set_member_pairs = diagram_set_member_pairs(pathway_id) except Exception: @@ -433,9 +449,9 @@ def generate_pathway_file( str(uuid_to_reactome_file) ) logger.info(f"Successfully exported stable ID to UUID mapping: {uuid_to_reactome_file}") - except IOError as e: + except Exception as e: logger.error(f"Failed to write stable ID to UUID mapping file {uuid_to_reactome_file}: {e}") - # Don't raise - this is supplementary + raise # Export entity→reaction proxy mapping. Curated species (often Complexes) # that were expanded into virtual variants lose their own stId from the @@ -454,7 +470,7 @@ def generate_pathway_file( logger.info(f"Successfully exported entity-reaction proxy mapping: {proxy_mapping_file}") except Exception as e: logger.error(f"Failed to write entity-reaction proxy mapping file {proxy_mapping_file}: {e}") - # Don't raise - this is supplementary + raise # Schema-backed provenance files (schema/logic_network.linkml.yaml): # nodes.csv (node_kind, diagram_entity_id, member_leaves, set provenance) @@ -496,8 +512,12 @@ def generate_pathway_file( str(pathway_output_dir / "containment.csv"), ) except Exception as e: + # These are not supplementary: containment.csv, cofactors.csv and + # node_resolution.csv feed the consumer's default model. Swallowing + # this let the orphan guard's ValueError skip every file after it + # while the pathway counted as built (LNG code review 2026-10-02). logger.error(f"Failed to write node provenance files: {e}", exc_info=True) - # Don't raise - supplementary + raise # Drug-derived entities (deltasignal specs/032), in their OWN block: a # failed drug query must not cost containment.csv, which the default @@ -511,6 +531,7 @@ def generate_pathway_file( ) except Exception as e: logger.error(f"Failed to write drugs.csv: {e}", exc_info=True) + raise # Interconversion pools (deltasignal specs/039), in their own block. try: @@ -525,6 +546,7 @@ def generate_pathway_file( ) except Exception as e: logger.error(f"Failed to write pools.csv: {e}", exc_info=True) + raise logger.info(f"Output directory: {pathway_output_dir}") diff --git a/src/reaction_generator.py b/src/reaction_generator.py index 801dce4..88f2957 100755 --- a/src/reaction_generator.py +++ b/src/reaction_generator.py @@ -29,6 +29,7 @@ import hashlib import itertools import os +from src.env_flags import env_flag import uuid import warnings from typing import Any, Dict, List, Optional, Sequence, Set, Tuple, Union @@ -205,24 +206,21 @@ def modifier_isoform_set_ids() -> Set[str]: Superset of _UBIQUITIN_ENTITY_SET_IDS: the full set of modifier-isoform sets (all ubiquitin forms incl. linkage-specific chains, plus SUMO/NEDD8/ATG8/…), - discovered from Neo4j by member-gene identity. Falls back to the hardcoded - ubiquitin seed when Neo4j is unavailable (offline / tests). Cached; safe to + discovered from Neo4j by member-gene identity. Cached on success; safe to call in hot loops. + + A failed lookup raises. It used to fall back to the 6-set ubiquitin seed + AND cache that fallback, so one transient Neo4j error decomposed the other + 40 modifier families for every later pathway in the build, with only a + warning (code review 2026-10-02). """ global _modifier_set_cache if _modifier_set_cache is not None: return _modifier_set_cache - try: - from src.neo4j_connector import get_modifier_isoform_entity_set_ids - _modifier_set_cache = ( - get_modifier_isoform_entity_set_ids() | _UBIQUITIN_ENTITY_SET_IDS - ) - except Exception: - logger.warning( - "Modifier-isoform set discovery failed; falling back to ubiquitin seed", - exc_info=True, - ) - _modifier_set_cache = set(_UBIQUITIN_ENTITY_SET_IDS) + from src.neo4j_connector import get_modifier_isoform_entity_set_ids + _modifier_set_cache = ( + get_modifier_isoform_entity_set_ids() | _UBIQUITIN_ENTITY_SET_IDS + ) return _modifier_set_cache @@ -670,7 +668,7 @@ def decompose_by_reactions(reaction_ids: List[str]) -> List[Any]: # multiple reactome_ids in decomposed_uid_mapping (hashes are computed # from sorted components, not reactions). all_best_matches: List[tuple] = [] - emit_one_sided = os.environ.get("LNG_EMIT_ONE_SIDED", "1") == "1" + emit_one_sided = env_flag("LNG_EMIT_ONE_SIDED") for reaction_id in reaction_ids: input_ids = get_reaction_input_output_ids(reaction_id, "input") broken_apart_input_id = [break_apart_entity(input_id) for input_id in input_ids] diff --git a/tests/test_composition_edges.py b/tests/test_composition_edges.py index c98046d..0ad07a0 100644 --- a/tests/test_composition_edges.py +++ b/tests/test_composition_edges.py @@ -99,6 +99,7 @@ def test_flag_default_is_off(monkeypatch): import src.logic_network_generator as lng calls = [] monkeypatch.setattr(lng, "_emit_composition_edges", lambda *a, **k: calls.append(1)) - src = open(lng.__file__).read() - assert 'os.environ.get("LNG_COMPOSITION_EDGES", "0") == "1"' in src + from src.env_flags import env_flag + assert env_flag("LNG_COMPOSITION_EDGES") is False + assert 'env_flag("LNG_COMPOSITION_EDGES")' in open(lng.__file__).read() assert calls == [] diff --git a/tests/test_diagram_set_member.py b/tests/test_diagram_set_member.py index 2ed2f3f..6ec16cb 100644 --- a/tests/test_diagram_set_member.py +++ b/tests/test_diagram_set_member.py @@ -155,5 +155,8 @@ def test_the_emitter_is_actually_wired_in(monkeypatch): gen = inspect.getsource(pg) assert "LNG_DIAGRAM_SET_MEMBER" in gen # off by default, and the documented kill switch still disables it - assert '"LNG_DIAGRAM_SET_MEMBER", "0"' in gen + from src.env_flags import env_flag + monkeypatch.delenv("LNG_DIAGRAM_SET_MEMBER", raising=False) + assert env_flag("LNG_DIAGRAM_SET_MEMBER") is False + assert 'env_flag("LNG_DIAGRAM_SET_MEMBER")' in gen assert "LNG_DIAGRAM_SET_MEMBER" in pg._FINGERPRINTED_ENV diff --git a/tests/test_env_flags.py b/tests/test_env_flags.py new file mode 100644 index 0000000..d4dd108 --- /dev/null +++ b/tests/test_env_flags.py @@ -0,0 +1,47 @@ +"""LNG_* switches take exactly 0 or 1, and an unknown LNG_* name is an error +(code review 2026-10-02: `!= "0"` switches turned on for "false", `== "1"` +switches turned off for "true", and a misspelt name was never read).""" +import pytest + +from src.env_flags import BOOL_FLAGS, env_flag, validate_env +from src.logic_network_generator import _REMOVED_ENV, _reject_removed_env + + +@pytest.mark.parametrize("name", sorted(BOOL_FLAGS)) +def test_switch_values(monkeypatch, name): + monkeypatch.delenv(name, raising=False) + assert env_flag(name) is BOOL_FLAGS[name] + monkeypatch.setenv(name, "") + assert env_flag(name) is BOOL_FLAGS[name] + monkeypatch.setenv(name, "1") + assert env_flag(name) is True + monkeypatch.setenv(name, "0") + assert env_flag(name) is False + for bad in ("true", "false", "yes", "2", "on"): + monkeypatch.setenv(name, bad) + with pytest.raises(ValueError, match=name): + env_flag(name) + + +def test_the_two_reported_inversions_are_errors(monkeypatch): + monkeypatch.setenv("LNG_HANDOFF_EDGES", "false") + with pytest.raises(ValueError): + _reject_removed_env() + monkeypatch.delenv("LNG_HANDOFF_EDGES") + monkeypatch.setenv("LNG_BOUNDARY_HIERARCHY", "true") + with pytest.raises(ValueError): + _reject_removed_env() + + +def test_a_misspelt_name_is_an_error(monkeypatch): + monkeypatch.setenv("LNG_BOUNDRY_HIERARCHY", "0") + with pytest.raises(ValueError, match="LNG_BOUNDRY_HIERARCHY"): + _reject_removed_env() + + +def test_known_names_pass(monkeypatch): + for n in BOOL_FLAGS: + monkeypatch.setenv(n, "0") + monkeypatch.setenv("LNG_MAX_VARIANTS", "512") + monkeypatch.setenv("LNG_PYTHON", "/usr/bin/python3") + validate_env(removed=_REMOVED_ENV) diff --git a/tests/test_export_failfast.py b/tests/test_export_failfast.py new file mode 100644 index 0000000..13fd603 --- /dev/null +++ b/tests/test_export_failfast.py @@ -0,0 +1,76 @@ +"""A pathway whose bundle cannot be written must FAIL, not count as built. + +Code review 2026-10-02: every export after logic_network.csv was wrapped in a +try/except that logged and carried on. The orphan guard in +export_node_reaction_context raises ValueError on purpose, and that error +skipped node_resolution.csv, cofactors.csv and containment.csv, while +generate_pathway_file returned normally. The pathway counted as successful, +the build exited 0 and catalog.sh moved `current`; the consumer then ran its +default model with the self-inhibitor rules inert. A regeneration into an +existing directory also left the previous run's files behind. + +These drive generate_pathway_file itself with the database and network +construction mocked, so the try/except blocks are what is under test. +""" +import pathlib + +import pandas as pd +import pytest + +import src.diagram_connectivity as dc +import src.pathway_generator as pg +from src.logic_network_generator import PathwayResult + +EXPORTS = ["export_uuid_to_reactome_mapping", "export_entity_reaction_proxy_mapping", + "export_nodes", "export_node_reaction_context", "export_node_resolution", + "export_cofactors", "export_containment", "export_drugs", "export_pools"] + + +@pytest.fixture +def mocked(monkeypatch): + monkeypatch.setattr(pg, "get_reactome_release", lambda: 97) + monkeypatch.setattr(pg, "get_reaction_connections", lambda pid: pd.DataFrame( + {"preceding_reaction_id": ["R-HSA-1"], "following_reaction_id": [None], + "event_status": ["No Preceding Event"]})) + monkeypatch.setattr(pg, "get_decomposed_uid_mapping", + lambda pid, rc: (pd.DataFrame({"uid": ["u"]}), [("a", "b", "R-HSA-1")])) + monkeypatch.setattr(dc, "augment_reaction_connections", lambda pid, rc: rc) + net = pd.DataFrame([{"source_id": "x", "target_id": "y", "pos_neg": "pos", "and_or": "and", + "edge_type": "input", "stoichiometry": 1}]) + monkeypatch.setattr(pg, "create_pathway_logic_network", lambda *a, **k: PathwayResult( + net, {"x": "R-HSA-2"}, pd.DataFrame(), pd.DataFrame({"uid": [], "reactome_id": []}), {})) + + def writes(*a, **k): + for p in (x for x in a if isinstance(x, str) and x.endswith(".csv")): + pathlib.Path(p).write_text("x\n") + for name in EXPORTS: + monkeypatch.setattr(pg, name, writes) + return monkeypatch + + +@pytest.mark.parametrize("failing", EXPORTS) +def test_a_failed_export_fails_the_pathway(tmp_path, mocked, failing): + def boom(*a, **k): + raise ValueError("12 of 40 context rows name nodes absent from the logic network") + mocked.setattr(pg, failing, boom) + with pytest.raises(Exception): + pg.generate_pathway_file("R-HSA-1", "x", str(tmp_path)) + + +def test_a_complete_run_writes_every_bundle_file(tmp_path, mocked): + pg.generate_pathway_file("R-HSA-1", "x", str(tmp_path)) + written = {p.name for p in (tmp_path / "R-HSA-1").glob("*.csv")} + assert written == set(pg.BUNDLE_FILES) + + +def test_a_regeneration_leaves_no_stale_file(tmp_path, mocked): + d = tmp_path / "R-HSA-1" + d.mkdir() + (d / "containment.csv").write_text("stale\n") + + def boom(*a, **k): + raise ValueError("refusing") + mocked.setattr(pg, "export_node_reaction_context", boom) + with pytest.raises(Exception): + pg.generate_pathway_file("R-HSA-1", "x", str(tmp_path)) + assert not (d / "containment.csv").exists() diff --git a/tests/test_lookup_failures.py b/tests/test_lookup_failures.py new file mode 100644 index 0000000..b64c763 --- /dev/null +++ b/tests/test_lookup_failures.py @@ -0,0 +1,32 @@ +"""A failed Neo4j lookup must fail the build, not be replaced by an empty or +fallback answer that changes the network (code review 2026-10-02).""" +import pytest + +import src.logic_network_generator as lng +import src.neo4j_connector as nc +import src.reaction_generator as rg + + +def test_modifier_set_failure_raises_and_is_not_cached(monkeypatch): + monkeypatch.setattr(rg, "_modifier_set_cache", None) + + def down(): + raise ConnectionError("neo4j down") + monkeypatch.setattr(nc, "get_modifier_isoform_entity_set_ids", down) + with pytest.raises(ConnectionError): + rg.modifier_isoform_set_ids() + assert rg._modifier_set_cache is None # the failure did not stick + monkeypatch.setattr(nc, "get_modifier_isoform_entity_set_ids", lambda: {"R-HSA-X"}) + assert "R-HSA-X" in rg.modifier_isoform_set_ids() + + +def test_containment_lookup_failure_raises(monkeypatch, tmp_path): + # export_containment imports these inside the function + monkeypatch.setattr(nc, "get_reactome_release", lambda: 97) + + def down(stid): + raise ConnectionError("neo4j down") + monkeypatch.setattr(rg, "get_terminal_components", down) + with pytest.raises(ConnectionError): + lng.export_containment({"u1": "R-HSA-1"}, str(tmp_path / "containment.csv")) + assert not (tmp_path / "containment.csv").exists() From c1e9ecaf7b45a0a5df3026a1c74f28515657567f Mon Sep 17 00:00:00 2001 From: Adam Wright <668955+adamjohnwright@users.noreply.github.com> Date: Fri, 2 Oct 2026 20:21:12 -0400 Subject: [PATCH 2/2] Tests state their offline Neo4j answers instead of leaning on the removed fallbacks 14 unit tests passed in CI only because the modifier-set and participating- entity lookups fell back silently when Neo4j was unreachable, and on a developer machine they quietly queried the live database. An autouse fixture now stubs both for every test not marked database. 338 pass with Neo4j unreachable and with it up. Co-Authored-By: Claude Opus 5.5 (1M context) --- tests/conftest.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/conftest.py b/tests/conftest.py index fb416e7..93ee33b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -17,3 +17,23 @@ def _clear_removed_env_flags(monkeypatch): for name in _REMOVED_ENV: monkeypatch.delenv(name, raising=False) yield + + +@pytest.fixture(autouse=True) +def _offline_neo4j_lookups(request, monkeypatch): + """Unit tests run without Neo4j (CI has none). Two lookups used to fall back + silently when the database was unreachable, and the unit tests leaned on + that: offline they got the fallback, and on a developer machine they + quietly queried the live database. The fallbacks now raise (code review + 2026-10-02), so the offline answer is stated here, explicitly. Tests marked + `database` keep the real lookups.""" + if request.node.get_closest_marker("database"): + yield + return + import src.neo4j_connector as nc + import src.reaction_generator as rg + monkeypatch.setattr(nc, "get_modifier_isoform_entity_set_ids", + lambda: set(rg._UBIQUITIN_ENTITY_SET_IDS)) + monkeypatch.setattr(nc, "get_pathway_participating_entities", lambda pathway_id: set()) + monkeypatch.setattr(rg, "_modifier_set_cache", None) + yield