Skip to content

Code quality pass: precompile workload, honest size validation, leaner deps - #138

Merged
IanButterworth merged 19 commits into
masterfrom
code-quality
Aug 7, 2026
Merged

Code quality pass: precompile workload, honest size validation, leaner deps#138
IanButterworth merged 19 commits into
masterfrom
code-quality

Conversation

@IanButterworth

Copy link
Copy Markdown
Collaborator

Note

This PR was written by Claude (Claude Code), from a code-quality review requested and supervised by @IanButterworth.

A quality pass over the whole package — no behavior changes beyond those called out below, everything sized for a minor release. One fix per commit. Verified locally: v2/v3_tiny/v4_csp/v7x still match the stored detection references at 1e-4, plus NMS unit checks, prepare_image input varieties, and all three draw_boxes paths.

Highlights

  • Precompile workload — PrecompileTools was a dependency with no workload; the (recently fixed) dummy-weight path now compiles cfg parsing, model construction, and the full inference+NMS pipeline at precompile time. Time-to-first-inference: ~12.8 s → ~0.7 s (M2 Pro, warm load ~3.8 s).
  • Input-size validation checks the real constraintassertdimconform required width/height divisible by the first conv's filter count (an output-channel number, spatially meaningless; it wrongly rejected yolov7x at 416, whose first conv has 40 filters). A new max_stride(cfgvec) walks the layer blocks tracking cumulative downsample (conv/maxpool multiply, upsample divides, reorg multiplies, routes adopt their source's scale) and validates against the true maximum. Verified: 32 for all classic models, 64 for v4_p6; invalid sizes still throw; unit tests added.
  • benchmark() moved behind a BenchmarkExt extension — BenchmarkTools and PrettyTables were hard deps used only by this one utility; they're now weakdeps, and calling benchmark() without them raises a MethodError with a hint. Also found and fixed along the way: the table printing was already broken under PrettyTables v3 (allowed by compat; renamed kwarg), and the hardcoded model list was stale — it's now keyed by YOLO_MODELS name and covers all 15 models at their native sizes.
  • Reference integrityget!(RES_REFS, key, computed) let a new model/image combination pass trivially by self-blessing on first run; missing references are now hard failures. The generation script (darknet-parity-asserting, per-model regeneration, same reduced test sizes as the suite) is committed at dev/generate_test_references.jl — round-trip verified.
  • AllocWrappedModel parameterized — concrete field types on the default CPU entry path instead of dynamic dispatch per call.

Cleanups

  • Removed verified-dead code: flipdict, createcountdict (no call sites), lhtan_grad (training-only gradient), the pre-1.9 get_extension fallback shim (unreachable on 1.10+), redundant usings, and the cu_functional guard in CUDAExt — which held a CUDA.allowscalar(false) that never executed. Notably it is deleted, not resurrected: allowscalar is a global CUDA.jl session flag that a library shouldn't impose on its users (thanks @IanButterworth for catching that in review).
  • linear(x) = x replaced by Base.identity, which the codebase already used for the same purpose.
  • Weights-file training metadata (seen/seen_images) is kept in cfg instead of read-and-discarded.
  • Flux compat narrowed to CI-tested versions (0.14.1, 0.15, 0.16) — 0.12/0.13 were advertised but never exercised, and predate the extension mechanism.

Deliberately deferred (bigger refactors, own PRs)

  • Concrete structs for the per-output Dict{Symbol,Any} post-processing state (touches Adapt/Functors traversal used by the GPU/bumper paths).
  • Decomposing the ~300-line Yolo constructor into parse/assemble/precompute stages.
  • The dispatch-based prepareimage.jl redesign (the #TODO: Make this multiple-dispatchy — most user-facing surface, wants its own test-backed PR).

🤖 Generated with Claude Code

IanButterworth and others added 19 commits August 7, 2026 10:29
Neither has any call site in src, test, or examples.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Gradient kernels are training-only; the package does no training and
nothing references it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
cu_functional() held a CUDA.allowscalar(false) call but was never
invoked, so it never ran. allowscalar is a global CUDA.jl session flag,
so a package should not impose it on load either -- scalar-indexing
policy belongs to the user. Delete the dead function and its Ref.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Base.get_extension exists since Julia 1.9 and the package requires
1.10, so the include branch was unreachable.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The including module already imports these.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Concretely-typed fields avoid dynamic dispatch on every call through
the wrapper, which is the default CPU entry path.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
seen/seen_images were read (necessarily, to advance the stream) and
then dropped; store them in cfg where darknetversion already lives.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The hardcoded constructor list with numeric select indices was stale
(none of the six new models) and fragile. Iterate YOLO_MODELS keys
instead; each model loads at its native default size.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Compat allows PrettyTables 2 and 3, but v3 renamed the header kwarg to
column_labels, so benchmark()'s final table throw a MethodError on any
v3 resolve. Gate on pkgversion.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
linear(x) = x duplicated identity; the codebase already used identity
elsewhere for the same purpose.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
assertdimconform required width/height divisible by the FIRST CONV'S
FILTER COUNT -- an output-channel number with no relation to spatial
downsampling. It worked for the classic models only because their first
conv happens to have 32 (or 16) filters, and it wrongly rejected valid
sizes (yolov7x at 416, first conv filters=40).

Compute the real constraint instead: walk the cfg blocks tracking each
layer's cumulative downsample (conv/maxpool multiply by stride, upsample
divides, reorg multiplies, routes adopt the referenced layer's scale)
and require divisibility by the maximum. Verified to give 32 for all
classic models and 64 for v4_p6, matching their head strides; v7x now
constructs at 416 and invalid sizes still throw.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both draw_boxes! variants recomputed the identical image/model ratio
and coordinate-index mapping; factor it into _box_geometry.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PrecompileTools was a dependency with no workload. Use the (recently
fixed) dummy-weight path to compile cfg parsing, model construction and
the full inference + NMS pipeline at precompile time: time-to-first-
inference in a fresh session drops from ~12.8s to ~0.7s on an M2 Pro
(package load ~3.8s warm).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
BenchmarkTools and PrettyTables were hard dependencies used only by the
benchmark() utility, taxing load time for every detection-only user.
They are now weakdeps triggering a BenchmarkExt extension; calling
benchmark() without them loaded raises a MethodError with a hint
explaining what to load.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
0.12/0.13 predate the extension mechanism this package relies on and
have never been exercised by CI; advertising them is risk without
evidence.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
get!(RES_REFS, key, computed) meant a new model/image combination
passed trivially on its first run without comparing anything. Missing
keys are now test failures, with the regeneration script referenced.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The script that produced resrefs.jl and the reference images lived
outside the repo; anyone needing to regenerate references (new model,
intentional output change) had to reconstruct it. It regenerates any
subset of models, asserts darknet parity for every model/image
combination before blessing, and uses the same reduced test sizes as
the suite. Round-trip verified on v3_tiny (max ref drift 9e-7, within
the suite's 0.05 tolerance).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Everything raised across the bug/performance/model-coverage/quality
review series that was not fixed in #131/#132/#137/#138, with effort
estimates and the explicitly-rejected items recorded so the reasoning
is not lost.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@IanButterworth
IanButterworth merged commit 0655036 into master Aug 7, 2026
12 of 15 checks passed
@IanButterworth IanButterworth mentioned this pull request Aug 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant