fix: unblock v0.74.0 crates.io publish, and surface the hf-hub patch/publish gaps - #1093
fix: unblock v0.74.0 crates.io publish, and surface the hf-hub patch/publish gaps#1093michaelneale wants to merge 1 commit into
Conversation
The v0.74.0 crates.io publish failed verifying model-package:
error[E0308]: mismatched types
.repo_id(candidate.target_repo.clone())
note: method defined here (hf-hub-1.0.0) repo_id: &str
The workspace patches hf-hub to a fork via [patch.crates-io], so workspace
builds and CI compile this bin against the fork. cargo publish verifies the
packaged tarball, where the patch does not apply and hf-hub resolves from
crates.io (1.0.0), whose create_repository().repo_id() takes &str. Passing an
owned String only fails on that registry path.
Pass a borrow so both the patched-fork workspace build and the registry
publish-verify build compile.
Assisted-by: Claude Sonnet
📝 WalkthroughWalkthrough
ChangesRepository builder update
Estimated code review effort: 1 (Trivial) | ~2 minutes Suggested reviewers: 🚥 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 |
|
@i386 tagging you because the fork here is yours (#709 "Use combined hf-hub fork for model downloads") and two of the three issues below are really design calls rather than review nits. The code change is one character. The reasons it happened are the point:
To answer the obvious question: no, I don't think we can just drop the patch — it's carrying real functionality. But the current shape seems wrong. Options in the PR body; the one I'd lean toward is publishing the fork under its own name (e.g. Also FYI the fork branch is 5 behind upstream main. Separately: this fix alone won't finish v0.74.0's crates chain, because |
|
Correcting two things from my comment above, after actually checking rather than assuming. 1. The sibling version pins are NOT broken — I was wrong. I'd flagged
2. On "does upstream hf-hub have it now?" — my first check was too shallow, but the conclusion holds, for a sharper reason. Upstream range: Option<std::ops::Range<u64>>,
//! Range parameters use Rust std::ops::Range<u64> semantics
But that's a caller-supplied range read on Resume(u64),
ExistingBlob,
pub(crate) fn incomplete_path(path: &Path) -> PathBuf { ... "{}.incomplete" ... }
std::cmp::Ordering::Less if len > 0 => PartialDownloadState::Resume(len),So: So it's down to two real issues, both design calls for you: the preflight gap (Issue 1) and published crates not getting the fork (Issue 2). |
|
Single change captured in #1096 along with Cargo updates and a corrected hf-hub pin - closing in favour of that |
What this fixes
v0.74.0published its GitHub release and all binaries/GPU bundles fine, but the crates.io publish died 28 of 42 crates in, failing to verifymodel-package:That left the workspace half-published: 27 crates at
0.74.0, 15 not.The code change here is one character (
&). The reasons it happened are the actual content of this PR.Root cause:
[patch.crates-io]does not apply to publish-verifyThe workspace patches
hf-hubto a fork:cargo check/clippy/just build/ all normal CI compile against the fork, where.repo_id(String)is accepted → green.cargo publishverifies the packaged tarball outside the workspace, where the patch does not apply.hf-hubresolves from the registry (1.0.0), whose builder takes&str→ E0308.The bug was structurally invisible to every normal build. A borrow compiles under both.
Issue 1 — the preflight gap that let this reach a GA publish
publish_crates_preflightrunsscripts/publish-crates.sh --dry-run, which should have caught this. It didn't, because the script intentionally skips verification for any crate whose in-workspace registry deps aren't on crates.io yet:model-packagelistsmodel-hfandmodel-refas unpublished registry deps, so at preflight time (nothing published yet for the new version) its verification was skipped entirely. The failure could therefore only surface during the real publish — after 27 crates had already gone out irreversibly.So for the whole class of crates with unpublished sibling deps, we currently have no pre-publish type-checking against real registry deps. That's the gap worth fixing, independent of this typo.
Issue 2 — published crates on crates.io do not get the fork
[patch.crates-io]is not transitive. It applies only to builds of this workspace. The published manifest confirms it —model-hf@0.74.0on crates.io declares:i.e. upstream registry hf-hub, not the Mesh-LLM fork. Consequences:
cargo add model-hf,mesh-llm-host-runtime,mesh-llm-commands,model-package) builds against upstream hf-hub, silently losing what the fork carries.Is the patch still needed? Yes — verified, and it is not the same feature upstream has
Worth being precise, because upstream does now have byte-range support and it is easy to mistake for resume. Upstream
main(hf-hub/src/repository/download.rs) has:That is a caller-supplied byte-range read on
download_file_stream/download_file_to_bytes. It is not resume-after-interruption. Upstreamhf-hub/src/cache/storage.rshas no.incomplete/ partial-blob handling at all.The fork's resume commit adds exactly that missing piece:
i.e.
.incompletestaging files, partial-length detection, and resume state — restarting an interrupted model download where it left off. Different feature from upstream's range API.The fork carries 5 commits ahead, all load-bearing for model downloads:
9d8ba581e8fb7ac427573dc6Content-Lengthas file size9008c312fd3bfcabTouching
cache/storage.rs,repository/download.rs,repository/files.rs,xet.rs(~900 added lines). The fork is also 5 commits behind upstreammain, and upstream's latest tag isv1.0.0-rc.2while we depend on^1.0.0-rc.1(registry has1.0.0final).So we can't drop the patch — it's carrying real functionality upstream lacks. But the current shape means our crates.io artifacts advertise a dependency we never build or test against. Options to decide between:
huggingface/hf-huband drop the fork+patch entirely (cleanest, slowest).mesh-llm-hf-hub) and depend on it directly — no[patch]needed, published crates become honest, CI and publish compile the same code.Not an issue: sibling version pins (correcting an earlier claim)
An earlier revision of this description claimed
crates/model-package/Cargo.tomlshipped stale0.72.1sibling pins. That was wrong — I checked the actual released tag:scripts/release-version.shhasupdate_versioned_path_dependency_versions()(invoked at lines 244 and 251) which rewrites every{ path = ..., version = ... }pin to the release version. The 940.72.1pins visible onmainare just un-bumped dev state and are normalized at release time. No action needed.Validation
cargo check -p model-package --bins— passcargo clippy -p model-package --all-targets -- -D warnings— passcargo fmt --all --check— passNote this fix alone is not sufficient to finish
v0.74.0's crates chain: thepublish_cratesjob checks outref: <tag>(v0.74.0), so the fix must be reachable from the tag it builds. Re-running the old job as-is replays the same failure. Finishing options: resume the chain manually from a checkout containing this fix, or fold it into av0.74.1.Rollback
Single revert; the code change is one borrow.