docs: pretrained model zoo, weight-availability clarity & loud missing-weights warning - #457
docs: pretrained model zoo, weight-availability clarity & loud missing-weights warning#457frgfm wants to merge 7 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #457 +/- ##
==========================================
+ Coverage 93.80% 93.89% +0.08%
==========================================
Files 62 62
Lines 3794 3799 +5
==========================================
+ Hits 3559 3567 +8
+ Misses 235 232 -3
🚀 New features to boost your workflow:
|
Code Review — PR #457Overall this is a well-scoped, high-value PR. The problem statement is precise, the Quick Tour fix is correct, and the loud missing-weights warning is a much-needed improvement. The docstring work on 1. Duplicated warning message — extract a helperThe same
Problem: When the message needs updating (e.g. adding a model name, changing the docs URL), you'll have to find and change both. The two paths also produce slightly different user-facing text for the same situation, which is confusing. Suggestion: Extract a single # checkpoints.py
def _warn_no_pretrained(model_name: str | None = None) -> None:
target = f" for {model_name}" if model_name else ""
msg = (
f"No pretrained weights are available{target}; it will be randomly initialized. "
"Browse the model zoo (https://frgfm.github.io/holocron/) or train your own "
"(https://github.com/frgfm/Holocron/tree/main/references)."
)
logger.warning(msg)
warnings.warn(msg, UserWarning, stacklevel=3)Then in 2. Model zoo table duplication — maintenance trapThe identical 29-row table is maintained in both Suggestion (minimal): Add a comment at the top of each table in both files: <!-- SYNC: this table must match the identical table in <other file> -->Better long-term: Auto-generate the table from the 3. Test coverage gap —
|
| Item | Severity | Effort |
|---|---|---|
| Extract shared warning helper | Medium | Low |
| Deduplicate model zoo table | Low (now) / Medium (over time) | Low |
Test _handle_legacy_pretrained path |
Medium | Low |
poly_loss functional docstring gaps |
Low | Low |
darknet24 accuracy — explanation |
Low | Trivial |
The PR is solid. The warning dedup and the missing test are the two items I'd address before merging. The rest are nice-to-haves that can ship as follow-ups.
Review: PR #457This is a well-structured PR with clear motivation, thorough investigation, and thoughtful implementation choices. The doc improvements address real user friction, and the warning behavior change is exactly the right level of intervention. A few areas worth tightening before landing: 1. Model zoo table is hardcoded in two places (maintenance debt)The table appears in both Suggestion: Define the zoo data once (e.g., a dict or CSV in a script) and have a small generation step that writes both files, or at minimum add a comment at the top of each table pointing to the other copy so maintainers know. 2. Test coverage gapsOnly one test was added ( Missing tests:
3.
|
Review SummaryThis PR effectively addresses critical documentation gaps that were causing user confusion around pretrained weights and loss functions. The changes are well-organized and solve real pain points identified in user issues. Strengths
Suggestions for Improvement
Minor Nitpicks
ConclusionThis is a strong PR that significantly improves the user experience. The changes are focused, well-documented, and address the root causes of user friction. With the minor suggestions above, it would be even more robust. LGTM - looks good to merge! |
frgfm
left a comment
There was a problem hiding this comment.
Review
Solid PR — the cluster-by-issue approach is the right way to make a documentation/warning change, and the tests are well-targeted. That said, there are a few implementation-quality and maintainability issues that I'd like to flag before landing. The current state is fine for pretrained=True users but creates maintenance debt that will compound. Most are small; a few are real bugs.
Note on the failing CI checks (
astral,headers,deps-sync,codecov): I checked and these are pre-existing onmain—astralfails onholocron/trainer/core.py:155(fastprogress typing, untouched here),headersflags 53 files whose copyright line still ends in-2025(unrelated), anddeps-synccomplains about apytest/Pillowmismatch betweenapi/pyproject.tomland./pyproject.toml. None of these are introduced by this PR. Not blocking from a review standpoint, but worth a separate fix-up issue.
Critical
1. Warning message is duplicated verbatim across two files — collapse to a single source of truth. holocron/models/utils.py:107-114 and holocron/models/checkpoints.py:106-112 carry the same multi-line msg = (...). block. Same wording, same URLs, same logger.warning + warnings.warn pair. If the model-zoo URL ever changes, you'll fix it in one and forget the other. Suggested path:
- Move the message into a single helper (e.g.
holocron/models/utils.py::warn_pretrained_unavailable(model_cls=None)) and call it from both sites. That also fixes the asymmetry whereload_pretrained_paramshasf"... for {model.__class__.__name__}"but_handle_legacy_pretraineddoesn't (the legacy path doesn't have the class handy — so the helper should take an optionalmodel_clsand degrade gracefully).
2. stacklevel=2 is wrong for both warnings — should be stacklevel=3 in load_pretrained_params. When a user runs darknet24(pretrained=True), the call chain is darknet24 → _handle_legacy_pretrained/_configure_model → load_pretrained_params → warnings.warn. With stacklevel=2, the warning is attributed to load_pretrained_params itself, not the user's darknet24(pretrained=True) call site. That's exactly the source of confusion the warning is supposed to resolve. The test passes because it calls load_pretrained_params directly (only one frame above warn), so the bug is invisible to the test. The same issue applies in _handle_legacy_pretrained (called from model factories) and to the Quick Tour in the README.
3. The new warning path in checkpoints.py is untested. Codecov is right: 25% patch coverage on checkpoints.py because the new test only exercises load_pretrained_params. Add a parallel regression test in tests/test_models.py for _handle_legacy_pretrained — e.g. patch a checkpoint URL to None in ReXNet1_0x_Checkpoint.DEFAULT (or use a model whose default_cfg["url"] is None) and assert a UserWarning is raised with the offending class name. As written, the checkpoints.py change has no regression guard.
4. Each pretrained=True call now fires both logger.warning and warnings.warn with the same message. Two channels, one signal — Jupyter users will see it in the cell output, CLI users will see it in stderr and their log handler. Pick one. The warnings system is the right one here (visible in notebooks, escalatable with -W error), so I'd drop the logger.warning and let application code configure warning routing if it wants a log record.
Important
5. Use a dedicated warning subclass. A bare UserWarning is too generic — every project emits them, and users can't filter this specific class without string-matching the message. Something like:
class PretrainedWeightsUnavailableWarning(UserWarning):
"""Raised when pretrained=True is requested but no checkpoint is available."""…in holocron/models/checkpoints.py, and emit that from the shared helper above. Users can then do warnings.filterwarnings("ignore", category=PretrainedWeightsUnavailableWarning) cleanly. The test can also assert on the class instead of a regex match (more robust to message edits).
6. The model zoo is hand-typed markdown and will go stale on the first new checkpoint. The PR description says "generated from the Checkpoint metadata" but the table is plain markdown in README.md (29 rows × 5 columns = 145 data points) and again in docs/docs/index.md (so 290 data points to keep in sync). Two ways out, in order of preference:
a. (best) Add a small references/generate_model_zoo.py (or a make target) that imports the registries, walks every Checkpoint enum, and writes the table to a <!-- AUTOGEN START --> … <!-- AUTOGEN END --> block in both files. CI runs the generator and fails on diff. ~50 lines.
b. (pragmatic) Use mkdocstrings snippets / markdown-include to pull the table from a single source file (e.g. docs/includes/model_zoo.md) and reference it from both README and index. Fewer LOC change but doesn't actually solve the "hand-typed" problem.
Either way, the hardcoded counts ("29 classification + 1 segmentation") at the bottom of the README and in the PR body are guaranteed to drift.
7. The PolyLoss example is now duplicated in four places. README (## Loss functions block), docs/docs/index.md (none added but README is mirrored), holocron/nn/modules/loss.py docstring, and tests/test_models.py style. If the constructor signature changes, four sites to update. The docstring is the natural single source — have the README import the same snippet via a mkdocstrings include or just paste the literal lines once and point at the class. At minimum, drop one.
Minor / Nitpicks
poly_lossfunctional docstring: the newtarget:description ("hard class indices … OR soft class probabilities …") duplicates shape info across the class/function docstrings. Consider:target: class indices of shape $(N, \\dots)$ with dtype ``torch.int64`` (use soft probabilities only via the class wrapper, which adds the log-softmax gate). Or just keep the class-level shape and have the function doc say "seePolyLossfor shape/dtype details".- The note block in the PolyLoss class docstring says "A
TypeErroris raised if the hardtargetis not of dtypetorch.int64." — make it "is raised byF.poly_loss" to keep the wrapper docstring free of implementation-detail references. - PR body says warnings can be escalated with
-W error— that's not a validpython -Wvalue. The correct form ispython -W error::UserWarning:holocron.models.utils your_script.pyorPYTHONWARNINGS=error::UserWarning:holocron.models.utils. Tiny but people copy-paste from PR bodies. stacklevelof2is the minimum useful value — and is what most call sites need, so it's defensible, but the helper from (1) should document this in its docstring so a future maintainer doesn't "fix" it to 1 or 2 again.- No way to test the "noisy in a loop" case. If a user calls
darknet24(pretrained=True)in a tight training loop (e.g. unit tests, sweeps), the warning will dedupe naturally via__warningregistry__, but the test would be valuable to lock that behaviour in. Optional. tests/test_models.pyis already an oddly named file — it now contains a test forutils.load_pretrained_params, a test forutils.fuse_conv_bn, and a test formodel_from_hf_hub. The new test fits the file's actual scope (models utilities), but if the file is renamed in the future, the new test will move with it — worth a one-line comment near the test pointing at_handle_legacy_pretrainedso the corresponding test for the legacy path lives next to it.
Suggested follow-up issues (not blocking)
- Migrate the 53 pre-existing files off the
-2025copyright header (or relaxvalidate_headers.pyto allow a 1-year grace period). Unrelated to this PR but it makes theheaderscheck permanently red. - Fix the pre-existing
astralfailures inholocron/trainer/core.py(fastprogress typing). - Sync
pytestandPillowversions acrossapi/pyproject.tomland./pyproject.toml.
Happy to push a fix for items 1–5 myself if you'd like — they're small and mechanical. The rest are follow-ups.
Review follow-upI did another pass through PR #457. Direction is good: clearer pretrained-weight docs, a working quick-tour model, and a visible missing-weights warning are all useful. I would tighten these before merge.
Verification:
|
Addressed the review feedback (3 commits)Thanks for the thorough reviews — I catalogued every distinct point across all five, deduped, and prioritized by ROI. 🐛 The highest-ROI item turned out to be a real bug — and it's #211While wiring up the loss docs I found that Done
Correction to my PR description
Deferred — agree these are non-blocking follow-ups
Green locally: 🤖 Generated with Claude Code |
…ability Driven by recurring confusion in community issues/discussions (#107, #123, #211, #230, #253, #255): - README + docs index: replace the inconsistent architecture list with an accurate model zoo table (29 classification + 1 segmentation checkpoints), generated from the Checkpoint metadata, with training dataset (Imagenette vs ImageNet-1k), top-1 accuracy and params. - State clearly that weights are Holocron-trained and NOT interchangeable with torchvision/timm, and that object detection ships no pretrained weights yet. - Fix the flagship Quick Tour: it called repvgg_a0 (whose legacy default_cfg is empty -> KeyError) and never imported torch. Now uses darknet24 and imports torch. - Add a 'Loss functions' usage snippet and clarify PolyLoss/poly_loss docstrings: input is raw logits, hard targets must be torch.int64 class indices, ignore_index masks samples. - Issue templates: disable blank issues so the bug template (repro snippet + env) is enforced. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… are missing Previously, calling a builder with pretrained=True when no checkpoint URL is available logged a vague 'Invalid model URL' through the logging module (usually silent in notebooks/scripts) and returned a randomly-initialized model, so users could not tell their 'pretrained' model was actually random (cf. #123, #253). load_pretrained_params() and _handle_legacy_pretrained() now also emit an explicit UserWarning with an actionable message pointing to the model zoo and reference scripts (load_pretrained_params additionally names the model class). Behavior is otherwise unchanged - it still falls back to random initialization - to stay backward-compatible. Adds a regression test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
poly_loss crashed when the hard target contained ignore_index: gather() was called with the raw target (out of bounds for the default -100), and the mask guard 'ignore_index >= 0' skipped negative values entirely. This is the failure reported in #211, which the new docs were inadvertently recommending. The hard-label path now computes the ignore mask first, gathers with a sanitized target, and excludes ignored positions for any ignore_index value -- zeroing their loss and gradient, including under reduction='none'. The PolyLoss docstring now documents the contract (logits in, int64 class indices), shows a runnable ignore_index example, and lists the real parameters. Adds regression tests for the default ignore_index=-100 and the int64 dtype guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… paths The missing-weights notice was duplicated across checkpoints.py and utils.py (with slightly different wording) and emitted on two channels (logging + warnings). It now lives in a single _warn_pretrained_unavailable() helper that raises a dedicated, filterable PretrainedWeightsUnavailableWarning via warnings.warn only, with stacklevel=3 so the warning points at the model builder (e.g. repvgg_a0) instead of a Holocron internal. Adds a regression test for the _handle_legacy_pretrained (model-factory) path and a no-false-positive guard for weight-bearing models, and relaxes the test lint rules (PLC2701) to allow importing the internal helper. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The model-zoo table was hand-maintained in both README.md and docs/index.md, guaranteeing drift on the next checkpoint. It is now generated by .github/generate_model_zoo.py, which derives the rows statically (stdlib only, no torch) from the Checkpoint metadata and the legacy default_cfgs, and writes them between AUTOGEN markers. 'make model-zoo' regenerates the tables; 'make model-zoo-check' (wired into pre-commit and a quality CI job, mirroring deps-check) fails on drift. Also corrects the accuracy wording (validation split, not training set) and demonstrates ignore_index in the README loss snippet. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…inks
Per review feedback:
- The Quick Tour (README + docs) uses repvgg_a0 again instead of darknet24 -- RepVGG is the more representative flagship. Since it uses the Checkpoint API, model.default_cfg is the Checkpoint object, so the snippet now reads model.default_cfg.pre_processing.{input_shape,mean,std} and .meta.categories (the original repvgg example was actually broken: it indexed default_cfg like a dict, which only works for the legacy models).
- Restored the per-task architecture lists with their paper links in the docs model zoo, alongside the pretrained-weights table. The links matter for SEO and for showing which architectures are implemented in a research repo. (The README's Paper references section was untouched.)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t docstrings Addresses two nits from the PR review: - .github/generate_model_zoo.py: _checkpoint_entry now resolves the `DEFAULT = <member>` alias (and reads `arch` by keyword rather than position) instead of taking the first-declared checkpoint. The generated table is byte-identical today (every enum's DEFAULT happens to be its first member), but it no longer risks silently emitting the wrong row if a future enum defaults to a non-first member. - rexnet.py: rexnet1_0x ships ImageNet-1k weights (not ImageNette) and rexnet2_2x ships Imagenette (not ImageNet); corrected both pretrained-arg docstrings to match their DEFAULT checkpoints (and the model-zoo table). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
3f6394a to
6572b3f
Compare
Why
I went through every issue and discussion opened by non-maintainers and clustered the recurring friction. Almost all of it traces back to documentation blind spots around pretrained weights and loss usage:
dtype,ignore_index)poly_lossdocstring said "predicted probability"; no usage example anywhereWhile verifying, I found the flagship Quick Tour example was broken: it called
repvgg_a0(pretrained=True)then readmodel.default_cfg['input_shape']— butdefault_cfgis empty for that model (KeyError) — and the snippet never importedtorch. I also found that requesting weights for a model that has none silently returned a randomly-initialized model (only aloggingcall, usually invisible in notebooks).What changed
Docs (P0)
README.md(collapsible) anddocs/docs/index.md— generated from theCheckpointmetadata: 29 classification + 1 segmentation pretrained checkpoints with training dataset, top-1 accuracy, and params.timm, and that object detection ships no weights yet.blank_issues_enabled: falseso the bug template (repro +collect_env) is enforced.Loss docs (P1)
PolyLossclass docstring now documents shapes, thetorch.int64requirement,ignore_index, and a runnable example; fixed thepoly_lossarg descriptions ("predicted probability" → raw logits / int64 class indices).Behavior (P1)
load_pretrained_params()and_handle_legacy_pretrained()now raise an explicitUserWarning(in addition to the log) when no checkpoint is available, with an actionable message + the offending model class.Quick Tour fix
darknet24(actually ships weights and has a populateddefault_cfg) and added the missingimport torch.Implementation choices
pretrained=Truecall. I kept the fallback but made it loud via the standardwarningssystem (so it shows in notebooks and can be escalated withpython -W error::holocron.models.checkpoints.PretrainedWeightsUnavailableWarning). Easy to switch to a hard error later if you prefer.CheckpointAPI (evaluation.results), so I included top-1 accuracy at zero extra cost. Imagenette (10-cls) vs ImageNet-1k (1000-cls) is called out so the numbers aren't misread. Easy to drop if unwanted.Checkpointenums and the legacydefault_cfgs['url'], so the table reflects exactly whatpretrained=Trueloads (e.g.darknet24/tridentnet50are legacy and have no stored accuracy → shown as "—").ruff/tyclean.How to test
Manual checks:
Notes / possible follow-ups (not in this PR)
🤖 Generated with Claude Code