Drop the LNG_BOUNDARY_LEAF_REUSE escape hatch - #93
Merged
Merged
Conversation
…rror 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
Contributor
Author
|
Adversarial review of the diff found the guard was decorative on both halves, and both are fixed in f5cf852. It had no runtime coverage. The only test asserted a string appeared in It could not stop a run even when it fired. Three mutations are now caught: dead guard body, missing entry-point call, restored exit-0-on-failure. One finding deliberately not fixed here. Also: a conftest clears removed names suite-wide (this diff otherwise turned a stale export into 10 spurious failures, up from 1), unused fixtures dropped, and a removed-flags table added to PIPELINE_SETTINGS.md. CI green against head f5cf852: ruff, test 3.10/3.11/3.12. |
adamjohnwright
added a commit
that referenced
this pull request
Sep 21, 2026
…n 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) <noreply@anthropic.com>
adamjohnwright
added a commit
that referenced
this pull request
Sep 21, 2026
…tral) (#94) * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First item in the flag-cleanup queue (deltasignal
specs/020-variant-node-sharing/cleanup-plan.md): a flag expires once the question it was added to ask has been answered.The question, and the answer
LNG_BOUNDARY_LEAF_REUSE=anyrestored the pre-fix behaviour, where a boundary-decomposition leaf could reuse a node the root complex reaches downstream. That welds a cycle Neo4j never had:root complex -> reactions -> ... -> produced protein -> (assembly) -> root complex. On the v97 catalog, 1,994 of the 2,077 cycle-carrying assembly edges had exactly that shape.Full record: deltasignal
specs/018-derived-edge-loops/research.md. The gain is 94% concentrated in DSB Repair and is mostly DOWN→NORMAL corrections, which is stated there rather than hidden.Why the name stays as an error
Deleting the lookup outright would let a stale
LNG_BOUNDARY_LEAF_REUSE=anyin a shell, a compose file or an old benchmark script be silently ignored — the run would measure the default while the operator believed it was measuring the old mode. That is the silent-input bug class this project has hit four times (DS #40-43), and the reproduction commands in specs/016-019 are exactly the scripts most likely to carry a stale value.So the variable is still read, and setting it to anything raises, naming the spec:
Tests
The two mode tests are replaced by one that asserts that error for three values, including both that used to be valid. The four behavioural tests are unchanged and still pass, including the invariant that no assembly source is reachable from its own complex.
tests/test_boundary_leaf_reuse.pyruff check src/ bin/mypy src/The
ruff formatgap on this file predates the change and is repo-wide (15 files); reformatting here would bury a 10-line diff.🤖 Generated with Claude Code