fix(hf): reject a non-mapping architecture/training in model_card.yaml - #134
Conversation
`architecture` and `training` are typed `dict | None` and consumed with `.get()` during README generation, but nothing validated the YAML. A list -- `architecture: [100, 100, 1]`, the plausible mistake -- survived load, and because it was not `None` it silently suppressed the pickle fallback that would have supplied the right shape. The failure then surfaced as `AttributeError: 'list' object has no attribute 'get'` deep inside `write_readme`, after the artifact upload had already begun. Validate both fields at load time instead, and say in the message that omitting them is the correct fix: the pickled configs are the authoritative record of what was trained. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
ChangesModel Card Validation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Model-card YAML now fails early with a clear error when architecture or training is not a mapping, preventing later README-generation failures and partial uploads. The change is covered for both fields and is ready to merge. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, prevents a confirmed runtime failure mode, and includes focused test coverage for the new validation behavior.
Pull request overview
This PR hardens HuggingFace model card loading so invalid YAML shapes for architecture/training fail early (at load time) with a targeted error, preventing late AttributeError failures during README generation after an upload has already begun.
Changes:
- Add
_require_mapping(...)helper to validate thatarchitectureandtrainingare either mappings or omitted/null, otherwise raise a descriptiveValueError. - Use
_require_mapping(...)insideload_model_card_yamlfor the two dict-typed fields. - Add a parametrized pytest covering both keys to ensure non-mapping values are rejected at load time.
File summaries
| File | Description |
|---|---|
src/lanfactory/hf/model_card.py |
Validates architecture/training types during YAML load to prevent late .get() crashes and preserve pickle fallback behavior. |
tests/hf/test_model_card.py |
Adds a parametrized test asserting a ValueError is raised for non-mapping architecture/training values. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
What
ModelCardConfig.architectureand.trainingare typeddict | Noneand read with.get()ingenerate_readme, butload_model_card_yamlaccepted whatever the YAML held.A list —
architecture: [100, 100, 1], which reads naturally enough to write by hand — did two bad things at once:None, so_fill_from_pickle_configsskipped the fallback that would have supplied the correct dict from*_network_config.pickle.config.architecture.get("network_type")insidewrite_readme— asAttributeError: 'list' object has no attribute 'get', after the artifact upload had already started, leaving a half-populated staging dir and a partial remote commit.Change
One helper, called on the two dict-typed fields at load time:
The message points at the right fix rather than just the wrong type — omitting the field is better than restating it, since a hand-written copy can drift from the pickle.
Scope
Deliberately narrow: only the two fields that crash.
tags/usage_exampleare formatted, not traversed, so a wrong type there degrades a string instead of raising.Testing
tests/hf/test_model_card.py— one parametrized case over both fields. Verified it fails onmain(AttributeErrorpath) and passes with the change. Full file: 23 passed; ruff check + format clean.Found while publishing a
gamma_drift_angleLAN tofranklab/HSSM_staging; the publisher's dry run does not reach README generation, so nothing caught it before the upload.🤖 Generated with Claude Code
Summary by CodeRabbit
architectureortrainingYAML values instead of failing later during README generation.