Pin the vector ISA in quicktest, and log what each host actually computes - #886
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 could not be compared, and nothing in the artifacts said
why. Sampling the GitHub runner pool explains it: the hardware varies enough to change the
arithmetic, and the CPU name does not predict how.
What the runner pool looks like
Eight
ubuntu-latestrunners, torch 2.7.1+cpu, fingerprinting a convolution, a matmul and asoftmax over bit identical inputs:
Two clean and separate dependencies:
results.
torch.matmul,nn.Linear,einsumorbmmanywhere in the four model directories.Counting only the two operations the networks use, two runs land in the same numerical class 53%
of the time, so nearly half of all quicktest pairs are not comparable. Pinning the ISA collapses
them to one class.
Note the first row: the same CPU model appears with and without AVX512, because the hypervisor
masks it on some VMs. The model name therefore cannot decide whether two runs are comparable,
which is what motivates the second half of this PR.
Pinning, in CI only
Both
docker runinvocations inrun-fastsurfernow set:Both are needed, because they cover disjoint operations: the first moves softmax and leaves
convolution alone, the second the reverse.
avx2rather thanavx512because the cap only worksdownwards, so pinning high would be silently ignored on exactly the runners that cause the split.
Deliberately on the
docker runand not in the Dockerfile. The penalty is roughly 50% onconvolution where AVX512 was available, so users keep the faster kernels and only CI pays. The
comment at the call site records that, so it is not moved into the image without knowing the cost.
A numerical fingerprint in the log
FastSurferCNN/host_info.pygainsnumerical_fingerprint(), which hashes a convolution and asoftmax over inputs built from exact integer arithmetic, so the inputs are identical on every host
and only the selected kernels can move the result:
This is the part that pays off repeatedly: comparing two runs now starts with a one line diff
instead of an investigation, and it identifies the numerical class exactly rather than by proxy.
The digest goes through
int32andstructrather than numpy, which costs about 4ms more andmeans a missing numpy can never be what breaks a log line.
Where it is emitted
Once per log file, at the top, because it describes the host and the host does not change during a
run:
run_fastsurfer.shdeep-seg.log, in the headerrecon-surf.shrecon-surf.logrecon-surfreg.shlong_prepare_template.shThe networks keep their existing torch line and do not repeat the fingerprint. Their line
carries the intra-op and inter-op thread counts, which do differ per process; that is why the
thread counts are per network while the fingerprint is not.
The header omits the thread counts for the same reason in reverse: there they would be torch's
defaults rather than the run's, which is why it did not report torch at all before.
This split relies on the fingerprint not depending on the thread count, since the header runs
before
--threadsis applied. Verified identical at 1, 2, 4 and 8 threads, and asserted bytest_does_not_depend_on_the_thread_countso a future torch version cannot quietly break it.What this does not fix
Pinning closes the AVX512 against AVX2 split. It does not close everything: one observed pair
already agreed on convolution and softmax and still differed by a single voxel in the
segmentation, which then cascaded into different vertex counts and every surface derived number
after them. That residual is unexplained. Removing the large known variable is what will make it
visible and attributable next time, rather than confounded with the ISA.
Testing
Six new tests in
test/utils/test_host_info.py: the fingerprint is stable within a process, hasthe documented shape, stays on one line, does not depend on the thread count, reports plainly when
torch is absent, and, the one that matters most, changes when the ISA is capped. That last one
skips on arm64, where there are no wider kernels to cap, so it will first do real work on the
linux CI run.
Cost of the header now importing torch: 0.04s to 0.66s locally, once per run, against a pipeline
measured in tens of minutes. In exchange three redundant log lines disappear.
Notes for reviewers
The pinning will not be exercised by this PR's own CI.
quicktest.yaml:236resolvesrun-fastsurfer@dev, so the pull request run uses the action fromdevand the two environmentvariables are absent. The fingerprint changes are repository source, baked into the image built
from this branch, so those are exercised. On the first
devrun after merge, confirm the logs showCPU capability AVX2and that both resolutions report the same fingerprint.Quicktest numbers will shift after this lands, because the pinned kernels are not the ones the
current reference was produced with. The reference is already stale and the suite already fails
continuously, so this makes nothing worse, but it is one more reason the refresh has to come after
this rather than before.