Correct metadata and docs our own merges made stale - #112
Merged
Conversation
- Model cards defaulted to license 'mit', but franklab/HSSM declares bsd-2-clause. The code is MIT; a card describes the *artifact*, so it follows the artifact repo. One DEFAULT_LICENSE next to DEFAULT_REPO_ID. Verified against every published artifact before the first real publish. - The 'install it with' hints named only pip. Reworded to give both, since the message reaches end users (who pip-installed) and developers (who did not). The repo's own uv rule carves out end-user install docs. - CLI --network-type help still said 'lan, cpn, or opn' after gonogo was added; it now derives from VALID_NETWORK_TYPES so it cannot drift again. - CLAUDE.md claimed jaxtrain has no ONNX export (false since the jax2onnx exporter), that model_card.yaml is required (false since it is generated), and that all exporters trace rank-1 (never true of the MLP exporter, and not true of the production artifacts). Adds onnx/contract.py: assert_single_trial_contract, the executable form of the rank paragraph. The invariant is concrete input dims; rank follows from the tracer's lowering. Checked against all 18 production artifacts on franklab/HSSM — every one passes, all (1, D) Gemm+Tanh — so this encodes what production already is rather than imposing something new. 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: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughChangesThe pull request adds a single-trial ONNX contract validator with runtime and operator checks. It standardizes Hugging Face artifact licensing, improves installation guidance, derives CLI network types dynamically, and updates architecture documentation. ONNX contract and export documentation
Hugging Face publishing defaults and CLI guidance
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Test
participant assert_single_trial_contract
participant ONNXModel
participant ONNXRuntime
Test->>assert_single_trial_contract: provide ONNX path and contract options
assert_single_trial_contract->>ONNXModel: load and inspect graph
assert_single_trial_contract->>ONNXRuntime: create inference session
ONNXRuntime-->>assert_single_trial_contract: input shape and runtime status
assert_single_trial_contract-->>Test: metadata or AssertionError
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lanfactory/hf/model_card.py`:
- Around line 15-16: Update ModelCardConfig.license and its docstring to use
DEFAULT_LICENSE instead of the hardcoded "mit" value, ensuring direct
ModelCardConfig() callers match YAML fallback and generated-card metadata.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ff07f9c-7e58-4741-ad3d-3efb2aed6125
📒 Files selected for processing (11)
CLAUDE.mdsrc/lanfactory/cli/download_hf.pysrc/lanfactory/cli/upload_hf.pysrc/lanfactory/hf/__init__.pysrc/lanfactory/hf/download.pysrc/lanfactory/hf/model_card.pysrc/lanfactory/hf/upload.pysrc/lanfactory/onnx/__init__.pysrc/lanfactory/onnx/contract.pytests/hf/test_dual_layout.pytests/onnx/test_contract.py
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates HuggingFace/model-card metadata and project documentation that became stale after recent merges, and introduces an executable check for the “single-trial ONNX contract” so exporters can validate artifacts via tests rather than prose.
Changes:
- Add
onnx/contract.py::assert_single_trial_contractplus a focused test suite to enforce “all input dims concrete” and optionally pin allowed op sets. - Fix HuggingFace model-card licensing defaults by introducing
DEFAULT_LICENSE="bsd-2-clause"(matchingfranklab/HSSM) and using it when generating/loading model cards. - Refresh CLI help strings and CLAUDE.md documentation to reflect current supported network types and the ONNX export reality (including JAX → ONNX).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/onnx/test_contract.py | Adds tests covering the executable ONNX “single-trial” contract. |
| tests/hf/test_dual_layout.py | Adds a regression test ensuring generated model cards use the artifact repo license. |
| src/lanfactory/onnx/contract.py | Implements assert_single_trial_contract using ONNX + ORT validation. |
| src/lanfactory/onnx/init.py | Exposes the new contract helper from the lanfactory.onnx package. |
| src/lanfactory/hf/upload.py | Uses DEFAULT_LICENSE for generated model cards; improves hf extra install guidance. |
| src/lanfactory/hf/model_card.py | Uses DEFAULT_LICENSE as the fallback when loading model_card.yaml. |
| src/lanfactory/hf/download.py | Improves hf extra install guidance. |
| src/lanfactory/hf/init.py | Adds DEFAULT_LICENSE constant alongside DEFAULT_REPO_ID. |
| src/lanfactory/cli/upload_hf.py | Makes --network-type help derived from VALID_NETWORK_TYPES; improves hf extra install guidance. |
| src/lanfactory/cli/download_hf.py | Makes --network-type help derived from VALID_NETWORK_TYPES; improves hf extra install guidance. |
| CLAUDE.md | Updates docs to reflect current ONNX export pipeline and contract details. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+56
to
+62
| for graph_input in model.graph.input: | ||
| for dim in graph_input.type.tensor_type.shape.dim: | ||
| assert dim.HasField("dim_value"), ( | ||
| f"symbolic dim {dim.dim_param!r} in input {graph_input.name!r}: " | ||
| "HSSM's make_jax_func rejects dynamic axes at load, and a graph " | ||
| "that slips through returns wrong numbers rather than failing" | ||
| ) |
Member
Author
There was a problem hiding this comment.
Fixed — added assert dim.dim_value > 0. You are right that HasField is true for an explicitly-set zero, and a zero axis is degenerate whether or not the producer meant it as 'unknown'. Test added.
Comment on lines
6
to
10
| __all__ = [ | ||
| "transform_to_onnx", | ||
| "transform_jax_to_onnx", | ||
| "transform_sbi_to_onnx", | ||
| "transform_bayesflow_to_onnx", |
Member
Author
There was a problem hiding this comment.
Fixed — added to all (and moved the import above the block so it reads in declaration order).
Comment on lines
+12
to
+15
| width = input_dims[-1] if isinstance(input_dims[-1], int) else 6 | ||
| x = helper.make_tensor_value_info("x", TensorProto.FLOAT, list(input_dims)) | ||
| y = helper.make_tensor_value_info("y", TensorProto.FLOAT, [1, 1]) | ||
| w = helper.make_tensor( |
Member
Author
There was a problem hiding this comment.
Fixed — the helper now declares [1] for rank-1 inputs and [1, 1] for rank-2. Relying on permissive checker behaviour in the tests for the checker was the wrong precedent.
Comment on lines
+9
to
13
| # MIT, but a model card describes the published artifact, so it follows the | ||
| # artifact repo — auto-generated cards claiming MIT would contradict it. | ||
| DEFAULT_LICENSE = "bsd-2-clause" | ||
| # gonogo included: the trainers already build gonogo networks (cli/utils.py | ||
| # train_output_type_dict) and HSSM resolves "{model}_gonogo.onnx", so excluding |
Member
Author
There was a problem hiding this comment.
Fixed — DEFAULT_LICENSE added to all.
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
CodeRabbit was right that my license fix was incomplete. I changed the YAML fallback and the generated card, but ModelCardConfig's own field still defaulted to 'mit', so any direct caller of ModelCardConfig() would still have minted a card contradicting the artifact repo. It now defaults to DEFAULT_LICENSE, and two pre-existing tests that pinned 'mit' are updated to the corrected contract (one keeps an explicit 'mit' to prove an explicit licence still passes through untouched). Copilot findings, all valid: - assert_single_trial_contract accepted a dim_value of 0. HasField is true for an explicitly-set zero, which some producers use for 'unknown' and which is a degenerate axis regardless. Now rejected, with a test. - DEFAULT_LICENSE and assert_single_trial_contract were importable but missing from __all__. - The contract test helper declared a [1, 1] output for the rank-1 input case, where MatMul actually yields rank-1. Leaning on a permissive checker in the tests for the checker is not a good look. - The license test hard-coded the string twice; it now pins the literal once and checks the wiring against the constant. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/lanfactory/onnx/init.py:6
- This intra-package import uses an absolute module path and a blanket
# noqa: E402even though imports are already at the top of the file. Using a relative import keeps this consistent with the other exports here and avoids suppressing lint unnecessarily.
from .transform_onnx import transform_to_onnx
from lanfactory.onnx.contract import assert_single_trial_contract # noqa: E402
src/lanfactory/cli/upload_hf.py:36
- The CLI help for --model-folder still claims the folder "should contain model_card.yaml", but upload_model can now generate a default card unless --require-model-card is set. This help text is misleading for end users and contradicts the new behavior.
"--network-type",
help=f"Network type: one of {', '.join(VALID_NETWORK_TYPES)}.",
),
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.
Follow-ups from #108/#109/#110, plus the executable form of the ONNX contract.
The one with a deadline
Generated model cards defaulted to
license: mit, butfranklab/HSSMdeclaresbsd-2-clause. The ecosystem's code is MIT; a model card describes the published artifact, so it follows the artifact repo. Left alone, the first automated publish would have stamped a contradicting license onto every card — annoying to retro-fix across 40 of them. Now oneDEFAULT_LICENSEbesideDEFAULT_REPO_ID.Stale after our own merges
--network-typehelp still read "lan, cpn, or opn" after gonogo was added in Publish networks where HSSM actually looks for them (dual HF layout + manifest) #110. Now derived fromVALID_NETWORK_TYPES, so it cannot drift again.CLAUDE.mdclaimed jaxtrain has no ONNX export (false since jax → ONNX export via jax2onnx: jaxtrain now produces HSSM-consumable artifacts #109), thatmodel_card.yamlis required (false since Publish networks where HSSM actually looks for them (dual HF layout + manifest) #110), and that all exporters trace rank-1 (never true of the MLP exporter).uv sync, then reverted: the repo's own.claude/rules/uv.mdsays end-user install docs may reference pip, and these messages reach end users who did pip-install — telling them to runuv syncis worse advice. They now give both.The contract, as a check instead of a paragraph
onnx/contract.py::assert_single_trial_contractreplaces prose that four exporters each had to remember, and that was wrong where it was written down.The invariant is exactly one thing: every input dim concrete. Rank is not part of it — it follows from how a tracer lowers a dense layer.
torch.onnx.exporton a rank-1 dummy gives rank-agnosticMatMul+Add(sbi, bayesflow); a(1, D)dummy, andjax2onnxalways, givesGemm, whose spec requires rank 2. Both load in HSSM and, measured undervmap+jit, run identically.Verified against production — nothing changes for existing artifacts
Ran the new check against all 18 root artifacts on
franklab/HSSM:All 18 are
(1, D)Gemm+Tanh. So this encodes what production already is; it does not impose a new rule that existing files would fail. It also settles the rank question empirically: production has always been(1, D), so the doc was wrong about the live artifacts too.Found while doing this — not fixed here
hssm.HSSM(model="ddm_sdv", loglik_kind="approx_differentiable")is broken for every user today. HSSM'smodelconfig/ddm_sdv_config.py:40asks forddm_sdv.onnx, which does not exist infranklab/HSSM(404). Reproduced end to end. That is an HSSM/artifact issue, not this PR's, and it makes an obvious first backfill target for the pipeline.Commands run
🤖 Generated with Claude Code
Summary by CodeRabbit
transform-jax-onnxcommand for native JAX/Flax-to-ONNX conversion.gonogoas a supported network type.