Skip to content

feat(hf): publish recovery_report.json alongside the network - #135

Closed
AlexanderFengler wants to merge 1 commit into
mainfrom
feat/publish-recovery-report
Closed

feat(hf): publish recovery_report.json alongside the network#135
AlexanderFengler wants to merge 1 commit into
mainfrom
feat/publish-recovery-report

Conversation

@AlexanderFengler

Copy link
Copy Markdown
Member

recovery_report.json was missing from DEFAULT_INCLUDE_PATTERNS, so it never reached the Hub.

That matters because the two reports answer different questions. validation_report.json says the artifact is structurally sound, loads in HSSM, and produces a plausible density. It cannot see a recovery failure — the gate has no recovery check in it. So a network whose parameter-recovery verdict is passed: false ships looking entirely clean, and the only quality evidence a downstream reader gets is the one that could not have detected the problem.

Concretely: gamma_drift_angle is on franklab/HSSM_staging with validation_report.json reporting all gates passed, while its standalone recovery verdict is passed: false (2 coverage failures, 3 bias-rate failures). Nothing in the published bundle says so.

One line in the include patterns, plus fixture and assertion in test_dual_layout.py.

96 passed, 1 skipped; ruff clean.

🤖 Generated with Claude Code

`validation_report.json` says the artifact is structurally sound; it cannot
see a recovery failure, because the gate has no recovery check in it. With
recovery_report.json excluded from the upload, a network whose recovery
verdict is false ships looking clean, and the only evidence that travels
with it is the one that could not have caught the problem.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings September 3, 2026 03:57
@coderabbitai

coderabbitai Bot commented Sep 3, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 3 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: b32412ba-f0e7-414e-a948-1822ab760442

📥 Commits

Reviewing files that changed from the base of the PR and between 35fdad9 and 9ecaa94.

📒 Files selected for processing (2)
  • src/lanfactory/hf/upload.py
  • tests/hf/test_dual_layout.py

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The change is small, correctly scoped (include pattern + test), and directly covers the previously missing artifact in the upload selection logic.

Pull request overview

Ensures HuggingFace uploads include recovery_report.json alongside other model artifacts, so downstream users can see parameter-recovery failures that are not detectable from validation_report.json gate checks.

Changes:

  • Add recovery_report.json to DEFAULT_INCLUDE_PATTERNS used by upload_model().
  • Extend the dual-layout HF upload dry-run test fixture and assertions to cover recovery_report.json.
File summaries
File Description
src/lanfactory/hf/upload.py Adds recovery_report.json to the default artifact include patterns so it is published to the Hub.
tests/hf/test_dual_layout.py Updates the test fixture to generate recovery_report.json and asserts it is included in the dry-run upload listing.
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

codecov Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
src/lanfactory/hf/upload.py 94.65% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@AlexanderFengler

Copy link
Copy Markdown
Member Author

Wrong package — closing.

recovery_report.json is a LAN_pipeline_minimal concept. LANfactory has no notion of parameter recovery: after this change the only two references to either report anywhere in src/lanfactory/ would be the two filename strings in DEFAULT_INCLUDE_PATTERNS. Nothing here produces or reads them.

And it is unnecessary: upload_model already accepts include_patterns (upload.py:191). The pipeline simply never passes it, so it inherits the default. The fix belongs at that call site.

I pattern-matched onto validation_report.json already being in the default list and treated that as the convention. It is the same leak, already present — a downstream package's filename baked into an upstream uploader's defaults. Worth cleaning up separately rather than adding a second instance.

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.

2 participants