Switch model2vec-rs to upstream git dependency - #43
Conversation
There was a problem hiding this comment.
Pull request overview
Removes the vendored forks/model2vec-rs fork and switches the workspace to a pinned upstream model2vec-rs git dependency, replacing fork-specific model asset resolution with a local bundle resolver built on hf-hub.
Changes:
- Deleted the vendored
forks/model2vec-rscrate contents (code + metadata files). - Updated
indexbind-coreandindexbind-wasmto depend on upstreammodel2vec-rsvia gitrev=53c5a618…. - Added a new
hf-hub-based resolver inindexbind-coreto collect/copy model assets into canonical bundles.
Reviewed changes
Copilot reviewed 12 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| forks/model2vec-rs/src/model.rs | Removed vendored fork implementation. |
| forks/model2vec-rs/src/main.rs | Removed vendored fork CLI entrypoint. |
| forks/model2vec-rs/src/lib.rs | Removed vendored fork library module export. |
| forks/model2vec-rs/rustfmt.toml | Removed vendored fork formatting config. |
| forks/model2vec-rs/README.md | Removed vendored fork documentation. |
| forks/model2vec-rs/LICENSE | Removed vendored fork license file. |
| forks/model2vec-rs/Cargo.toml.orig | Removed vendored fork manifest source. |
| forks/model2vec-rs/Cargo.toml | Removed vendored fork normalized manifest. |
| forks/model2vec-rs/Cargo.lock | Removed vendored fork lockfile. |
| forks/model2vec-rs/.gitignore | Removed vendored fork ignore rules. |
| crates/indexbind-wasm/Cargo.toml | Switched model2vec-rs from path dependency to pinned upstream git rev (wasm features). |
| crates/indexbind-core/src/canonical.rs | Replaced fork-only asset resolver usage with local bundling resolver using hf-hub. |
| crates/indexbind-core/Cargo.toml | Switched model2vec-rs to pinned upstream git rev; added native-only hf-hub dep. |
| Cargo.toml | Added hf-hub to [workspace.dependencies] for native-only bundling support. |
| Cargo.lock | Updated lockfile to reflect git-sourced model2vec-rs and dependency graph changes. |
Files not reviewed (1)
- forks/model2vec-rs/Cargo.toml.orig: Language not supported
Comments suppressed due to low confidence (1)
crates/indexbind-core/src/canonical.rs:256
- The new bundle asset download/copy logic in
maybe_write_model_assetsisn’t covered by tests. Consider adding a unit test that creates a temporary local model directory (with dummytokenizer.json,config.json,model.safetensorsfiles) and asserts thatmaybe_write_model_assetscopies them intooutput_dir/modeland returns the expected relative paths.
pub(crate) fn maybe_write_model_assets(
output_dir: &Path,
embedding_backend: &EmbeddingBackend,
) -> Result<Option<CanonicalModelFiles>> {
let EmbeddingBackend::Model2Vec { model, .. } = embedding_backend else {
return Ok(None);
};
let files = resolve_model_files_for_bundle(model, None)
.map_err(|error| IndexbindError::Embedding(error.into()))?;
let model_dir = output_dir.join("model");
fs::create_dir_all(&model_dir)?;
fs::copy(&files.tokenizer, model_dir.join("tokenizer.json"))?;
fs::copy(&files.config, model_dir.join("config.json"))?;
fs::copy(&files.model, model_dir.join("model.safetensors"))?;
Ok(Some(CanonicalModelFiles {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -243,7 +246,7 @@ pub(crate) fn maybe_write_model_assets( | |||
| return Ok(None); | |||
| }; | |||
|
|
|||
| let files = resolve_model_files(model, None, None) | |||
| let files = resolve_model_files_for_bundle(model, None) | |||
There was a problem hiding this comment.
maybe_write_model_assets calls resolve_model_files_for_bundle(...) unconditionally, but resolve_model_files_for_bundle is only compiled for not(target_arch = "wasm32"). Building indexbind-core for wasm32 (directly or via indexbind-wasm) will fail due to the missing symbol. Fix by either cfg-gating maybe_write_model_assets/build_canonical_artifact for non-wasm targets, or providing a wasm32 implementation that returns Ok(None) (no model bundling).
| if base.exists() { | ||
| return resolve_local_model_files(base); | ||
| } |
There was a problem hiding this comment.
resolve_model_files_for_bundle uses base.exists() to decide whether the input is a local model directory. If model points to an existing file (not a directory), this path is treated as local and then fails with a confusing "missing tokenizer / model / config" error. Prefer base.is_dir() (and/or return a clearer error when it exists but isn’t a directory).
| if base.exists() { | |
| return resolve_local_model_files(base); | |
| } | |
| if base.is_dir() { | |
| return resolve_local_model_files(base); | |
| } | |
| if base.exists() { | |
| anyhow::bail!("local model path {base:?} exists but is not a directory"); | |
| } |
Summary
forks/model2vec-rscopymodel2vec-rsto upstream commit53c5a618f5d04e889359a3a0c04676e9c3138769hf-hubWhy
The upstream
model2vec-rsrepository has already merged the changes we needed for:from_bytes(...)hf-hubwasm/local-onlysupportcrates.iois still onmodel2vec-rs = 0.1.4, so we cannot switch back to a registry release yet. Pinning to upstream git removes the vendored fork now, while keeping the required functionality.Validation
cargo check --workspace --config net.git-fetch-with-cli=truecargo test --workspace --config net.git-fetch-with-cli=truenpm run check -- --config net.git-fetch-with-cli=true