From 186c765aaa8f73bc5bf269aa016843a4c8e168b7 Mon Sep 17 00:00:00 2001 From: Bill Hlavacek Date: Sat, 20 Jun 2026 14:49:04 -0600 Subject: [PATCH 1/7] Fix #109: error (not silent subprocess) when bngsim can't inspect actions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Under simulator='bngsim', the routing bridge silently fell back to the legacy BNG2.pl subprocess engine whenever the Python parser (modelapi.bngmodel) could not inspect a BNGL's actions — e.g. a typo'd or unknown simulate argument (atoll, abstol, par_scan_vals) that bngmodel rejects but BNG2.pl tolerates. This violated the simulator='bngsim' contract ("use BNGsim, or error"): the caller got legacy output with no exception and no warning. A parity sweep over 895 models hit this on 9 models. _classify_bngl_actions_for_bngsim() returned ROUTE_SUBPROCESS for the un-inspectable case unconditionally, with no reference to simulator. Fix, at the classify_bngsim_route call site where simulator is in scope: - simulator='bngsim' + un-inspectable actions -> ROUTE_ERROR, propagating the underlying parse reason so the caller learns why it couldn't route to BNGsim. Both consumers (main.py, runner.run) already raise BNGSimError(route.reason) on ROUTE_ERROR. - simulator='auto' keeps the subprocess fallback but emits a one-time WARNING per file (deduped across the ~4 routing queries per run). - simulator='subprocess' is unchanged (returns before action inspection). To surface the reason, the routing parse now returns (actions, reason): _parse_bngl_actions_for_routing and the memoized loader (renamed _load_bngl_routing_actions; _load_bngl_actions_for_routing kept as a thin list-only wrapper for its other callers) carry the parse message. The unavailable-BNGsim case already returned ROUTE_ERROR and is untouched. Adds tests covering the bngsim error, the auto one-time-warning fallback, the subprocess no-inspect path, reason propagation through the cache, and an end-to-end repro of the atoll-typo model from the issue. --- bionetgen/core/tools/bngsim_bridge.py | 90 +++++++++++++++---- tests/test_bngsim_routing_classifier.py | 114 +++++++++++++++++++++++- 2 files changed, 185 insertions(+), 19 deletions(-) diff --git a/bionetgen/core/tools/bngsim_bridge.py b/bionetgen/core/tools/bngsim_bridge.py index 5548a3d9..ad17d5ff 100644 --- a/bionetgen/core/tools/bngsim_bridge.py +++ b/bionetgen/core/tools/bngsim_bridge.py @@ -1310,9 +1310,16 @@ def _bngl_has_protocol_block(bngl_path): # on its next run (matters for long-lived consumers like the VS Code # extension). _CACHE_MISS = object() -# key: (abspath, st_mtime_ns, st_size); value: parsed actions list (or None). -_ROUTING_ACTIONS_CACHE: dict[tuple[str, int, int], list | None] = {} +# key: (abspath, st_mtime_ns, st_size); value: (actions list or None, parse +# failure reason or None). The reason rides along so a strict +# ``simulator='bngsim'`` request can surface *why* inspection failed instead +# of silently downgrading to the legacy subprocess route (see issue #109). +_ROUTING_ACTIONS_CACHE: dict[tuple[str, int, int], tuple[list | None, str | None]] = {} _ROUTING_ACTIONS_CACHE_MAX = 128 +# Paths already warned about in auto mode, so a model whose actions can't be +# inspected for BNGsim routing produces one WARNING per run, not one per the +# ~4 routing queries each ``bionetgen.run`` makes for the same file. +_ROUTING_INSPECT_WARNED: set[str] = set() def _clear_routing_actions_cache(): @@ -1321,16 +1328,18 @@ def _clear_routing_actions_cache(): never needs this because the cache key invalidates on file change. """ _ROUTING_ACTIONS_CACHE.clear() + _ROUTING_INSPECT_WARNED.clear() -def _load_bngl_actions_for_routing(bngl_path): - """Parse BNGL actions for routing only — memoized per file identity. +def _load_bngl_routing_actions(bngl_path): + """Parse BNGL actions for routing — memoized per file identity. - Returns the parsed action items (treat the list as read-only — routing - callers never mutate it) or ``None`` when the file cannot be parsed. - Parse failures fall back to BNG2.pl rather than blocking the legacy - path, and the ``None`` is cached too so a failing parse is not retried - several times per run. + Returns ``(actions, reason)``: the parsed action items (treat the list as + read-only — routing callers never mutate it) with ``reason=None``, or + ``(None, reason)`` when the file cannot be parsed, where ``reason`` is the + underlying parse message. Parse failures fall back to BNG2.pl rather than + blocking the legacy path, and the failure is cached too so a failing parse + is not retried several times per run. """ try: st = os.stat(bngl_path) @@ -1341,19 +1350,30 @@ def _load_bngl_actions_for_routing(bngl_path): cached = _ROUTING_ACTIONS_CACHE.get(key, _CACHE_MISS) if cached is not _CACHE_MISS: return cached - actions = _parse_bngl_actions_for_routing(bngl_path) + result = _parse_bngl_actions_for_routing(bngl_path) if len(_ROUTING_ACTIONS_CACHE) >= _ROUTING_ACTIONS_CACHE_MAX: # FIFO eviction — drop the oldest entry (dicts keep insertion order). _ROUTING_ACTIONS_CACHE.pop(next(iter(_ROUTING_ACTIONS_CACHE)), None) - _ROUTING_ACTIONS_CACHE[key] = actions - return actions + _ROUTING_ACTIONS_CACHE[key] = result + return result + + +def _load_bngl_actions_for_routing(bngl_path): + """Memoized routing action list (or ``None`` on parse failure). + + Thin wrapper over :func:`_load_bngl_routing_actions` for callers that + only need the action items, not the parse-failure reason. + """ + return _load_bngl_routing_actions(bngl_path)[0] def _parse_bngl_actions_for_routing(bngl_path): """Parse a BNGL file's action items via a throwaway ``bngmodel``. - Uncached — :func:`_load_bngl_actions_for_routing` is the memoized - entry point callers should use. + Returns ``(actions, reason)`` — the action list with ``reason=None`` on + success, or ``(None, reason)`` with the parse message on failure. Uncached + — :func:`_load_bngl_routing_actions` is the memoized entry point callers + should use. """ try: import bionetgen.modelapi.model as mdl @@ -1361,14 +1381,33 @@ def _parse_bngl_actions_for_routing(bngl_path): model = mdl.bngmodel(bngl_path) except Exception as exc: logger.debug("could not parse BNGL for BNGsim routing (%s): %s", bngl_path, exc) - return None + return None, str(exc) or exc.__class__.__name__ try: - return list(model.actions.items) + return list(model.actions.items), None except Exception as exc: logger.debug( "could not read BNGL actions for BNGsim routing (%s): %s", bngl_path, exc ) - return None + return None, str(exc) or exc.__class__.__name__ + + +def _warn_routing_inspection_fallback_once(bngl_path, reason): + """Warn once per file when auto-mode can't inspect a BNGL's actions for + BNGsim routing and falls back to the legacy subprocess engine.""" + try: + key = os.path.abspath(bngl_path) + except Exception: + key = bngl_path + if key in _ROUTING_INSPECT_WARNED: + return + _ROUTING_INSPECT_WARNED.add(key) + logger.warning( + "Could not inspect BNGL actions for BNGsim routing (%s): %s. Using the " + "legacy subprocess (BNG2.pl) route. Pass simulator='bngsim' to require " + "BNGsim and surface this as an error instead.", + bngl_path, + reason, + ) def _classify_bngl_actions_for_bngsim( @@ -1628,8 +1667,23 @@ def classify_bngsim_route( if has_protocol is None: has_protocol = _bngl_has_protocol_block(input_path) + parse_reason = None + if bngl_actions is None: + bngl_actions, parse_reason = _load_bngl_routing_actions(input_path) if bngl_actions is None: - bngl_actions = _load_bngl_actions_for_routing(input_path) + # Action inspection failed: the Python parser rejects this BNGL even + # though BNG2.pl may tolerate it (e.g. a typo'd or unknown ``simulate`` + # argument). A strict ``simulator='bngsim'`` request must not silently + # downgrade to legacy BNG2.pl — surface why instead (issue #109). For + # ``auto`` the subprocess fallback stays, with a one-time warning. + detail = parse_reason or "the BNGL actions could not be parsed" + if simulator == "bngsim": + return BngsimRouteDecision( + ROUTE_ERROR, + "simulator='bngsim' was requested but the BNGL actions could " + f"not be inspected for BNGsim routing: {detail}", + ) + _warn_routing_inspection_fallback_once(input_path, detail) return _classify_bngl_actions_for_bngsim( bngl_actions, method=method, diff --git a/tests/test_bngsim_routing_classifier.py b/tests/test_bngsim_routing_classifier.py index 56568066..0203283f 100644 --- a/tests/test_bngsim_routing_classifier.py +++ b/tests/test_bngsim_routing_classifier.py @@ -1,3 +1,4 @@ +import logging import os import textwrap import time @@ -624,9 +625,120 @@ def test_unstattable_path_parses_without_caching(self): bridge._clear_routing_actions_cache() with patch( - f"{BRIDGE}._parse_bngl_actions_for_routing", return_value=None + f"{BRIDGE}._parse_bngl_actions_for_routing", return_value=(None, "boom") ) as parse: bridge._load_bngl_actions_for_routing("/no/such/file.bngl") bridge._load_bngl_actions_for_routing("/no/such/file.bngl") assert parse.call_count == 2 + + def test_parse_failure_reason_rides_along_with_actions(self, tmp_path): + from bionetgen.core.tools import bngsim_bridge as bridge + + bridge._clear_routing_actions_cache() + bngl = tmp_path / "broken.bngl" + bngl.write_text("not valid bngl\n", encoding="utf-8") + + with patch( + "bionetgen.modelapi.model.bngmodel", + side_effect=RuntimeError("argument atoll not recognized"), + ): + actions, reason = bridge._load_bngl_routing_actions(str(bngl)) + + assert actions is None + assert "atoll" in reason + + +class TestUninspectableActionsRouting: + """Issue #109: under ``simulator='bngsim'``, a BNGL whose actions can't be + inspected (the Python parser rejects an argument BNG2.pl tolerates) must + raise — not silently downgrade to the legacy subprocess engine.""" + + def test_bngsim_uninspectable_actions_error_not_silent_subprocess(self): + from bionetgen.core.tools import bngsim_bridge as bridge + + reason = "argument atoll not recognized for action simulate" + with patch( + f"{BRIDGE}._load_bngl_routing_actions", return_value=(None, reason) + ): + decision = bridge.classify_bngsim_route( + "model.bngl", + "bngl", + simulator="bngsim", + bngsim_available=True, + has_protocol=False, + ) + + assert decision.route == bridge.ROUTE_ERROR + assert "bngsim" in decision.reason + assert reason in decision.reason + + def test_auto_uninspectable_actions_fall_back_with_one_warning(self, caplog): + from bionetgen.core.tools import bngsim_bridge as bridge + + bridge._clear_routing_actions_cache() + reason = "argument atoll not recognized for action simulate" + with patch( + f"{BRIDGE}._load_bngl_routing_actions", return_value=(None, reason) + ), caplog.at_level(logging.WARNING, logger="bionetgen.bngsim_bridge"): + first = bridge.classify_bngsim_route( + "model.bngl", + "bngl", + simulator="auto", + bngsim_available=True, + has_protocol=False, + ) + second = bridge.classify_bngsim_route( + "model.bngl", + "bngl", + simulator="auto", + bngsim_available=True, + has_protocol=False, + ) + + assert first.route == bridge.ROUTE_SUBPROCESS + assert second.route == bridge.ROUTE_SUBPROCESS + # One warning per file across the repeated routing queries, not per call. + warnings = [ + r + for r in caplog.records + if r.levelno == logging.WARNING + and "inspect BNGL actions for BNGsim routing" in r.getMessage() + ] + assert len(warnings) == 1 + + def test_subprocess_simulator_does_not_inspect_actions(self): + from bionetgen.core.tools import bngsim_bridge as bridge + + with patch(f"{BRIDGE}._load_bngl_routing_actions") as load: + decision = bridge.classify_bngsim_route( + "model.bngl", + "bngl", + simulator="subprocess", + bngsim_available=True, + ) + + assert decision.route == bridge.ROUTE_SUBPROCESS + load.assert_not_called() + + def test_bngsim_error_surfaces_parser_reason_end_to_end(self, tmp_path): + from bionetgen.core.tools import bngsim_bridge as bridge + + bridge._clear_routing_actions_cache() + bngl = tmp_path / "atoll.bngl" + bngl.write_text( + 'simulate({method=>"ode",t_end=>10,n_steps=>100,atoll=>1e-8})\n', + encoding="utf-8", + ) + err = RuntimeError("argument atoll not recognized for action simulate") + with patch("bionetgen.modelapi.model.bngmodel", side_effect=err): + decision = bridge.classify_bngsim_route( + str(bngl), + "bngl", + simulator="bngsim", + bngsim_available=True, + has_protocol=False, + ) + + assert decision.route == bridge.ROUTE_ERROR + assert "atoll" in decision.reason From 7b0572dbd7015202dbad3d463a97f8c6df3a37dd Mon Sep 17 00:00:00 2001 From: Bill Hlavacek Date: Sat, 20 Jun 2026 14:58:12 -0600 Subject: [PATCH 2/7] Apply black formatting to issue #109 routing tests --- tests/test_bngsim_routing_classifier.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/test_bngsim_routing_classifier.py b/tests/test_bngsim_routing_classifier.py index 0203283f..0a659edd 100644 --- a/tests/test_bngsim_routing_classifier.py +++ b/tests/test_bngsim_routing_classifier.py @@ -658,9 +658,7 @@ def test_bngsim_uninspectable_actions_error_not_silent_subprocess(self): from bionetgen.core.tools import bngsim_bridge as bridge reason = "argument atoll not recognized for action simulate" - with patch( - f"{BRIDGE}._load_bngl_routing_actions", return_value=(None, reason) - ): + with patch(f"{BRIDGE}._load_bngl_routing_actions", return_value=(None, reason)): decision = bridge.classify_bngsim_route( "model.bngl", "bngl", From 43fd7de3504885aa93e49fc14c8a581433c32a02 Mon Sep 17 00:00:00 2001 From: Bill Hlavacek Date: Sat, 20 Jun 2026 15:34:10 -0600 Subject: [PATCH 3/7] Fix #110: accept e-notation and trailing comma in list action args MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ActionList.define_parser's list-valued argument grammar was stricter than BNG2.pl/Perl: arg_type_list matched elements with `pp.Word(pp.nums + ".")` (digits and '.' only) inside a plain `pp.delimitedList`, so it rejected - scientific notation, e.g. par_scan_vals=>[2.3e-10,5.1e-10] - a trailing comma, e.g. par_scan_vals=>[1,2,3,] Both are valid Perl that BNG2.pl parses and runs. When a model used either (commonly par_scan_vals on parameter_scan), modelapi.bngmodel raised BNGParseError, so under simulator='bngsim' the bridge couldn't inspect the actions and silently fell back to the legacy subprocess (the #109 class) — the model never ran on bngsim. Real models hit this: RuleHub's Mitra2019/15-igf1r fits and Salazar-Cavazos2019 CHO_EGFR best-fit. Broaden arg_type_list to use arg_type_expr (which already spans e/E and +/-), allow an empty list, and tolerate one optional trailing comma: arg_type_list = "[" + Optional(delimitedList(quote_word ^ arg_type_expr)) + Optional(",") + "]" Still rejects genuinely-malformed lists (double commas `[1,,2]`, unclosed `[1,2,`). Adds parametrized accept/reject tests over the issue's matrix and the affected real-model forms. --- bionetgen/core/utils/utils.py | 13 ++++++++- tests/test_bng_parsing.py | 52 +++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/bionetgen/core/utils/utils.py b/bionetgen/core/utils/utils.py index 22695eb2..5863ac2d 100644 --- a/bionetgen/core/utils/utils.py +++ b/bionetgen/core/utils/utils.py @@ -576,7 +576,18 @@ def define_parser(self): arg_type_expr = pp.Word( pp.nums + "." + "+" + "-" + "e" + "E" + "(" + ")" + "/" + "*" + "^" ) - arg_type_list = "[" + pp.delimitedList((quote_word ^ arg_type_float)) + "]" + # Match BNG2.pl/Perl list syntax: elements may be quoted strings or + # numeric expressions including scientific notation (arg_type_expr + # covers e/E and +/-), an empty list `[]` is allowed, and a single + # trailing comma `[1,2,]` is tolerated. arg_type_float only spans + # digits and '.', so it rejected `2.3e-10`; delimitedList rejected the + # trailing comma — both are valid Perl that BNG2.pl runs (issue #110). + arg_type_list = ( + "[" + + pp.Optional(pp.delimitedList(quote_word ^ arg_type_expr)) + + pp.Optional(",") + + "]" + ) arg_type_string = quote_word # # BNGL/Perl `=>` auto-quotes its left operand, so dict keys diff --git a/tests/test_bng_parsing.py b/tests/test_bng_parsing.py index feda7f16..b108dc64 100644 --- a/tests/test_bng_parsing.py +++ b/tests/test_bng_parsing.py @@ -1,4 +1,5 @@ import os, glob +import pytest from pytest import raises import bionetgen as bng from bionetgen.main import BioNetGenTest @@ -107,3 +108,54 @@ def test_action_normalization_preserves_double_commas_inside_quotes(): out = _normalize_action_text('something({xs=>"0,,1,,2"})') assert '"0,,1,,2"' in out + + +def _build_action_parser(): + from bionetgen.core.utils.utils import ActionList + + al = ActionList() + al.define_parser() + return al.action_parser + + +# Issue #110: the list-valued action-argument grammar must accept the same +# forms BNG2.pl/Perl runs — scientific notation and an optional trailing +# comma — or `simulator='bngsim'` silently routes these models to the legacy +# stack. Mirrors the model forms seen in RuleHub (Mitra2019, Salazar-Cavazos2019). +@pytest.mark.parametrize( + "action_text", + [ + # Issue repro: e-notation in par_scan_vals. + 'parameter_scan({parameter=>"x",par_scan_vals=>[2.3e-10,5.1e-10],method=>"ode"})', + # Mixed plain and decimal values. + 'parameter_scan({parameter=>"x",par_scan_vals=>[0.3,1,3],method=>"ode"})', + # Zero plus e-notation values. + 'parameter_scan({parameter=>"x",par_scan_vals=>[0.0,0.05e-9,50.0e-9],method=>"ode"})', + # Trailing comma plus e-notation (both gaps at once). + 'parameter_scan({parameter=>"x",par_scan_vals=>[2.3e-10,4.9e-07,],method=>"ode"})', + # Quoted-string list still parses. + 'simulate({method=>"ode",foo=>["a","b"]})', + # Empty list. + 'simulate({method=>"ode",foo=>[]})', + ], +) +def test_action_grammar_accepts_perl_list_forms(action_text): + parser = _build_action_parser() + parser.parse_string(action_text, parse_all=True) + + +@pytest.mark.parametrize( + "action_text", + [ + # Double comma is genuinely malformed at the grammar level. + 'simulate({method=>"ode",foo=>[1,,2]})', + # Unclosed list (no `]`). + 'simulate({method=>"ode",foo=>[1,2,})', + ], +) +def test_action_grammar_still_rejects_malformed_lists(action_text): + import pyparsing as pp + + parser = _build_action_parser() + with raises(pp.ParseBaseException): + parser.parse_string(action_text, parse_all=True) From 02e0ea88b0a0e42b25d69a4f5f3c4a36fac38e0b Mon Sep 17 00:00:00 2001 From: Bill Hlavacek Date: Sat, 20 Jun 2026 16:59:42 -0600 Subject: [PATCH 4/7] Strict simulator='bngsim': error on unrecognized actions, not silent legacy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the #109 contract. classify_bngsim_route returned the _classify_bngl_actions_for_bngsim result verbatim, so under an explicit simulator='bngsim' request a BNGL with an action the router doesn't recognize still silently ran on the legacy BNG2.pl stack (ROUTE_SUBPROCESS was never promoted to ROUTE_ERROR). #109 only closed the parse-failure (actions==None) hole; this closes the unrecognized-action hole. BNGsim can run such a model via the BNG2.pl backend hook — the router just can't confirm an unknown action is safe to delegate — so strict mode now surfaces ROUTE_ERROR with the reason instead of degrading silently. PLA and BNGsim-unsupported methods are genuine BNGsim incapabilities, so they stay on the subprocess route in every mode (auto and bngsim alike); auto keeps the legacy fallback for unrecognized actions too. Threads `simulator` into _classify_bngl_actions_for_bngsim (default 'auto', back-compatible) and promotes only the unrecognized-action branch. Adds tests: bngsim unknown-action -> error, auto unknown-action -> subprocess, and bngsim PLA / unsupported-method -> subprocess. --- bionetgen/core/tools/bngsim_bridge.py | 23 +++++++++-- tests/test_bngsim_routing_classifier.py | 55 +++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 4 deletions(-) diff --git a/bionetgen/core/tools/bngsim_bridge.py b/bionetgen/core/tools/bngsim_bridge.py index ad17d5ff..7581f4f0 100644 --- a/bionetgen/core/tools/bngsim_bridge.py +++ b/bionetgen/core/tools/bngsim_bridge.py @@ -1415,11 +1415,18 @@ def _classify_bngl_actions_for_bngsim( method=None, has_protocol=False, bngsim_has_nfsim=None, + simulator="auto", ): """Classify whether BNGL can use the BNG2.pl-owned BNGsim backend hook. This routing pass only reads action names and method hints. It does not evaluate BNGL expressions or replay any action semantics in Python. + + ``simulator`` only affects the *unrecognized action* decline: BNGsim could + run such a model via the BNG2.pl backend hook, so under a strict + ``simulator='bngsim'`` request that decline is surfaced as ``ROUTE_ERROR`` + rather than silently running legacy. PLA and BNGsim-unsupported methods are + genuine BNGsim incapabilities and stay on the subprocess route regardless. """ if actions_items is None: return BngsimRouteDecision( @@ -1455,10 +1462,17 @@ def _classify_bngl_actions_for_bngsim( continue if atype is not None: - return BngsimRouteDecision( - ROUTE_SUBPROCESS, - f"BNGL action '{atype}' is not a conservative BNGsim route", - ) + reason = f"BNGL action '{atype}' is not a conservative BNGsim route" + if simulator == "bngsim": + # Strict mode: BNGsim could run this through the BNG2.pl + # backend hook, but the router can't confirm an unrecognized + # action is safe to delegate. Surface it instead of silently + # downgrading to legacy (issue #109 contract). + return BngsimRouteDecision( + ROUTE_ERROR, + f"simulator='bngsim' was requested but {reason}.", + ) + return BngsimRouteDecision(ROUTE_SUBPROCESS, reason) if len(sim_actions) > 1: has_backend_hook_workflow = True @@ -1689,6 +1703,7 @@ def classify_bngsim_route( method=method, has_protocol=has_protocol, bngsim_has_nfsim=bngsim_has_nfsim, + simulator=simulator, ) diff --git a/tests/test_bngsim_routing_classifier.py b/tests/test_bngsim_routing_classifier.py index 0a659edd..d06b6a4f 100644 --- a/tests/test_bngsim_routing_classifier.py +++ b/tests/test_bngsim_routing_classifier.py @@ -740,3 +740,58 @@ def test_bngsim_error_surfaces_parser_reason_end_to_end(self, tmp_path): assert decision.route == bridge.ROUTE_ERROR assert "atoll" in decision.reason + + +class TestStrictModeClassifierDeclines: + """Issue #109 follow-up: under ``simulator='bngsim'`` an *unrecognized* + action must error (BNGsim could run it via the BNG2.pl backend hook), but + PLA and BNGsim-unsupported methods are genuine incapabilities and stay on + the legacy subprocess route in every mode.""" + + def test_bngsim_unknown_action_errors_instead_of_silent_legacy(self): + from bionetgen.core.tools.bngsim_bridge import ROUTE_ERROR + + decision = _classify( + "bngl", + simulator="bngsim", + bngsim_available=True, + actions=[_action("someExoticAction"), _action("simulate_ode")], + ) + + assert decision.route == ROUTE_ERROR + assert "bngsim" in decision.reason + assert "someExoticAction" in decision.reason + + def test_auto_unknown_action_still_falls_back_to_subprocess(self): + from bionetgen.core.tools.bngsim_bridge import ROUTE_SUBPROCESS + + decision = _classify( + "bngl", + simulator="auto", + bngsim_available=True, + actions=[_action("someExoticAction"), _action("simulate_ode")], + ) + + assert decision.route == ROUTE_SUBPROCESS + + @pytest.mark.parametrize( + "actions,method", + [ + # PLA: BNGsim genuinely can't run it. + ([_action("simulate_pla")], None), + # Method override BNGsim doesn't support. + ([_action("simulate_ode")], "quadratic"), + ], + ) + def test_bngsim_genuine_incapability_stays_on_legacy(self, actions, method): + from bionetgen.core.tools.bngsim_bridge import ROUTE_SUBPROCESS + + decision = _classify( + "bngl", + simulator="bngsim", + bngsim_available=True, + actions=actions, + method=method, + ) + + assert decision.route == ROUTE_SUBPROCESS From d7070e669474694642a8aadf562da2fe254a3051 Mon Sep 17 00:00:00 2001 From: Bill Hlavacek Date: Sat, 20 Jun 2026 17:25:07 -0600 Subject: [PATCH 5/7] Error on unknown simulation methods instead of silent legacy fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The router lumped "method BNGsim can't run" together as ROUTE_SUBPROCESS, so a malformed/unknown method (a typo like 'oed', or 'quadratic') silently ran on the legacy stack in auto AND bngsim modes. BNG2.pl's simulate() validates the method against its $METHODS hash (cvode/ssa/pla/psa/nf, plus the 'ode' alias) and itself rejects anything else with "Simulation method '...' is not a valid option" — so routing an unknown method to legacy just defers to a less clear BNG2.pl error. Split the unsupported-method handling (_unsupported_method_route): - A method in the valid BNG universe that this BNGsim can't run — pla, or nf without an NFsim build — stays on the subprocess route (genuine/build incapability), in both modes. - A method outside the valid set is malformed and now errors in both auto and bngsim, surfacing it instead of silently degrading. _KNOWN_BNGL_METHODS = {ode, cvode, ssa, psa, pla, nf, rm} is taken verbatim from BNG2.pl's $METHODS keys (+ ode alias, + rm RuleMonkey extension), so valid methods like the cvode alias keep their existing legacy routing rather than newly erroring. Unrecognized *actions* are deliberately NOT treated this way: BNG2.pl supports many actions outside the router's curated list (setVolume, setOption, readSBML, quit, ...), so auto keeps the legacy fallback for them (only bngsim errors, per the prior commit). Methods are a closed, BNG2.pl-validated set; actions are open. Adds parametrized tests: unknown method -> error in both modes; cvode alias and nf-without-nfsim -> subprocess (regression guards, no spurious errors). --- bionetgen/core/tools/bngsim_bridge.py | 43 +++++++++++++----- tests/test_bngsim_routing_classifier.py | 58 ++++++++++++++++++++++++- 2 files changed, 89 insertions(+), 12 deletions(-) diff --git a/bionetgen/core/tools/bngsim_bridge.py b/bionetgen/core/tools/bngsim_bridge.py index 7581f4f0..f3020fb5 100644 --- a/bionetgen/core/tools/bngsim_bridge.py +++ b/bionetgen/core/tools/bngsim_bridge.py @@ -1201,6 +1201,14 @@ def _nfsim_session_kwargs(nf_params): _BNGSIM_NETWORK_METHODS = frozenset({"ode", "ssa", "psa", "rm"}) +# Every method string BNG2.pl's simulate() accepts: its $METHODS hash keys +# (cvode, ssa, pla, psa, nf) plus the documented 'ode' alias for 'cvode', +# extended with 'rm' (RuleMonkey), which the bridge rewrites onto the nf +# backend. A method outside this set is malformed — BNG2.pl itself rejects it +# ("Simulation method '...' is not a valid option.") — so the router surfaces +# it as an error rather than silently shipping it to the legacy stack. +_KNOWN_BNGL_METHODS = frozenset({"ode", "cvode", "ssa", "psa", "pla", "nf", "rm"}) + _BNGL_ROUTING_COMPLEX_ACTIONS = frozenset( { "parameter_scan", @@ -1246,6 +1254,29 @@ def _method_supported_by_bngsim_for_routing(method, bngsim_has_nfsim=None): return False +def _unsupported_method_route(method_name): + """Route a (non-PLA) method the direct BNGsim path can't run. + + A known BNG method this BNGsim build can't run — currently only ``nf`` + when the install lacks an NFsim binary — falls back to the legacy + subprocess. A method outside the valid BNG universe is malformed (BNG2.pl + rejects it too), so it is surfaced as an error in every mode rather than + silently shipped to legacy. + """ + if method_name in _KNOWN_BNGL_METHODS: + return BngsimRouteDecision( + ROUTE_SUBPROCESS, + f"BNGL method '{method_name}' is not supported by the BNGsim route", + method=method_name, + ) + return BngsimRouteDecision( + ROUTE_ERROR, + f"BNGL method '{method_name}' is not a recognized simulation method " + "(expected one of ode, ssa, pla, psa, nf).", + method=method_name, + ) + + def _bngl_action_method_for_routing(action): """Extract only the method hint needed for conservative routing. @@ -1517,11 +1548,7 @@ def _classify_bngl_actions_for_bngsim( "BNGL method override is a BNGsim-supported simulation", method=method_name, ) - return BngsimRouteDecision( - ROUTE_SUBPROCESS, - f"BNGL method '{method_name}' is not supported by the BNGsim route", - method=method_name, - ) + return _unsupported_method_route(method_name) candidate_methods = [] for action in sim_actions: @@ -1552,11 +1579,7 @@ def _classify_bngl_actions_for_bngsim( for method_name in candidate_methods: if not _method_supported_by_bngsim_for_routing(method_name, bngsim_has_nfsim): - return BngsimRouteDecision( - ROUTE_SUBPROCESS, - f"BNGL method '{method_name}' is not supported by the BNGsim route", - method=method_name, - ) + return _unsupported_method_route(method_name) if has_backend_hook_workflow: return BngsimRouteDecision( diff --git a/tests/test_bngsim_routing_classifier.py b/tests/test_bngsim_routing_classifier.py index d06b6a4f..89fe2e9a 100644 --- a/tests/test_bngsim_routing_classifier.py +++ b/tests/test_bngsim_routing_classifier.py @@ -779,8 +779,9 @@ def test_auto_unknown_action_still_falls_back_to_subprocess(self): [ # PLA: BNGsim genuinely can't run it. ([_action("simulate_pla")], None), - # Method override BNGsim doesn't support. - ([_action("simulate_ode")], "quadratic"), + # cvode: a valid BNG2.pl method (the 'ode' alias) the bridge keeps + # on the legacy route — known, so it stays subprocess, not error. + ([_action("simulate_ode")], "cvode"), ], ) def test_bngsim_genuine_incapability_stays_on_legacy(self, actions, method): @@ -795,3 +796,56 @@ def test_bngsim_genuine_incapability_stays_on_legacy(self, actions, method): ) assert decision.route == ROUTE_SUBPROCESS + + +class TestMethodValidationRouting: + """A method outside BNG2.pl's valid set (ode/ssa/pla/psa/nf, the cvode + alias, and the rm extension) is malformed — BNG2.pl rejects it too — so the + router errors in BOTH modes instead of silently routing to legacy. Known + methods this BNGsim can't run (pla, nf without an NFsim build) stay on the + subprocess route.""" + + @pytest.mark.parametrize("simulator", ["auto", "bngsim"]) + @pytest.mark.parametrize("method", ["quadratic", "oed", "euler"]) + def test_unknown_method_errors_in_both_modes(self, simulator, method): + from bionetgen.core.tools.bngsim_bridge import ROUTE_ERROR + + decision = _classify( + "bngl", + simulator=simulator, + bngsim_available=True, + method=method, + actions=[_action("simulate_ode")], + ) + + assert decision.route == ROUTE_ERROR + assert method in decision.reason + + @pytest.mark.parametrize("simulator", ["auto", "bngsim"]) + def test_cvode_alias_stays_on_subprocess_not_error(self, simulator): + from bionetgen.core.tools.bngsim_bridge import ROUTE_SUBPROCESS + + decision = _classify( + "bngl", + simulator=simulator, + bngsim_available=True, + method="cvode", + actions=[_action("simulate_ode")], + ) + + assert decision.route == ROUTE_SUBPROCESS + + @pytest.mark.parametrize("simulator", ["auto", "bngsim"]) + def test_nf_without_nfsim_build_falls_back_not_error(self, simulator): + from bionetgen.core.tools.bngsim_bridge import ROUTE_SUBPROCESS + + decision = _classify( + "bngl", + simulator=simulator, + bngsim_available=True, + bngsim_has_nfsim=False, + method="nf", + actions=[_action("simulate_ode")], + ) + + assert decision.route == ROUTE_SUBPROCESS From 8dfd7c4e4b1db9b2027caa28967c0cfa30c8c7c2 Mon Sep 17 00:00:00 2001 From: Bill Hlavacek Date: Sat, 20 Jun 2026 17:45:55 -0600 Subject: [PATCH 6/7] Make rm RuleMonkey-aware; build incapabilities error under strict bngsim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit method=>"rm" routes to BNGsim's RuleMonkey (via the rm->nf rewrite + backend hook) only when RuleMonkey is installed. But the router marked rm supported UNCONDITIONALLY — _method_supported_by_bngsim_for_routing never checked RuleMonkey, unlike nf/NFsim — and classify_bngsim_route took no rulemonkey arg. So an rm model on a BNGsim without RuleMonkey still classified as ROUTE_BNGL_BNGSIM, then run_bngl_with_bngsim (which hardcodes simulator="auto") silently fell back to legacy — invisibly even under simulator='bngsim'. Same silent-downgrade class as #109. Make the classifier RuleMonkey-aware (thread bngsim_has_rulemonkey through classify_bngsim_route -> _classify_bngl_actions_for_bngsim -> _method_supported_by_bngsim_for_routing) so the decision is made up front where strict mode is enforced, not in the simulator-blind runner. Treat nf-without-NFsim and rm-without-RuleMonkey uniformly as *build* incapabilities (_BNGSIM_OPTIONAL_COMPONENT_METHODS): BNGsim could run them with the component installed, so simulator='bngsim' errors (surfacing "install NFsim/RuleMonkey") while auto falls back to legacy. This matches the existing direct BNG-XML nf behavior (classify already errored there under bngsim). NOTE: this revises the BNGL nf-without-NFsim case shipped in the previous commit (was subprocess in both modes) so nf and rm are consistent and the strict-bngsim silent gap is closed for both. pla stays subprocess in both modes (a categorical incapability, not a missing component); the cvode alias keeps its legacy route. Updates tests accordingly and adds rm coverage. --- bionetgen/core/tools/bngsim_bridge.py | 86 +++++++++++++++++++------ tests/test_bngsim_routing_classifier.py | 42 +++++++++--- 2 files changed, 99 insertions(+), 29 deletions(-) diff --git a/bionetgen/core/tools/bngsim_bridge.py b/bionetgen/core/tools/bngsim_bridge.py index f3020fb5..9a62b927 100644 --- a/bionetgen/core/tools/bngsim_bridge.py +++ b/bionetgen/core/tools/bngsim_bridge.py @@ -1209,6 +1209,13 @@ def _nfsim_session_kwargs(nf_params): # it as an error rather than silently shipping it to the legacy stack. _KNOWN_BNGL_METHODS = frozenset({"ode", "cvode", "ssa", "psa", "pla", "nf", "rm"}) +# Methods that need an optional BNGsim component the build may lack: nf needs +# NFsim, rm (RuleMonkey) needs the RuleMonkey session. BNGsim *can* run these +# when the component is present, so an absent component is a build incapability +# (surfaced as an error under strict simulator='bngsim', legacy fallback under +# auto) rather than a categorical one like pla. +_BNGSIM_OPTIONAL_COMPONENT_METHODS = frozenset({"nf", "rm"}) + _BNGL_ROUTING_COMPLEX_ACTIONS = frozenset( { "parameter_scan", @@ -1243,8 +1250,15 @@ def _nfsim_session_kwargs(nf_params): ) -def _method_supported_by_bngsim_for_routing(method, bngsim_has_nfsim=None): +def _method_supported_by_bngsim_for_routing( + method, bngsim_has_nfsim=None, bngsim_has_rulemonkey=None +): """Return True if a normalized method can be handed to BNGsim.""" + if method == "rm": + # RuleMonkey is an optional BNGsim component, like NFsim for nf. + if bngsim_has_rulemonkey is None: + bngsim_has_rulemonkey = BNGSIM_HAS_RULEMONKEY + return bool(bngsim_has_rulemonkey) if method in _BNGSIM_NETWORK_METHODS: return True if _is_nf_method(method): @@ -1254,25 +1268,45 @@ def _method_supported_by_bngsim_for_routing(method, bngsim_has_nfsim=None): return False -def _unsupported_method_route(method_name): +def _unsupported_method_route(method_name, simulator="auto"): """Route a (non-PLA) method the direct BNGsim path can't run. - A known BNG method this BNGsim build can't run — currently only ``nf`` - when the install lacks an NFsim binary — falls back to the legacy - subprocess. A method outside the valid BNG universe is malformed (BNG2.pl - rejects it too), so it is surfaced as an error in every mode rather than - silently shipped to legacy. + Three cases: + + * A valid method that needs an optional BNGsim component the build lacks + (``nf`` without NFsim, ``rm`` without RuleMonkey) is a *build* + incapability: BNGsim could run it with the component installed, so a + strict ``simulator='bngsim'`` request surfaces it as an error while + ``auto`` falls back to legacy. + * Another known method the BNGsim path doesn't drive directly (e.g. the + ``cvode`` alias) keeps the legacy subprocess route in every mode. + * A method outside the valid BNG universe is malformed — BNG2.pl rejects + it too — so it errors in every mode rather than silently going legacy. """ - if method_name in _KNOWN_BNGL_METHODS: + if method_name not in _KNOWN_BNGL_METHODS: + return BngsimRouteDecision( + ROUTE_ERROR, + f"BNGL method '{method_name}' is not a recognized simulation method " + "(expected one of ode, ssa, pla, psa, nf).", + method=method_name, + ) + if method_name in _BNGSIM_OPTIONAL_COMPONENT_METHODS: + component = "NFsim" if _is_nf_method(method_name) else "RuleMonkey" + if simulator == "bngsim": + return BngsimRouteDecision( + ROUTE_ERROR, + f"simulator='bngsim' was requested but method '{method_name}' " + f"requires BNGsim {component} support, which this build lacks.", + method=method_name, + ) return BngsimRouteDecision( ROUTE_SUBPROCESS, - f"BNGL method '{method_name}' is not supported by the BNGsim route", + f"BNGsim {component} support is unavailable; using legacy route.", method=method_name, ) return BngsimRouteDecision( - ROUTE_ERROR, - f"BNGL method '{method_name}' is not a recognized simulation method " - "(expected one of ode, ssa, pla, psa, nf).", + ROUTE_SUBPROCESS, + f"BNGL method '{method_name}' is not supported by the BNGsim route", method=method_name, ) @@ -1446,6 +1480,7 @@ def _classify_bngl_actions_for_bngsim( method=None, has_protocol=False, bngsim_has_nfsim=None, + bngsim_has_rulemonkey=None, simulator="auto", ): """Classify whether BNGL can use the BNG2.pl-owned BNGsim backend hook. @@ -1453,11 +1488,12 @@ def _classify_bngl_actions_for_bngsim( This routing pass only reads action names and method hints. It does not evaluate BNGL expressions or replay any action semantics in Python. - ``simulator`` only affects the *unrecognized action* decline: BNGsim could - run such a model via the BNG2.pl backend hook, so under a strict - ``simulator='bngsim'`` request that decline is surfaced as ``ROUTE_ERROR`` - rather than silently running legacy. PLA and BNGsim-unsupported methods are - genuine BNGsim incapabilities and stay on the subprocess route regardless. + ``simulator`` affects the declines BNGsim *could* satisfy: under a strict + ``simulator='bngsim'`` request an *unrecognized action* (runnable via the + BNG2.pl backend hook) and a *build incapability* (``nf`` without NFsim, + ``rm`` without RuleMonkey) are surfaced as ``ROUTE_ERROR`` instead of + silently running legacy; ``auto`` falls back. PLA is a categorical BNGsim + incapability and stays on the subprocess route in every mode. """ if actions_items is None: return BngsimRouteDecision( @@ -1542,13 +1578,15 @@ def _classify_bngl_actions_for_bngsim( "BNGL PLA is not supported by BNGsim", method="pla", ) - if _method_supported_by_bngsim_for_routing(method_name, bngsim_has_nfsim): + if _method_supported_by_bngsim_for_routing( + method_name, bngsim_has_nfsim, bngsim_has_rulemonkey + ): return BngsimRouteDecision( ROUTE_BNGL_BNGSIM, "BNGL method override is a BNGsim-supported simulation", method=method_name, ) - return _unsupported_method_route(method_name) + return _unsupported_method_route(method_name, simulator) candidate_methods = [] for action in sim_actions: @@ -1578,8 +1616,10 @@ def _classify_bngl_actions_for_bngsim( ) for method_name in candidate_methods: - if not _method_supported_by_bngsim_for_routing(method_name, bngsim_has_nfsim): - return _unsupported_method_route(method_name) + if not _method_supported_by_bngsim_for_routing( + method_name, bngsim_has_nfsim, bngsim_has_rulemonkey + ): + return _unsupported_method_route(method_name, simulator) if has_backend_hook_workflow: return BngsimRouteDecision( @@ -1605,12 +1645,15 @@ def classify_bngsim_route( bngsim_has_nfsim=None, bngl_actions=None, has_protocol=None, + bngsim_has_rulemonkey=None, ): """Choose the conservative Stage 1 route for a simulation request.""" if bngsim_available is None: bngsim_available = BNGSIM_AVAILABLE if bngsim_has_nfsim is None: bngsim_has_nfsim = BNGSIM_HAS_NFSIM + if bngsim_has_rulemonkey is None: + bngsim_has_rulemonkey = BNGSIM_HAS_RULEMONKEY if simulator not in {"auto", "bngsim", "subprocess"}: raise ValueError( @@ -1726,6 +1769,7 @@ def classify_bngsim_route( method=method, has_protocol=has_protocol, bngsim_has_nfsim=bngsim_has_nfsim, + bngsim_has_rulemonkey=bngsim_has_rulemonkey, simulator=simulator, ) diff --git a/tests/test_bngsim_routing_classifier.py b/tests/test_bngsim_routing_classifier.py index 89fe2e9a..f8ec89fe 100644 --- a/tests/test_bngsim_routing_classifier.py +++ b/tests/test_bngsim_routing_classifier.py @@ -801,9 +801,9 @@ def test_bngsim_genuine_incapability_stays_on_legacy(self, actions, method): class TestMethodValidationRouting: """A method outside BNG2.pl's valid set (ode/ssa/pla/psa/nf, the cvode alias, and the rm extension) is malformed — BNG2.pl rejects it too — so the - router errors in BOTH modes instead of silently routing to legacy. Known - methods this BNGsim can't run (pla, nf without an NFsim build) stay on the - subprocess route.""" + router errors in BOTH modes instead of silently routing to legacy. A build + incapability (nf without NFsim, rm without RuleMonkey) errors under bngsim + but falls back under auto; the cvode alias keeps its legacy route.""" @pytest.mark.parametrize("simulator", ["auto", "bngsim"]) @pytest.mark.parametrize("method", ["quadratic", "oed", "euler"]) @@ -835,17 +835,43 @@ def test_cvode_alias_stays_on_subprocess_not_error(self, simulator): assert decision.route == ROUTE_SUBPROCESS + @pytest.mark.parametrize( + "method,absent_component", + [ + ("nf", {"bngsim_has_nfsim": False}), + ("rm", {"bngsim_has_rulemonkey": False}), + ], + ) + def test_build_incapability_errors_under_bngsim_but_falls_back_under_auto( + self, method, absent_component + ): + from bionetgen.core.tools.bngsim_bridge import ROUTE_ERROR, ROUTE_SUBPROCESS + + common = dict( + bngsim_available=True, + method=method, + actions=[_action("simulate_ode")], + **absent_component, + ) + auto = _classify("bngl", simulator="auto", **common) + strict = _classify("bngl", simulator="bngsim", **common) + + assert auto.route == ROUTE_SUBPROCESS + assert strict.route == ROUTE_ERROR + assert method in strict.reason + @pytest.mark.parametrize("simulator", ["auto", "bngsim"]) - def test_nf_without_nfsim_build_falls_back_not_error(self, simulator): - from bionetgen.core.tools.bngsim_bridge import ROUTE_SUBPROCESS + def test_rm_with_rulemonkey_routes_to_bngsim(self, simulator): + from bionetgen.core.tools.bngsim_bridge import ROUTE_BNGL_BNGSIM decision = _classify( "bngl", simulator=simulator, bngsim_available=True, - bngsim_has_nfsim=False, - method="nf", + bngsim_has_rulemonkey=True, + method="rm", actions=[_action("simulate_ode")], ) - assert decision.route == ROUTE_SUBPROCESS + assert decision.route == ROUTE_BNGL_BNGSIM + assert decision.method == "rm" From 7107d6faf5115b3827fd657e1fa420ec2673c37d Mon Sep 17 00:00:00 2001 From: Bill Hlavacek Date: Sat, 20 Jun 2026 18:18:32 -0600 Subject: [PATCH 7/7] Fix rm routing test to declare RuleMonkey precondition (CI was red) test_atomic_supported_bngl_methods_use_bngsim forced bngsim_available=True but let bngsim_has_nfsim/bngsim_has_rulemonkey default to the ambient module values, which are False in any environment without a feature-complete BNGsim (all CI runners). After "rm" routing was made RuleMonkey-aware, the rm case correctly routes to subprocess there, so the test's hard-coded bngl-bngsim expectation failed on all 15 matrix jobs. Force bngsim_has_nfsim=True and bngsim_has_rulemonkey=True in the test so the "supported methods route to BNGsim" assertion is environment-independent, matching how it already forces bngsim_available. Verified by rerunning the routing + method-defaults suites with both component flags monkeypatched to False (the CI condition): 93 passed. --- tests/test_bngsim_routing_classifier.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/test_bngsim_routing_classifier.py b/tests/test_bngsim_routing_classifier.py index f8ec89fe..eac90926 100644 --- a/tests/test_bngsim_routing_classifier.py +++ b/tests/test_bngsim_routing_classifier.py @@ -204,10 +204,15 @@ def test_required_formats_error_without_bngsim(self, fmt): def test_atomic_supported_bngl_methods_use_bngsim(self, action, expected_method): from bionetgen.core.tools.bngsim_bridge import ROUTE_BNGL_BNGSIM + # Force the optional-component flags, not just availability: rm needs + # RuleMonkey and nf needs NFsim, and the ambient values are False in + # environments without a feature-complete BNGsim install (e.g. CI). decision = _classify( "bngl", simulator="auto", bngsim_available=True, + bngsim_has_nfsim=True, + bngsim_has_rulemonkey=True, actions=[action], )