Repository navigation
fix(tests): narrow the conftest path filter and make a missing build loud - #53
Merged
Merged
Conversation
…loud
The guard that resolves `bionetgen` to the tree under test filtered sys.path
with a substring test on "BioNetGen". That also strips this repository's
build-artifact directory (`build/cpp`), so in any worktree without its own
build the compiled extension becomes unimportable.
Measured both ways in one worktree, same commit, same .so bytes, only the
path STRING differing (reported by sciStochastic):
PYTHONPATH=python:/tmp/sciStoch/ext -> 888 passed, 54 skipped, 0 failed
PYTHONPATH=python:build/cpp:.../BNG3/build/cpp -> 46 failed, 685 passed, 91 skipped
All 46 are AttributeError: 'NoneType' object has no attribute 'parse_file'
because _cpp imported as None -- none are real regressions. Worse, and the
class this guard exists to prevent: any test file using
pytest.importorskip("bionetgen._bionetgen_cpp") now reports SKIPPED with
exit 0 where its tests should have run.
Two changes:
1. Narrow the filter to the entries actually guarded -- the editable
install's source path and an installed `bionetgen` package directory --
preserving `build/cpp`, which is a legitimate sys.path entry. The
guard's real work is removing the `__editable__` meta_path finder; the
substring test was collateral.
2. Make an unimportable extension RAISE with the resolved bionetgen path,
the surviving sys.path and the underlying error, instead of letting
importorskip convert a misconfigured environment into a quiet pass. A
guard that silences its own tests is passing quietly, which is the
opposite of what it was written for.
Also wires tests/validation/test_parity_nfsim_seed.py into parity.yml. All
three existing invocations named test_parity_nfsim.py by path, so the new
519-line parity module was collected by no job -- the same orphaned-work
class as the uncovered Coverage.lean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow-up to the previous commit on this branch. sciSignaling identified a case the first fix re-broke: dropping "entries not under the worktree root" also drops a legitimate PYTHONPATH pointing at a SHARED build-artifact directory, which is the workaround most agents use. That trades one silent failure for another. The filter now removes the editable install's own source path -- the thing this guard guards -- identified structurally from the finder's search_paths, and nothing else. Verified in a worktree with no build of its own and the extension reachable only via a shared directory: pkg: .../BNG3-fix-segv/python/bionetgen/__init__.py ext: /tmp/guardcheck/ext/_bionetgen_cpp.cpython-314-darwin.so guard fired: True worktree python/ first: True So the shadowing the guard exists to prevent is still defeated, and the shared artifact survives. This worktree's own build/cpp is prepended when present, so a local build takes precedence over a shared one without displacing it. Reported by sciSignaling; mechanism confirmed by orchScope independently, who additionally found the underlying trap: a STALE _bionetgen_cpp*.so vendored inside python/bionetgen/ in 13 worktrees (Sep 27, 5,764,264 B, failing to dlopen with "symbol not found ... simulateNfcore2"). Any sys.path ordering that puts python/ first silently skips every importorskip-guarded file. Those artifacts are removed separately; the one legitimate copy in BNG3-parity is its own verified build and is preserved. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Oct 1, 2026
feat(bngir): round-trip units, count observables, and builtin rate laws; fail closed on the rest
#33
Merged
akutuva21
pushed a commit
that referenced
this pull request
Oct 1, 2026
…path filter PR #53 (1c78a72) landed on main while this branch was open, and it fixes the defect this harness existed to work around: with it, the canonical command PYTHONPATH=python:build/cpp python3 -m pytest tests/python resolves both the package and the extension to the worktree under test. Measured after rebasing: 974 passed, 28 skipped, 2 xfailed, and the extension probe resolves to BNG3-ir-snapshot/build/cpp. So the harness is no longer a workaround. Kept, and rewritten as documentation of the probe rather than a path-patcher, because the check costs two seconds and its absence is invisible. It records the three traps that cost the session time: the probe must run INSIDE a pytest session (conftest decides extension reachability), bionetgen._cpp is inert and reads None on every run, and the sys.modules form works on both the direct and fallback import routes.
akutuva21
pushed a commit
that referenced
this pull request
Oct 1, 2026
…path filter PR #53 (1c78a72) landed on main while this branch was open, and it fixes the defect this harness existed to work around: with it, the canonical command PYTHONPATH=python:build/cpp python3 -m pytest tests/python resolves both the package and the extension to the worktree under test. Measured after rebasing: 974 passed, 28 skipped, 2 xfailed, and the extension probe resolves to BNG3-ir-snapshot/build/cpp. So the harness is no longer a workaround. Kept, and rewritten as documentation of the probe rather than a path-patcher, because the check costs two seconds and its absence is invisible. It records the three traps that cost the session time: the probe must run INSIDE a pytest session (conftest decides extension reachability), bionetgen._cpp is inert and reads None on every run, and the sys.modules form works on both the direct and fallback import routes.
akutuva21
added a commit
that referenced
this pull request
Oct 1, 2026
…gy structurally (#57) * snapshot emitter: refuse illegal symbol kinds, serialize barrier energy structurally Two defects in cpp/bindings/bind_compile_snapshot.cpp, the producer of the BNGIR v0.2 wire object. 1. symbolKindName could emit a symbol kind outside the format's closed enum. It returned "barrier_pattern", "population_type" and "invalid" for SymbolKind::BarrierPattern / PopulationType / Count. The schema's symbol.kind enum is exactly {parameter, function, observable, molecule_type, compartment, reaction_rule, energy_pattern}, so a document carrying any of those fails validation. Each now raises a diagnostic naming the construct instead. Note: pyBngir corrected my initial report here -- bngir.py:986-987 DOES map barrier_pattern/population_type to their sections, so the defect was schema-invalidity only, not unreadability. 2. A barrier's energy was serialized as parser text. CompiledBarrierFactor carries both `energyExpression` (printable BNGL text) and `expression` (a ResolvedExpression); the emitter wrote the former. A wire object required to hold resolved semantics must not carry BNGL source, and the reader walks `expression` as an expression tree, so a bare string is not usable there either. Now emits expressionSnapshot(barrier.expression). Tests: tests/python/test_bngir_snapshot_emitter.py, 16 passed / 2 strict-xfail, compared semantically (model -> IR -> model -> IR) with a structural differ that names any dropped key, plus a guard-the-guard test proving the differ detects a dropped feature. HONEST CAVEAT, stated because it changes how this PR should be read: both emitter changes sit on paths that are UNREACHABLE today. barrier_patterns is always empty on the Python parse path because cpp/bindings/bind_parser.cpp:26-43 omits the finalizeThermodynamicMetadata() call that cpp/parser/BNGAstVisitor.cpp:2039 makes (thermoParse owns that), so barrier symbols and population-type symbols can never reach symbolKindName. I verified this by reverting the emitter to 6889fba and rebuilding: the new tests pass identically before and after the change. They specify intended behaviour and will start asserting for real when barriers are lowered; they do not currently demonstrate it. Two dormant tests are labelled as such rather than presented as gates. Reports (not fixed here, owners notified): - bngir-0.2.schema.json is stale for population_maps; pyBngir ruled the reader authoritative, schema rewrite assigned to provenance/schemas' owner. - bngir.py:1112-1141 _pattern_v02 emits the bond marker AFTER the state, but BNGL requires it before, so any site with both regenerates as `A(b~0.)` and fails to re-parse. Reported to pyBngir. Gates: ctest -j4 -> 100% tests passed out of 461. Full pytest suite isolated to this worktree -> 878 passed, 28 skipped, 2 xfailed. black --check and ruff check clean. * rebase onto origin/main; make this file fail loudly when the extension is missing Two changes, both found by rebasing rather than by review. 1. Rebased onto origin/main (was 22 commits behind; 8d0b9be pyBngir's bngir PR and 8c5c315 the conftest guard both landed). Verified with git rev-list --count HEAD..origin/main -> 0 git diff --name-only origin/main HEAD -> exactly my 5 files 2. The conftest guard from PR #48 strips this worktree's own build/cpp, so the extension became unimportable and this file reported "1 skipped" while pytest exited 0. That is precisely the silent-skip mechanism the session has been culling, sitting in the file whose entire subject is `_cpp._compiled_snapshot`. Replaced pytest.importorskip("bionetgen._bionetgen_cpp") with a hard import, and the same run now fails loudly: before: 1 skipped (passing, testing nothing) after: 16 failed, 2 xfailed, all with AttributeError: 'NoneType' object has no attribute 'parse_string' which is the correct signal. jsonschema keeps its importorskip because it is a genuinely optional validator, not a hard dependency. Re-verified after the rebase, extension resolved inside a pytest session: PKG: .../BNG3-ir-snapshot/python/bionetgen/__init__.py EXT: .../BNG3-ir-snapshot/build/cpp/_bionetgen_cpp.cpython-314-darwin.so ctest -j4 -> 100% tests passed out of 473 pytest tests/python -> 974 passed, 28 skipped, 2 xfailed * verify: restate conftest_isolation as a probe now that #53 fixed the path filter PR #53 (1c78a72) landed on main while this branch was open, and it fixes the defect this harness existed to work around: with it, the canonical command PYTHONPATH=python:build/cpp python3 -m pytest tests/python resolves both the package and the extension to the worktree under test. Measured after rebasing: 974 passed, 28 skipped, 2 xfailed, and the extension probe resolves to BNG3-ir-snapshot/build/cpp. So the harness is no longer a workaround. Kept, and rewritten as documentation of the probe rather than a path-patcher, because the check costs two seconds and its absence is invisible. It records the three traps that cost the session time: the probe must run INSIDE a pytest session (conftest decides extension reachability), bionetgen._cpp is inert and reads None on every run, and the sys.modules form works on both the direct and fallback import routes. --------- Co-authored-by: irSnapshot <agent@local>
This was referenced Oct 1, 2026
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.
Follow-up to #48. The guard that resolves
bionetgento the tree under test filteredsys.pathwith a substring test on"BioNetGen"— which also strips this repository's build-artifact directory (build/cpp). In any worktree without its own build, the compiled extension becomes unimportable.Measured both ways, same worktree, same commit, same
.sobytesOnly the path STRING differs (measurement from
sciStochastic):All 46 failures are
AttributeError: 'NoneType' object has no attribute 'parse_file'atpython/bionetgen/model.py:650because_cppimported asNone. None are real regressions.Worse, and the class this guard exists to prevent: any test file using
pytest.importorskip("bionetgen._bionetgen_cpp")reportsSKIPPEDwith exit 0 where its tests should have run. A guard that silences its own tests is passing quietly.Two changes
bionetgenpackage directory — preservingbuild/cpp. The guard's real work is removing the__editable__meta_path finder; the substring test was collateral damage.bionetgen.__file__, the survivingsys.pathand the underlying error, rather than lettingimportorskipturn a misconfigured environment into thirteen passing-looking tests.Also in this PR
Wires
tests/validation/test_parity_nfsim_seed.pyintoparity.yml. All three existing invocations nametest_parity_nfsim.pyby path, so the new 519-line parity module was collected by no job — the same orphaned-work class as the uncoveredCoverage.lean.Why this was missed
The guard was merged on a genuinely strong three-variant demonstration:
pytest_configureplacement and module-docstring placement both resolvebionetgento the wrong tree while leaving the suite green. Nobody tested it against an agent with no build in its worktree, which is the majority case on a 30-agent host. A fix can be rigorously verified against the failure it was designed for and still break an adjacent case nobody checked.Verified locally:
tests/test_ci_contract.py38 passed, black and ruff clean,parity.ymlparses and the new step is present.