Repository navigation
fix(petab, experiments): skip protocol blocks in the #907 gate; require directives BNG2.pl reads as PyBNF does (#907, #969) - #975
Merged
Conversation
…othing runs (#907, #969) The PEtab importer writes a fixed parameter's nominalValue into its copy of a BNGL model (#907). Before it does, a guard checks whether the model's own actions set that parameter, since such an action would overwrite the table's value. Since #969 an edition-2 model may carry no such action, so the only thing left for the guard to find was a setParameter inside a `begin protocol` block. The import was refused, saying the block "would override" the table's value. That is not true. BioNetGen only stores a protocol block, and runs it only when a simulate({method=>"protocol"}) action calls it. An imported job never calls it: #969 refuses that action in the model, and PyBNF never writes one. So the block never runs. The review of #969 confirmed this with BNG2.pl: the model with the table's value and the block simulates at the table's value and keeps it. The refusal blocked a problem that imports correctly. The guard now skips lines inside a `begin protocol` ... `end protocol` block, in any case; a setParameter or parameter_scan anywhere else is still caught. The block itself is carried into the copy unchanged. Edition-1 jobs, where a protocol block can run, are not affected: PEtab import always writes an edition-2 job. The maintainer chose this over refusing protocol blocks in edition-2 models. Tests (tests/test_petab_import.py): - test_a_protocol_block_that_nothing_runs_leaves_the_table_value, the #969 review's strict xfail, now passes. Its oracle is BNG2.pl, and the imported job scores 0 at the table's value. - test_the_907_gate_skips_what_a_protocol_block_sets replaces the test that asserted the old refusal. It checks that the guard ignores a protocol block in either case, still sees a setParameter or scan after the block, and is not fooled by a comment mentioning one. On main the guard reports the protocol block's parameter. No changelog fragment: #907's guard has not been released, so between releases nothing changes for users (labelled `changelog exempt`). Independent review is still to come. Signed-off-by: Bill Hlavacek <hlavacek@lanl.gov>
…e, as BNG2.pl does (#907, #969) Review of the change that lets the #907 gate skip a `begin protocol` block. What was tried. Every action that can run a protocol block was checked with BNG2.pl 2.9.3, and each was put through the import: simulate_protocol({}) in an actions block, with a space before the parenthesis, with a line index, and loose after `end model`; parameter_scan and bifurcate with method=>'protocol'. BNG2.pl runs the block for each, and the #969 check refuses each at import, naming the line. simulate({method=>"protocol"}), which the gate's docstring named as the call, is not one of them: BNG2.pl aborts on it ("Simulation method 'protocol' is not a valid option"). PyBNF never writes a protocol call either: import_job(method='protocol') writes a conf that the fitter refuses at load. Delimiter spellings were tried too: extra spaces, tabs, trailing comments, a line continuation and a numbered line inside the block, a block inside `begin model`, `begin protocols`, mixed case, an unterminated block, and a block inside `begin actions`. What was found. BNG2.pl matches `begin protocol` and `end protocol` case-sensitively. It skips a `Begin Protocol` line as unidentified input and runs the setParameter under it as a loose action. It reads `END PROTOCOL` as a line inside the block. The gate matched both delimiters in any case, so it reported nothing for a setParameter under `Begin Protocol`, which BNG2.pl runs, and it refused a problem whose `END PROTOCOL` line sits inside a block that never runs. The first case cannot reach a fit today, because the #969 check (case-sensitive, like BNGLModel's scan) refuses the lines first. But the gate is meant as the second line of defence, and the test pinned the wrong reading. The gate now matches the delimiters as BNG2.pl does, and the docstring names the actions that do run a block. Also found, and not fixed here. BNG2.pl evaluates a directive's options as Perl. So generate_network({overwrite=>1, ($model->simulate_protocol({})) x 0}) runs the protocol block, and the #969 check accepts it as a single generate_network call. With the gate now skipping the block, the import writes v3 = 10 while the job simulates v3 = 3: a check scores 18.375 instead of 0. On main the gate refused this problem. The same hole exists on main without a protocol block: ($model->setParameter("v"."3", 3)) x 0 gets past both checks and gives the same wrong score. The fix belongs in the #969 check (refuse Perl code in a directive's arguments), so it is recorded here as a strict xfail. Tests added (tests/test_petab_import.py): - test_every_action_that_runs_a_protocol_block_is_refused: BNG2.pl is the oracle that each call runs the block; the import must refuse each one. - test_a_protocol_delimiter_counts_only_as_bng2pl_spells_it: BNG2.pl is the oracle for both mis-cased spellings. Both cases fail on the change as first written. - test_a_directive_that_runs_the_protocol_block_is_refused: strict xfail for the hole above. - The gate's parametrized test now expects v3 under `Begin Protocol`, and has a case with tabs, spaces and comments around the delimiters. - A stale comment, which said the gate still covers a protocol block, is corrected. tests/test_petab_import.py, tests/test_petab_export.py and tests/test_model_class.py pass (762 passed, 3 xfailed), and ruff is clean. Signed-off-by: Bill Hlavacek <hlavacek@lanl.gov>
…lues (#969, #907) BNG2.pl runs an action line by evaluating `$model->` and the line as Perl, so a directive's arguments run as code too. ADR-0152 already refused anything after the call's closing parenthesis, but not code inside the parentheses: setOption("NumberPerQuantityUnit", $model->simulate_protocol()) generate_network({overwrite=>1, ($model->setParameter("k",5)) x 0}) Both passed the #969 check on main. The review of the #907 protocol-block fix found the first shape: once the #907 import gate stops counting what a protocol block sets, a directive that runs the block turns a refused import into a silently wrong one. The hole is older than that fix, so it is closed here, in the same PR, rather than left behind it. A directive's arguments may now hold only numbers, quoted strings (a double-quoted one without `$` or `@`, which Perl interpolates), names used as hash keys or as bare values, `=>`, commas, and `{...}` or `[...]` of these. A bare name must be followed by `=>`, `,`, `}`, `]` or nothing; anything else after it makes it a Perl call with arguments (`system "ls"`). The refusal says why, by line, beside the existing reasons. Swept 5,953 BNGL files under the maintainer's model trees: no directive line this refuses that main accepted. The sweep is what admitted bare values, for BioNetGen's MWC.bngl (`max_stoich=>{Protein=>unlimited}`). The strict xfail `test_a_directive_that_runs_the_protocol_block_is_refused` now passes, and is a plain test. Signed-off-by: Bill Hlavacek <hlavacek@lanl.gov>
…lues, not tokens (#969) The plain-values check accepted any sequence of allowed tokens. Perl does not read every such sequence as the values it looks like, and BNG2.pl evaluates a directive's arguments as Perl, so the check passed lines whose meaning it had not seen: generate_network({overwrite=>1,max_agg=>4 -2}) two values, no separator: 2 generate_network({overwrite=>1,max_agg=>010}) octal: 8 setOption("SpeciesLabel","H'Nauty") in an actions block, BNG2.pl turns " into ' first, so the string ends early and Perl parses the rest BNG2.pl builds the first one's network at max_agg 2, the same .net as max_agg=>2. The third aborts BNG2.pl with a Perl parse error, which shows that what PyBNF checked as string text is code to Perl. Bare values were admitted for BioNetGen's MWC.bngl (max_stoich=>{Protein=>unlimited}). BNG2.pl evaluates action lines under `use strict`, and it aborts on that line: 'Bareword "unlimited" not allowed while "strict subs" in use'. It aborts the same way on a bare value in a loose setOption, in an actions-block setOption, and in a list. So a bare value is never data to BNG2.pl, and one it does accept is a call. The bngsim BNGL loader flattens through BNG2.pl, so it reads the line the same way. The arguments must now parse as a list. An element is a number, a quoted string, a name directly before `=>` (Perl's `=>` quotes any name on its left, checked for q, qq, s, y, tr, sub, do, eval, __END__ and others), or a nested {...} or [...]. A `,` or `=>` separates an element from the next. A string holds no quote character of either kind and no backslash, and a double-quoted one no `$` or `@`, so the " -> ' swap leaves it the same string. A number has no leading zero. Sweep of the 5,953 BNGL files the #969 sweep used (4,604 directive lines): one line refused that main accepted, the bare value in a BNGParser test copy of MWC.bngl, which BNG2.pl 2.9.3 aborts on. No real line uses a backslash, a quote inside a string, two adjacent values, or a list. Tests: the cases above and others refused by the scanner's check, with the reason and line; real shapes accepted; a BNG2.pl oracle test showing it aborts on a bare value and on a quote inside a string, and reads the quoted value. The refusal is checked at load, at PEtab export, and at PEtab import, and the import test of the protocol-running directive now requires the plain-values reason instead of any error. Two of the author's accept cases changed: MWC's line is accepted with the value quoted, and an escaped quote is now refused. Signed-off-by: Bill Hlavacek <hlavacek@lanl.gov>
BNG2.pl's reader (BNGModel.pm get_line) joins a line continued with `\` to the next line as that line stands, indentation included, and ends the statement at a blank or comment-only line. The scanner strips each physical line before joining and continues past blank and comment lines. So the #969 check could pass a directive that BNG2.pl reads as something else: setOption("MoleculesObservables",\ # count each dimer once "CountUnique") The scanner reads one setOption and the job loads. BNG2.pl, outside every block, skips both halves with "Unidentified input! Will not attempt to execute as Perl." and runs under BioNetGen's default, CountAll. An edition-2 job fitting a dimer count to 50 e^-kt, scored with BNG2.pl at the true k = 0.5, gave 1964.15. With the option on one line it gave 9.3e-13. Nothing but those two warnings in the per-simulation log shows it. This predates the branch: main's scanner joins past a comment line in the same way. Inside a block, an indented continuation splits a name: `max_\` then ` agg=>2})` passed as max_agg=>2, but Perl reads `max_ agg` (a syntax error here). The plain- values grammar checks what the scanner joined, not what Perl gets. A directive must now be one plain call both as the scanner joins it and as BNG2.pl does. The second reading comes from the raw lines, with each comment and continuing `\` removed and nothing else changed. Lines that are not consecutive (a blank or comment line inside the continuation) are not one statement to BNG2.pl, so they are refused. The refusal gives its own reason and says to continue a directive only between its arguments. Sweep of the same 5,953 BNGL files: no new refusal. The seven continued directives there all break after a comma, and still load. Tests: the scanner refuses the three shapes with the new reason and accepts the real shape and an unindented split. A load-and-export test uses BNG2.pl as the oracle: the model file as written counts each dimer twice. The import refuses both shapes. Signed-off-by: Bill Hlavacek <hlavacek@lanl.gov>
…s BNG2.pl reads it (#969) The previous commit checks a continued directive both as the scanner joins it and as BNG2.pl's reader joins it, but only that each reading is one plain call, not that the two are the same call. They differ when the indentation of a continued line lands inside a quoted string: setOption("MoleculesObs\ ervables","CountUnique") The scanner reads "MoleculesObservables". BNG2.pl reads "MoleculesObs ervables", an option name it does not know, which setOption stores without a word, so it counts under CountAll. Both readings are plain, so the job loaded. The fit writes the scanner's stripped lines and counted the dimer under CountUnique (50 e^-kt, BNG2.pl on the fit's model text); the PEtab export keeps the lines as written, and BNG2.pl on the exported model counted under CountAll (100 e^-kt), as it does on the model file itself. Nothing reports the difference. Main accepted this shape too. A directive must now be the same call both ways: the same directive and the same argument tokens, the space between tokens ignored. A continuation between arguments, or a split with nothing in front of the second half, still reads the same and still loads. The refusal reuses the continuation reason, which already says to continue a directive only between its arguments. Sweep of the 5,953 BNGL files the #969 sweep used: no new refusal beyond the MWC.bngl bare value the previous commits refuse (BNG2.pl 2.9.3 aborts on it, rechecked). Tests: the scanner refuses a split option name, a split label inside an actions block and a split hash key, and accepts the unindented splits and a split before the parenthesis. A load-and-export test uses BNG2.pl as the oracle on the file as written (CountAll) and as the fit reads it (CountUnique), then requires the same refusal at load and at export. The import refuses the shape. The refusal tests fail on the previous commit. Signed-off-by: Bill Hlavacek <hlavacek@lanl.gov>
…ile ends (#969) The scanner joins a line ending in `\` to the next one it reads, and reads the statement when a line does not continue. A statement whose last line continues when the file ends is never read: not as an action, not as a directive, and not by the edition-2 rule. BNG2.pl's reader removes the `\` and runs it. So a model file ending in setParameter("k",5)\ passed the #969 rule. The job loaded, and the PEtab export wrote the line as it stands. BNG2.pl on that file sets k to 5 (the XML it writes after reading the file has k = 5). The fit ran nothing of it: in the model text the fit writes, the line is followed by PyBNF's own `begin actions`, which BNG2.pl joins to it and skips with the rest of the line as unidentified input, before it runs the block's lines as loose actions. So the fit, the export and the model file each mean something different, with nothing but two warnings in a per-simulation log. Blank and comment lines after the statement do not end it for the scanner, so a file that ends in such lines behaves the same. Main has this too. The scan now records such a statement, and the edition-2 rule refuses it with its own reason and the line it starts on, whatever the statement is. Edition 1 is unchanged: the scan reads the file as before, and only the rule reads the new record. Sweep of the same 5,953 BNGL files: no file ends this way. Tests: the scanner refuses a continued setParameter, a continued setOption followed by blank and comment lines, and a continued simulate with no final newline, each with the reason and line. A BNG2.pl oracle test shows BNG2.pl sets k from such a line while the scan reads no action. A load-and-export test requires the same refusal at both, and the import refuses it. The refusal tests fail on the previous commit. Signed-off-by: Bill Hlavacek <hlavacek@lanl.gov>
…ive checks (#969) Signed-off-by: Bill Hlavacek <hlavacek@lanl.gov>
wshlavacek
added
the
changelog exempt
This was referenced Oct 5, 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.
Two related fixes to the edition-2 checks on a BNGL model's actions (#907, #969).
1. A PEtab import no longer refuses a fixed parameter that only a
begin protocolblock sets (#907).The #907 import gate refuses a
parameters.tsvrow withestimate = falsewhen the model's actions set that parameter, because the table value would be overwritten. It counted asetParameterinside abegin protocol...end protocolblock too. Nothing in an imported job runs that block: BNG2.pl runs one only throughsimulate_protocol(...), orparameter_scan/bifurcatewithmethod=>"protocol", and ADR-0152 refuses all of those in an edition-2 model. The gate now skips the block. Like BNG2.pl, it matches the delimiters case-sensitively, soBegin Protocolis not a block, and asetParameteron such a line still counts.2. A directive must be plain data that BNG2.pl reads the same way PyBNF does (#969).
BNG2.pl runs each action line by evaluating
$model->followed by the line as Perl, so a directive's arguments run as code too. ADR-0152 refused anything after the call but nothing inside it. On main:setOption("X", $model->simulate_protocol())passed the check and ran the protocol block. With fix 1 alone, that would have turned a refused import into a silently wrong one.generate_network({overwrite=>1, max_agg=>4 -2})passed, and BNG2.pl built the network formax_agg=>2.max_agg=>010passed, and Perl reads010as octal 8.setOption("MoleculesObservables",\continued past a comment line passed. BNG2.pl skipped both halves with a warning in its own log only, so observables were counted under CountAll. At the true parameter value, the objective was 1964.15 instead of 9.3e-13.A directive's arguments must now be a list of plain values:
=>;{...}and[...]of these.A continued directive is also checked as BNG2.pl's reader joins it, and both readings must give the same call. For example, an indented continuation inside
"MoleculesObs\+ervables"makes BNG2.pl store an unknown option name. A statement still continued with\when the file ends is refused: BNG2.pl runs it, but PyBNF's scanner never read it. Each case is refused at load, PEtab export and PEtab import, with its own reason and line number.A bare value such as
max_stoich=>{A=>unlimited}is now refused. BNG2.pl 2.9.3 aborts on it ("Bareword not allowed while strict subs in use"); quoted, it works.Checks (AGENTS.md)
unlimited, in a test model for BioNetGen's separate parser, which BNG2.pl aborts on.setOptionaftergenerate_networkis moved ahead of it by both the fit and the export; Unicode whitespace around a loose directive; and a misleading message for a trailing\before a blank line.Tests:
tests/test_model_class.py,test_petab_import.py,test_petab_export.py,test_experiment_independence.pyandtest_tutorial_petab_import.py: 862 passed and 2 xfailed (both older and unrelated). Ruff,tools/changelog.py checkand the Sphinx-Wbuild pass.