feat: discover compatible Hugging Face embedding models - #61
Conversation
a8367b6 to
0f5d650
Compare
SummaryThe following content is AI-generated and provides a summary of the pull request: feat: Discover compatible Hugging Face embedding models via official
|
![hyperspace-pr-bot[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
The pull request is well-structured and the overall logic is sound, with good test coverage for the new check-model command, the layout-aware ONNX discovery, and the Hub client retry/timeout behaviour. The four comments above flag a null-dereference in discoverFileIntegrity when a file has no metadata and is not a known JSON file, a misleading error message in selectOnnxModel when multiple nested model.onnx exports are present, a potential TypeError in formatModelCheck if the model file entry is absent, and an eager optional-peer import at client-construction time rather than at first use.
PR Bot Information
Version: 1.29.54
- Correlation ID:
bca38cf0-a2c8-11f1-869c-5c31b4f61c80 - LLM:
anthropic--claude-4.6-sonnet - Event Trigger:
pull_request.ready_for_review - File Content Strategy: Full file content
| retries: options.requestRetries ?? HUB_REQUEST_RETRIES, | ||
| retryDelayMs: options.requestRetryMs ?? HUB_RETRY_DELAY_MS | ||
| }; | ||
| const loadedBindings = resolveBindings(bindings); |
There was a problem hiding this comment.
Bug: resolveBindings(bindings) is called eagerly at client-construction time. When bindings is absent or incomplete, this immediately starts a dynamic import('@huggingface/hub'), even if no Hub operation is ever performed (e.g. a caller that only uses checkModel with a pre-built hubClient). The resulting Promise is stored and re-awaited on each call, so correctness is preserved, but the eager import side-effects (including the friendly error for the missing peer) happen at construction rather than at first use. Consider calling resolveBindings lazily inside each method, or memoising it only on the first actual call.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| if (conventional.length === 1) return conventional[0]; | ||
| if (conventional.length > 1 || paths.length > 1) { | ||
| throw new Error( | ||
| `Hugging Face model '${repository}' contains ambiguous ONNX exports: ${paths.join(', ')}` | ||
| ); | ||
| } |
There was a problem hiding this comment.
Bug: When conventional.length > 1 the error message interpolates the full paths array (all .onnx files), but the ambiguity is among the conventional entries (multiple nested model.onnx files). Listing paths instead of conventional will include unrelated non-model.onnx files and misrepresent the conflict. Consider using conventional in the error message for that branch.
| if (conventional.length === 1) return conventional[0]; | |
| if (conventional.length > 1 || paths.length > 1) { | |
| throw new Error( | |
| `Hugging Face model '${repository}' contains ambiguous ONNX exports: ${paths.join(', ')}` | |
| ); | |
| } | |
| if (conventional.length === 1) return conventional[0]; | |
| if (conventional.length > 1) { | |
| throw new Error( | |
| `Hugging Face model '${repository}' contains ambiguous ONNX exports: ${conventional.join(', ')}` | |
| ); | |
| } | |
| if (paths.length > 1) { | |
| throw new Error( | |
| `Hugging Face model '${repository}' contains ambiguous ONNX exports: ${paths.join(', ')}` | |
| ); | |
| } |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| } | ||
|
|
||
| const file = knownFile ?? (await fetchFile(context, repository, revision, sibling.rfilename)); | ||
| const downloaded = knownFile ?? (await fetchFile(context, repository, revision, file.path)); |
There was a problem hiding this comment.
Bug: file is sourced from filesByPath.get(entry.path), which returns undefined for files not present in the Hub file listing (e.g. JSON files that were only fetched directly). When metadataChecksum and metadataSize are both absent and file is undefined, the fallback file.path on the next line throws a TypeError: Cannot read properties of undefined instead of a useful error. The knownFile parameter covers JSON files, so this only affects non-JSON files without LFS metadata, but the crash is still confusing. Should guard against undefined or ensure the caller always passes a valid file object.
| const downloaded = knownFile ?? (await fetchFile(context, repository, revision, file.path)); | |
| if (!file?.path && !knownFile) { | |
| throw new Error(`Cannot derive integrity for a file with no metadata or known content`); | |
| } | |
| const downloaded = knownFile ?? (await fetchFile(context, repository, revision, file.path)); |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| const modelFile = model.files.find(({ role }) => role === 'model'); | ||
| return `Likely compatible: ${model.repository} | ||
| Revision: ${model.revision} | ||
| Task: ${model.task ?? 'not declared'} | ||
| ONNX: ${modelFile.path} |
There was a problem hiding this comment.
Bug: modelFile will be undefined if model.files contains no entry with role === 'model', causing modelFile.path to throw a TypeError. While validateModelCandidate in checkModel validates the descriptor shape, the task field causes the shape to differ from what validateModelDescriptor expects. Should guard against a missing model file.
| const modelFile = model.files.find(({ role }) => role === 'model'); | |
| return `Likely compatible: ${model.repository} | |
| Revision: ${model.revision} | |
| Task: ${model.task ?? 'not declared'} | |
| ONNX: ${modelFile.path} | |
| const modelFile = model.files.find(({ role }) => role === 'model'); | |
| if (!modelFile) throw new Error(`check-model result for '${model.repository}' has no model file`); | |
| return `Likely compatible: ${model.repository} | |
| Revision: ${model.revision} | |
| Task: ${model.task ?? 'not declared'} | |
| ONNX: ${modelFile.path} |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
1 participant
Summary
@huggingface/hubclient@huggingface/hubas a lazy-loaded optional peer dependencynpx @cap-js/ai check-model <model>for a metadata-only, cache-free compatibility pre-checkReview hardening
@huggingface/hublibrary against a fake HTTP transport, including normalized.task, file/LFS metadata, and downloads.taskfield; missing task metadata is explicitly allowed, while declared non-embedding tasks are rejectedHF_TOKEN/Bearer handling and the obsoleteoriginalias; discovery currently supports public repositories onlyValidation
npm test— 102 tests passednpm run lintgit diff --checkThe local checkout does not provide the separate
format-cdsexecutable, but no CDS source files are changed by this PR.Larger architectural follow-ups are tracked together in #62. ONNX Runtime version compatibility remains tracked separately in #54.