Record the host in the run logs, and stop losing warnings, tracebacks and diagnostics - #881
Merged
Merged
Conversation
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
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 quicktest runs of the same pipeline agreed bit for bit at 1.0mm but not at 0.8mm, and
nothing in the artifacts said which machine either ran on. PyTorch dispatches its CPU kernels
on the ISA the host reports, so two runners of different generations produce different last
bits, which is enough to flip an argmax at a label boundary. Chasing that turned up that the
logs were missing rather more than the host.
What the logs gain
deep-seg.logandrecon-surf.lognow open with the machine:CPU coresreports affinity and names a cgroup quota separately, becausedocker run --cpus=2leaves affinity at every core. The hostname is deliberately absent:
uname -a,recon-all.log,recon-all.envandrecon-surf.donealready carry it, and in a container it is a random string.Each network then logs one line once its threads and device are final, so the numbers are the
ones the run used rather than torch's defaults:
CPU capabilityis the field that answers the original question: if two runs disagree on it,last-bit differences are expected.
recon-surf.sh,recon-surfreg.shandlong_prepare_template.shreport the torch build too, because neuroreg imports torch, so theirregistration steps run torch kernels.
Things that were reaching no file at all
Measured on a full run by diffing stdout against the log:
setup_logging(None, options.verbose)with--verbosedefaulting to 0, which maps to WARNING, so "Using device", model loading and sliceselection were dropped from every run. It now takes
--seg_loglike the other three networks.recon-surf.sh, so the total segmentation volume and the ventricle intersection volume itcomputes never reached
recon-surf.log.cc_visualization.pyhad it too.nowhere.
logging.captureWarnings(True)insetup_loggingfixes it for all callers.the log ending at the last record. A
sys.excepthooknow writes it to the log and stillprints it to the terminal; Ctrl-C stays a clean interrupt.
Plus the remaining 17 stdout
print()calls in CerebNet and CorpusCallosum are now loggerrecords. After all of it, a full
--seg_onlyrun loses nothing but tqdm progress bars.No first-party script uses a counted verbosity flag any more: all are
-vfor DEBUG, INFO bydefault.
Three bugs found on the way
The callosum networks ignored
--threads.fastsurfer_cc.pyaccepted the flag andrun_fastsurfer.shpassed it, but the module never calledtorch.set_num_threads;FastSurferCNN/utils/parallel.pyonly records the value for FastSurfer's own executors. A--threads 2run gave the CC networks 4 threads. This changes CC output on any run where--threadsdiffers from the core count, so the next quicktest will show it. It is the flagfinally doing what the documentation already says.
14 of 19 error messages in
run_fastsurfer.shcould never print.time_itended withexit, and a function's exit ends the shell it runs in, which is the caller for a plain callbut only a subshell inside a pipeline. So every unpiped call site had a dead error branch. It
now returns and the caller reports which step failed. One call site had no check at all and
relied on the hard exit; it has one now.
RunItgoes through$timecmddirectly, sorecon-surf.shis unaffected.recon-surfreg.shnever setFASTSURFER_HOMEin its fallback branch, unlikerecon-surf.sh:47. Line 274 already builtPYTHONPATHfrom it for the version probe, so astandalone run without the variable exported silently reported
VERSION=unknown. Fixed herebecause the new host-info line needs the same variable; it is a pre-existing bug rather than
anything this branch introduced.
Testing
New:
test/utils/test_host_info.py(23 cases: both/proc/cpuinfopaths, the ARM fallback,sysctl, all four cgroup shapes, affinity, the no-
sched_getaffinitycase, and both sides of thetorch import) and
test/shell/test_time_it.py(28 cases across piped/unpiped and timed/untimed).The static half of the latter, which only reads
run_fastsurfer.sh, is a lint and lives intest/lint/test_time_it_call_sites.py. Both were verified against the old behaviour: exactly thecases that should fail do.
The
test/utilsjob installs no project, so the torch-dependent assertions skip there and theImportError branch is asserted instead; with torch present that one skips and the others run.
98 tests pass, ruff clean,
bash -nclean on all shell scripts.