Repository navigation
nfsim: a load leaves the process's working directory alone, and a failed one names the file (#1314) - #1334
Conversation
…d that throws names the file (lanl#1314) create_system() changed into the XML's directory so NFsim could open a TFUN's file= by relative path, and changed back as plain code after the load. An exception out of NFinput::initializeFromXML (initCompartments' convertToDouble on a malformed size: the one loader stage with no catch-all) skipped the change back, and left the host in the XML's directory; its error was NFsim's bare message, with no file name. The directory is now never changed. tfun_paths.hpp writes each relative file= in <ListOfFunctions> absolute, against the original XML's directory (not the override or seed-round copy's), escaped, in the copy NFsim reads. That also closes the window in which every other thread saw the XML's directory, and the swallowed restore failure goes with it. A load that throws is reported as one that returns NULL: the file, the reason, and NFsim's log. The stream redirection and NFutil's shared RNG are documented in the network-free guide and the class comment. That interleaved sessions do not each reproduce their seed is pinned on a TLBR model, as a strict xfail for the fix of the three vendored draws; on simple_system, whose matching molecules are interchangeable, it does not show (the control).
|
Thanks. We reviewed this independently, Opus 5.5 and Fable 5.1, each on our own build of cdc4f10 merged onto main c0877fa (clean), on macOS arm64. For XML as BNG2.pl writes it, the fix does what it says:
One medium finding: the rewrite reads less of the XML than NFsim does, so some valid files that loaded on main no longer do, or read the wrong table. Then one tracking point and some low items. What we checked comes first, so it needn't be redone. What we checked
1. The rewrite misses layouts NFsim accepts
On main, the directory change made any relative path resolve. Here, a
In the last column, the cwd held a bngsim writes none of these layouts itself, so the reach is hand-edited, minified or third-party XML. But it worked before, and it can now be wrong without saying so. The same rewrite already exists in Python for RuleMonkey's copies, in On the error text: when a path is missed, the message is 2. The RNG xfail points at the issue this PR closesThe strict xfail's reason names #1314, and this PR closes #1314; #283, the index, is closed too. An open-issue search for A follow-up issue for the three draws, named in the reason, would keep it. The reason could also say that a re-vendor of NFsim that changes those draws flips it, so that red run explains itself. Low
Not checked: Linux and Windows beyond CI; the fixture regenerated from |
…l#1314) Review items on lanl#1334: - write_tfun_absolute_xml scanned the XML a line at a time, from the line after <ListOfFunctions>, for a literal file=" or file='. NFsim reads the attribute with TinyXML, which allows whitespace round the =, a value in either quote or none, and a tag that shares a line with others or runs over several. A file= the scan missed was opened against the process's working directory: the load failed there, or read a table of the same name, and ran on it without a word. The scan now reads the whole text, tag by tag, skipping comments and CDATA, and takes each <Function>'s attributes as TiXmlAttribute::Parse does. - A path with a root is left as written (has_root_path, not is_relative), so on Windows /data/t.dat keeps its drive. - The TFUN tests compare with the run from the XML's own directory, with a table of the same name and every multiplier 0 in the cwd, where they checked only that the observables were finite. New cases cover the layouts the line scan missed. - The RNG xfail takes raises=AssertionError, and its reason cites lanl#1438, which tracks the three shared-RNG draws once lanl#1314 closes, and says a re-vendor that changes those draws flips it.
…rows bring (lanl#1314) The review commit's nine TFUN layout rows take the windows-nfsim leg to 315 passed (d35445c on main 959a0ca: 315 passed, 1 xfailed, 0 skipped). That is 17 above the floor of 298 set by cdc4f10, and NFSIM_PASSED_MAX_SLACK allows 16, so the count guard failed the leg with every test green. The floor goes to 307, 8 below the count as its note says, and the note's count to 315.
|
Following up on our review above: we've pushed the review items as two commits on top, d35445c and d5d1a0f, and we're merging now that CI is green on them. The working directory left alone, the error that names the file, the copies and the RNG pin all stand as you wrote them. What d35445c and d5d1a0f change
On the error text in item 1: the scan now reads what TinyXML reads, so it no longer misses a path. A table that really is missing fails as it does on main, so we left the message alone. What we checked
|
Fixes #1314 (item 1, and item 2's documentation; the vendored RNG patch is pinned for a follow-up, as the issue allows).
The bug: a failed load left the host in the XML's directory
create_system()changed into the XML's directory, so NFsim could open a TFUN'sfile=by relative path, and changed back as plain code afterinitializeFromXML. A load that threw skipped the change back.initCompartments'convertToDoubleon a malformedsizeis that throw: the one loader stage with no catch-all. The host was then left in the XML's directory, and the error was NFsim's bare message with no file name.Reproduced on main with the issue's fixture, a compartment
size="1.0abc"loaded from another directory:The fix: never change directory
As the issue prefers, there is no change of directory at all:
src/tfun_paths.hpp(write_tfun_absolute_xml) writes each relativefile=inside<ListOfFunctions>as absolute, in the copy NFsim reads. Each one resolves against the original XML's directory, even when NFsim loads an override or seed-round copy from the temp directory. The inserted directory is XML-escaped, and the written value is kept as encoded.catch (...) {}), and the window in which every other thread saw the XML's directory.Item 2: streams and the RNG
Documented in the network-free guide ("What an NFsim run changes in your process") and in
NfsimSimulator's class comment:std::cout/std::cerrswap, and that C++ output from other threads in the window is captured while Cstdiois not;The RNG leak is confirmed and pinned, not fixed. On a TLBR model, preparing a second session between a seeded session's two
simulate()segments moves its trajectory, by up to 8 molecules. That test isxfail(strict=True), so the fix of the three vendored draws (ReactantList::pickRandomandreaction.cppL771) flips it into a failure to remove. That fix changes same-seed trajectories, whichnfsim_v1143_compatexists to keep, so it belongs in its own PR, the #283 item.The leak shows only where the molecules a rule can fire on differ: in
simple_systemthey are interchangeable, so the pick never shows. That case is kept as the control.The new fixture
tests/data/nfsim/tlbr_shared_rng.xmlistlbr_8(wshlavacek/BNGL-Models@81c90d8f, already vendored underparity_checks/bng_parity/models), the BNG-XML from thebng_nfreference archive, with a provenance header.Tests (
python/tests/test_nfsim_process_state.py)Fail on main, pass here:
set_parambeforeinitialize());The throw tests drive the core
NfsimSimulator: bngsim's Python layer refuses a compartment model before NFsim reads it (#1067).Pins, passing on both:
file=loads from elsewhere;a&b 'c'(escaping);xfail(strict): another session between segments moves a seeded TLBR trajectory.
Mutations of the C++, each rebuilt and caught:
Suites:
Out of scope, as the issue says: the ~75 vendored
exit()calls.