Uh oh!
There was an error while loading. Please reload this page.
benchmarks: create the .net cache gen_networks.py exists to fill (#492) - #494
Merged
Conversation
Phase 1 of the full-network ODE benchmark writes every generated network into NETS = HERE / "nets" and never created that directory. On a machine where it did not exist -- a fresh checkout, or one where the cache had been cleaned to reclaim disk -- every model failed. Expensively, and that is the part worth naming: the generation work happens first and only the write fails, so six models spent 28 s to produce nothing, and the full corpus would spend hours the same way. A missing cache is also exactly the state in which this script is the documented recovery path, so the one script whose job is to *build* the cache was the one script that could not create it. The pass now calls ensure_cache_dir(NETS) before the header, so the "cache: ..." line it prints names a directory that exists rather than one every write is about to fail against. That makes the old failure unreachable on a normal run, which leaves the case it cannot cover: the directory removed *while* a multi-hour sweep is in flight. describe_write_failure() handles that one and says so in those words. It matters because the old message named the wrong thing -- FileNotFoundError on a .net path reads as "this model could not be generated", which is what a genuine BNG2.pl netgen failure looks like too -- and because the console truncates a row's detail, so the path the reader needed was cut off. The new detail leads with the directory, and the row's status is cache_write_failed rather than a bare error, so the console tag alone answers netgen-or-write without needing the detail at all. Nothing downstream enumerates statuses; run_timing.py only compares against "ok". test_gen_networks_cache_dir.py drives main() with a model filter that matches nothing, so it reaches the "nothing to do" exit without BNG2.pl and without generating anything -- which is the ordering under test. #492 was not that the cache was never created, it was that it was not created first. Removing only the ensure_cache_dir call fails that test on its assertion, with the run still printing "cache: <path>" for a path that does not exist. One correction to the issue's third point: the manifest did carry the message, under "detail" rather than "error" -- `.get("error")` is None because there is no such key. The 400-char detail held the full path all along; it was the 70-char console truncation that hid it.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes#492.
Taken as asked — this one is on a recovery path, so the
mkdirand the errnocheck land together.
The fix
ensure_cache_dir(NETS)runs before the header, so thecache: ...line thepass prints names a directory that exists rather than one every write is about
to fail against.
The errno check, paired as you asked
Once the directory is created first, the old failure is unreachable on a normal
run. What is left is the case
mkdircannot cover — the directory removedwhile a multi-hour sweep is in flight — and that is what
describe_write_failure()is for:Three details, matching the three things you said conspired to make it hard to
read:
It names the directory, not the file. And the row's status is now
cache_write_failedrather than a bareerror, so the console tag aloneanswers netgen-or-write without needing the detail at all. Nothing downstream
enumerates statuses —
run_timing.pyonly compares against"ok".The path comes first in the message, because the console truncates a
row's detail at 70 characters. That truncation is why the path was cut off in
your reproduction.
One correction here, and it may save you time later: the manifest did
carry the message, under
detailrather thanerror. There is noerrorkey in a manifest row, so
.get("error")isNonefor every row includingthe healthy ones — the 585
okrows have noerroreither.detailholdsup to 400 characters and had the full path all along. Checking a
netgen_timeoutrow in the current manifest:{"status": "netgen_timeout", "detail": "BNG2.pl netgen timed out after 180.0s"}So the report could have answered it; the key name was the obstacle, not the
record. I have not renamed or aliased the key — that would rewrite the shape
of a tracked 592-row manifest for a naming preference, which is your call to
make, not a fix to slip into this PR.
The test
python/tests/test_gen_networks_cache_dir.py— 6 cases, no BNG2.pl needed,which is the point: it drives
main()with a model filter matching nothing, sothe run reaches its "nothing to do" exit without generating anything. That is
the ordering under test — #492 was not that the cache was never created, it was
that it was not created first.
Negative-verified precisely: removing only the
ensure_cache_dircall (leavingthe helper and everything else in place) fails that one test on its assertion,
with the run still printing
cache: <path>for a path that does not exist. Theother five cover idempotence on the 585-network resume, and the three
classification branches — vanished directory, a real ENOENT with the directory
present, and a non-ENOENT
PermissionError(a full disk or read-only mount isnot a missing directory).
For the run machine
This is safe to take before the measurement phase, on your reasoning: it changes
what happens when the cache is missing, and nothing about what is generated
once it exists. The generated
.netbytes are untouched — the only new codepaths are a
mkdirand an error-message branch that a successful run neverreaches.
The
mkdir -pworkaround stays valid, so nothing forces the timing of this.