[Montana]

Fixes #1511.

Important

A .net file the loader reads no block from (a 0-byte file, comments only, or classic-Mac CR line endings) loaded as an empty model with no error. It is now refused, as run_network refuses it. A bare CR is still read as BNG2.pl and run_network read it, as a blank inside its line, and a refusal of a file that holds one now says so.

The problem

A .net file from which the loader reads no block loaded as a model of 0 species and 0 reactions, with no error, and a run returned a trajectory with no columns. On main (0b2055a9), for the issue's network (A <-> B, A(0.4) = 3.839397):

file	bngsim on main	run_network 3.0	this PR
lines ending in a bare CR, behind BNG2.pl's # Created by BioNetGen line	0 species, 0 reactions	refuses: "Couldn't read rates array"	refused, with the CR named
0 bytes; blank lines only; comments only (LF or CRLF); actions only; begin model/end model only; a skipped block only	0 species, 0 reactions	refuses: "Couldn't read rates array"	refused
one bare CR in place of the newline before end reactions	refuses: "'begin groups' before the end of the reactions block", without saying why	refuses: "No reactions in the network"	refused, with the CR named

The issue's fourth row (a reaction dropped, A(0.4) = 3.145303) was already closed by #1539: the marker is now read off the comment-stripped line, so the block's end inside a comment is refused rather than taken. That refusal did not say why the end was missing.

What the referees do with a bare CR

The issue offers two fixes: end lines at a bare CR, or refuse it. Before choosing, I ran run_network 3.0 and BNG2.pl 2.9.3's readFile on 21 edits of the network, plus 2 large CR files. Both read a bare CR as a blank inside its line, never as a line end:

• A CR inside a line changes nothing: in a trailing comment, between two tokens, at a line's start, in the header comment, on a blank line, \r\r\n, \n\r. Both read 2 species and 2 reactions, and run_network gives A(0.4) = 3.839397 for each.
• A CR in place of a newline joins the two lines:
  • BNG2.pl refuses the stray-CR file with "begin block before end of previous block".
  • It refuses the headerless CR-only file with "begin parameters 1 kf 2 … has no matching end".
  • run_network refuses both.

Important

bngsim's loader already reads a CR this way, because trim and split_ws take it as whitespace. On the same edits it gives the referees' results everywhere except the rows in the table. So the fix keeps that reading and adds the refusal the referees have.

The fix

src/net_file_loader.cpp:

• $\color{#8250df}{\textsf{Refuse a file with no block.}}$ A file with none of the five blocks the loader reads is refused. The check is blocks_seen.empty() after the main loop, and the message is "the .net file has no parameters, species, functions, reactions or groups block".
  • It catches: the CR-only file, which is one comment line; the 0-byte and comment-only files; and the rest of the table's second row.
  • A block that is there and empty still loads, as the issue asks: a lone parameters block, an empty one (test_net_reader_param_expressions.py:75), and species with an empty reactions block. run_network refuses those too, so that difference stays as it was.
• $\color{#8250df}{\textsf{Say why a CR file is refused.}}$ A refusal of a file that holds a bare CR says so. parse_net_file_structure now wraps the reading. On the way out, and only then, it scans the file for a CR that is not followed by \n. If it finds one, it adds: "The file holds a carriage return that is not followed by a newline, which BioNetGen reads as a blank inside its line rather than as a line end, so the lines on either side of it are one line; save the file with LF or CRLF line endings".
  • The quoted line is kept readable: the line such a file makes can run to the whole file, and a printed CR returns the cursor over the message. So in this path a CR is shown as \r, and the reading's own message is cut at 300 characters plus " ...".
  • Measured: a comment-free CR copy of fceri_gamma.net (2.4 MB) is refused in 649 characters. Without the cut, the 2,000-parameter test file was quoted whole.
  • No cost on success: the successful load path is unchanged.

python/bngsim/_model.py, _unit_conversions_in (the sibling the issue names):

• The bug: it read the file with read_text, whose universal newlines end a line at a bare CR, so it split lines the loader keeps whole.
• $\color{#8250df}{\textsf{End lines where the loader does.}}$ The fix: it now reads with newline="", which makes its docstring ("Lines end where the loader's end, at a newline and nowhere else") true.
• What it changes: with a CR between #_R1 and unit_conversion=1/Ve, main listed only Vc in frozen_params, so set_param("Ve", 20) again took the value and moved nothing (cBNGL (from_bngl / BNG2.pl .net): set_param, parameter_scan and sensitivities on a compartment-volume parameter are silently accepted but change nothing. The loader throws away BNG2.pl's '# unit_conversion=1/V' annotation and keeps the... #711). Now Ve is listed and the write is refused, as in the LF file.

Docs:

• User guide: docs/user-guide/loading-models.md's parse_net_file section, which lists what the loader refuses "as run_network does", gets a paragraph on the no-block refusal and how a bare CR is read.
• Docstring: Model.from_net's Raises section names the new refusal.

How this was checked

1. Reproduced on main with the issue's script: every row of the table.
2. The referees, on 21 edits and 2 large CR files (above):
   • Agreement: with the fix, every one agrees with run_network, except the explicit empty blocks the issue asks to keep loading.
   • The 8 CR placements both referees read give the LF file's trajectory bit for bit.
   • parse_net_file agrees with Model.from_net on every edit.
3. Nothing that loaded before moves: I loaded every unique .net I have, 179 files (the repository's plus local copies), on main and here, on ea5fcb6e, whose loader code is the final tree's: the last commit changes only a comment there.
   • 179 of 179 give the same model and the same parse_net_file dictionary: species, reactions, parameters, frozen_params, and a digest of the whole dictionary.
   • None holds a bare CR, and none loaded empty on main.
4. Cost and round trip:
   • Cost: Model.from_net on the largest .net here, fceri_gamma.net (3,744 species, 58,276 reactions), median of 7, two rounds each: 119.7 / 124.0 ms on main, 111.3 / 126.3 ms here.
   • Round trip: write_net writes every block, empty or not. So a model of 0 species and 0 reactions (an SBML time-only model, which ModelBuilder accepts on purpose) loads back on main and here, and a control pins it. fceri_gamma written and reloaded keeps its 3,744 species and 58,276 reactions.
5. New tests fail on main for the right reason: 29 cases.
   • test_net_cr_line_endings_and_empty_files.py, 27 cases:
     • 13 fail on main: the CR-only file through Model.from_net and parse_net_file; the CR hint on the two files main already refused; the 2 large CR files, which main quotes whole; and the 7 no-block files.
     • 14 controls pass on main: the 8 CR placements, the 5 empty-block files (an empty functions block and an empty groups block among them, so that every one of the five blocks counts), and the write_net round trip.
   • test_cbngl_volume_folded_into_rates.py, 2 new edits: the CR in the unit_conversion comment fails on main; CRLF line endings, a control, pass.
6. Mutation tests: 9 mutants, all killed, on the final tree:
   • the no-block check removed (9 failing);
   • the bare-CR scan returning false (6);
   • the scan counting a CRLF's CR as bare (1, the CRLF comments-only file);
   • the hint never added (6);
   • CRs not shown as \r (1, the line-outside-any-block file);
   • the message not cut (2);
   • the cut set too high to matter, 1,000,000 for 300 (2);
   • only parameters, species and reactions counting as a block (2, the empty functions and groups blocks);
   • the Python reader back on universal newlines (1).
7. C++ tests: ctest, 8 of 8 pass.
8. Special legs over the 358 test files that load a .net, on ea5fcb6e, whose loader code is the final tree's: the last commit changes only a comment there:
   • BNGSIM_SKIP_AUDIT=strict: 13,769 passed, 84 skipped;
   • forced cc (BNGSIM_CODEGEN_THRESHOLD=1): 13,755 passed, 98 skipped;
   • forced MIR: 13,726 passed, 127 skipped. Main on the same files and leg gives the same 127 skipped, and fails only the new CR edit in test_cbngl_volume_folded_into_rates.py.
9. Full Python suite, macOS arm64, no corpus (as CI runs it), on 0b2055a9, on the final tree: 19,392 passed, 140 skipped, 1 xfailed, 0 failed, so 19,533 collected, main's 19,504 plus these 29.
   • Collected floor: 82 above PYTEST_COLLECTED_FLOOR (19,451), inside the 500 slack.
   • Skips: 140, under PYTEST_SKIPPED_CEILING (252). The new tests have no skip.
10. Docker, Linux x86-64 (python:3.12, --platform linux/amd64, gcc 14, built from ea5fcb6e with the [antimony,hdf5] extras and scipy):

• The two test files: 58 passed, 1 skipped (the BNG2.pl-gated test; the container has no BioNetGen), on ea5fcb6e. The last commit adds the two empty-block controls, which have not run on Linux.
• Full suite: 18,826 passed, 537 skipped, 1 xfailed, 4 failed. The 4 are this container's standing failures (test_codegen_compile_reaping, test_codegen_toolchain_verdict, test_run_tests_sh, test_vendor_script_git_cwd), which fail the same way on main in it and on simulator: leave assignment-rule species at the value a run reports (#1510) #1637's run; none touches the loader.

11. pre-commit --all-files passes on the final tree (13 hooks, clang-format, ruff and mypy among them). ci/changelog.py check passes, and local sphinx-build -W exits 0.

Not explained. A forced-MIR run on an earlier tree, made while the Linux suite ran in Docker beside it, gave 3,688 failures and 121 errors; its output was not kept. They did not come back: the first 40 files alone (1,955 passed), a full rerun on that tree (13,723 passed), and the full leg above all passed, each with a Linux suite running in Docker beside it. I did not find their cause.

Which CI legs this PR runs

The MIR and Windows legs run only on the files in their paths: filters (as on #1629, 60f6528).

This PR changes src/net_file_loader.cpp, python/bngsim/_model.py, two test files, a changelog fragment and a docs page:

leg	runs here?	what covers it
python-tests.yml (Ubuntu, macOS)	yes	the whole suite
native-tests.yml	yes (src/**)	ctest, 8 of 8 locally
windows-tail.yml (MSVC)	yes, through _model.py, but its run list does not hold the new tests	the loader is built and the leg's own tests run under MSVC; the new tests have not run on Windows
mir.yml	no	forced MIR locally, on the 358 files that load a .net (step 8)
windows-klu.yml, windows-nfsim.yml	no	not run: their tests target the KLU and NFsim paths, and this change is the .net reader, built and run under MSVC by the leg above

Checked against past review findings

• More referee cases than the issue names (net: read block markers as BNG2.pl reads them, and refuse what it refuses (#1498) #1539): 23 files through the referees, against the issue's 6. The rule is stated only as far as they go.
• Sibling sites:
  • _unit_conversions_in is fixed and pinned.
  • parse_net_file is pinned through its own test.
  • The deprecated prepare_codegen(net_path) loads through the same core.
  • Not probed, and not a .net: the BNGL readers (_bngl_loader.py, convert/_protocol.py) split their text with splitlines(), which ends a line at a bare CR too. BNG2.pl builds the network there, so only the Python reading of a protocol could differ. That is outside this issue.
  • Left alone: src/table_function.cpp's tfun reader already refuses a CR-only data file loudly ("fewer than 2 data rows"), and nfsim_simulator.cpp:622 reads NFsim XML, not a .net.
• Exception types: unchanged.
  • Model.from_net refuses with ModelError and parse_net_file with ValueError, as before.
  • The wrapper rethrows as std::runtime_error only when the file holds a bare CR. Every caller (both bindings and bngsim_api.cpp) catches std::exception.
• A stated limitation gets a docs note: the user guide and from_net's docstring name the refusal, and the guide notes that an empty block still loads.
• Strict xfails and quarantines citing A .net file with CR line endings loads as an empty model: 0 species and 0 reactions for 2 and 2, with no error (run_network refuses it) #1511: none.
• In-flight PRs: net: read a Hill law with 0 < h < 1 as 0 at zero substrate, as run_network does (#1497) #1629 also changes src/net_file_loader.cpp, in the Hill rewrite. Trial merges with net: read a Hill law with 0 < h < 1 as 0 at zero substrate, as run_network does (#1497) #1629, cvode: take an output at the start of a plain run as reached, as the event-capable loop does (#1501) #1633, result: write the species a Result reports to .cdat, in the core's layout (#1505) #1635 and simulator: leave assignment-rule species at the value a run reports (#1510) #1637 are each clean, so this can merge in any order.
• Builds not run: KLU-off, LAPACK-on and Windows. The change is file reading, the same code in each. On Windows the tests write their files with newline="", and the one edit that uses write_text keeps its meaning, as a bare CR is left alone and an LF becomes CRLF.
• Count bounds, both directions: step 9.
• Tolerances and bounds have their reasons (result: write the species a Result reports to .cdat, in the core's layout (#1505) #1635): the closed-form check's CLOSED_FORM = 1e-6 (measured 7.7e-09) and the refusal's MESSAGE_ROOM = 1000 (measured 583, against 33,000 to 41,000 uncut) are named, with the measurement beside each, and the loader's 300-character cut says why it is 300.
• Claims are measured: each number here comes from a run above.

The diff (abridged)

@@ src/net_file_loader.cpp — the bare-CR scan and the wrapper
+static bool holds_bare_cr(const std::string &path) {
+    std::ifstream file(path, std::ios::binary);
+    char c;
+    while (file.get(c)) {
+        if (c == '\r' && file.peek() != '\n')
+            return true;
+    }
+    return false;
+}
+
 NetFileStructure parse_net_file_structure(const std::string &path) {
+    try {
+        return read_net_file_blocks(path);
+    } catch (const std::exception &e) {
+        if (!holds_bare_cr(path))
+            throw;
+        constexpr std::size_t kMaxShown = 300;
+        std::string what;
+        for (const char *p = e.what(); *p != '\0' && what.size() < kMaxShown; ++p) {
+            if (*p == '\r')
+                what += "\\r";
+            else
+                what += *p;
+        }
+        if (what.size() >= kMaxShown)
+            what += " ...";
+        throw std::runtime_error(
+            what + ". The file holds a carriage return that is not followed by a newline, ...");
+    }
+}
+
+static NetFileStructure read_net_file_blocks(const std::string &path) {

The old body is now read_net_file_blocks, unchanged apart from the check below. The scan runs only after a refusal, and a file with no bare CR rethrows the original exception as it was.

@@ src/net_file_loader.cpp — after the main loop
+    if (blocks_seen.empty())
+        throw std::runtime_error(
+            "the .net file has no parameters, species, functions, reactions or groups "
+            "block: it is empty, or holds only comments, actions or blocks this loader "
+            "passes over");

blocks_seen is the set #803's once-per-block check already fills, so a block that is there and empty counts.

@@ python/bngsim/_model.py — _unit_conversions_in
-        text = Path(path).read_text(errors="replace")
+        with open(path, errors="replace", newline="") as f:
+            text = f.read()

The lines now end where the loader's do, as the docstring says. The from_net docstring's Raises section names the new refusal.

@@ docs/user-guide/loading-models.md — parse_net_file
+A file with none of the parameters, species, functions, reactions and groups
+blocks is refused, as run_network refuses it: a 0-byte file, one of comments or
+actions only (issue #1511). A block that is there and empty still loads. Lines
+end at a newline, LF or CRLF. A bare carriage return is read as BNG2.pl and
+run_network read it, as a blank inside its line, so a file saved with classic
+Mac OS line endings is one line, and is refused, with a word on why.

Tests

• python/tests/test_net_cr_line_endings_and_empty_files.py (new, 27 cases):
  • Refused:
    • the CR-only file, through Model.from_net and parse_net_file;
    • the headerless CR file and the stray-CR file, with the hint;
    • 2 large CR files, whose refusal is cut short and shows its CRs as \r;
    • 7 files with no block, without the hint (including a CRLF one).
  • Read as before:
    • 8 CR placements, bit for bit as the LF file;
    • 5 files whose blocks are there and empty, one per kind of block that holds no species;
    • a 0-species SBML model through write_net and back.
• python/tests/test_cbngl_volume_folded_into_rates.py: 2 more edits in EDITED: a CR in the unit_conversion comment, and CRLF line endings.

[elyfmiller]

Montana, thanks. The fix reproduces here. On the PR head (44219fa, arm64 with MIR), the two test files' 61 cases pass; on its base (0b2055a), 14 fail and 47 pass, as the body says, and v0.17.1 fails the same 14. The branch merges cleanly with today's main (0354cd2), and onto it with #1639, #1642 and #1643, which also touch src/net_file_loader.cpp, _model.py or the loading guide.

What else we checked:

• Every .net file in the repository (185), base against our commit: each loads to the same species, reaction count, parameters, frozen parameters and initial state, and parse_net_file gives the same dictionary.
• \r\r\n line endings, a CR inside each line: the file loads, on the base and here, as the body says.
• The full Python suite on today's main with our commit merged: 19,087 passed, 402 skipped, 1 xfailed and 1 failed. The one, test_run_tests_sh, wants optional extras this machine lacks (jax, diffrax, roadrunner, vivarium-core), and fails the same on v0.17.1. The C++ unit suite, configured as native-tests.yml configures it, passes 6 of 6 on our commit.
• Cost: Model.from_net on fceri_gamma.net, median of 7, two rounds: 96.4 and 97.1 ms on the base, 97.2 and 97.2 here. A CR copy of it (1.9 MB, comments taken off) is refused in 11.8 ms, the scan for a bare CR included, in 687 characters.

We've pushed one commit on top of yours, 36114db, for items 1 to 3:

1. The cut could split a character, and the refusal became a decode error. The reading's words are cut at 300 bytes, and a cut inside a multi-byte UTF-8 character left a byte Python cannot decode.

   • What it did: a CR file whose quoted line holds names of two-byte characters, such as kµµµµ2, raised UnicodeDecodeError: 'utf-8' codec can't decode byte 0xc2 in position 325 from parse_net_file on 4 of 14 one-byte shifts of the text, and from_net's ModelError carried only that. The refusal and the CR hint were both gone.
   • The fix: the cut finishes the character it is in.
   • The new test: test_the_cut_ends_on_a_whole_character moves the cut along such names a byte at a time and checks the refusal and the hint arrive each time. It fails without the fix.

2. Words of exactly 300 characters were marked cut. The test for a cut was the length after the loop, so a whole message of 300 bytes ended in " ...".

   • The fix: " ..." now marks text left unshown.
   • The new test: test_words_of_300_characters_are_shown_whole grows the words one character at a time across 300. It fails without the fix.

3. The hint said the lines were joined where they were not. It is added to any refusal of a file that holds a bare CR, and a CR inside a line changes nothing.

   • What it did: with \r\r\n line endings the file loads, and refused for another reason (a token after a rate law) it was told "so the lines on either side of it are one line".
   • The fix: the hint now says where a CR joins two lines: "BioNetGen reads one as a blank inside its line rather than as a line end, so where one stands in place of a newline the lines on either side of it are one line".
   • The new test: test_a_cr_inside_a_line_is_not_said_to_join_two refuses such a file and checks the hint's wording. It fails without the fix.

Not changed:

4. The cut applies to such a refusal too. A file with a harmless CR refused for another reason gets its words cut at 300 characters. The loader can't tell which CR did what, so we left it.

5. _warn_dropped_state_changes reads a BNGL file with read_text, which ends a line at a bare CR. It's the BNGL side you named as outside A .net file with CR line endings loads as an empty model: 0 species and 0 reactions for 2 and 2, with no error (run_network refuses it) #1511, so it's a note.