feat: rank model-id suggestions by downloads and suggest on the not-found path#66
Conversation
…ound path Two follow-ups to the HuggingFace model-id suggestions from kubeflow#43: - Rank suggestions by downloads (list_models sort="downloads") so canonical repos surface ahead of community fine-tunes (e.g. qwen3:8b now suggests Qwen/Qwen3-8B rather than community forks). - Also suggest on the not-found path: a valid-format but nonexistent id (e.g. a typo like meta-lama/Llama-3) previously returned only the raw Hub error. It now reuses _suggest_hf_model_ids, and when a typo'd org makes the full "org/name" search empty, it retries on just the model name so the user still gets a "did you mean" list. Covered by unit tests (mocking list_models/model_info) and verified end to end against the live Hub. Fixes kubeflow#65 Signed-off-by: Ahmed <ahmed.dlshad.m@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
Improves Hugging Face model-ID suggestions for invalid or nonexistent models.
Changes:
- Ranks suggestions by downloads.
- Retries searches without the organization name.
- Surfaces suggestions when model lookup fails.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
kubeflow_mcp/trainer/api/planning.py |
Implements ranking, fallback searches, and error-path suggestions. |
tests/unit/trainer/test_planning.py |
Tests the new suggestion behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except Exception as e: | ||
| return {"error": str(e)} | ||
| # A valid-format but nonexistent id (e.g. a typo) lands here rather than | ||
| # the format branch above; offer the same best-effort suggestions so | ||
| # callers still get a "did you mean" list. | ||
| result: dict[str, Any] = {"error": str(e)} | ||
| suggestions = _suggest_hf_model_ids(model) |
There was a problem hiding this comment.
Narrowed the suggestion lookup to huggingface_hub RepositoryNotFoundError, so timeouts, rate-limits, and metadata errors now surface unchanged with no extra Hub request. Confirmed that gated repos (Llama/Gemma) return metadata from model_info without raising, so they never reach this branch. Added a test asserting a non-not-found error (a timeout) attaches no suggestions and makes no list_models call.
…rors Address review feedback. The not-found suggestion lookup ran in a broad except that caught every model_info failure, so a timeout, rate-limit, or metadata error would make an extra Hub request and could attach misleading "did you mean" results. Narrow it to huggingface_hub RepositoryNotFoundError; gated repos return metadata fine and never reach here, and all other errors are surfaced unchanged with no extra Hub call. Signed-off-by: Ahmed <ahmed.dlshad.m@gmail.com>
|
/ok-to-test |
There was a problem hiding this comment.
Nit: This file has some pre-existing comments still saying suggestions come from the format check. They also attach on RepositoryNotFoundError now .. maybe worth a one-line update. WDYT?
| if not normalized: | ||
| return [] | ||
| terms = [normalized] | ||
| name = normalized.rsplit("/", 1)[-1] |
There was a problem hiding this comment.
Nit only: when the org typo path falls back to just the model name (e.g. "Llama-3"), Hub search can be broad.
limit=3 + sort=downloads can keep it usable.. wdyt?
There was a problem hiding this comment.
Btw optionally we can skip the name-only retry when len(name) is very short.
Description
Follow-up to #43 (fix for #40), implementing the two enhancements from #65. Both improve the HuggingFace model-id suggestions that
pre_flight/estimate_resourcesreturn for a bad model id.Type of Change
Checklist
Related Issues
Fixes #65
Changes
_suggest_hf_model_idsnow passessort="downloads"tolist_models, so canonical repos surface ahead of community fine-tunes.meta-lama/Llama-3) previously hit theexceptbranch in_get_model_info_from_hfand returned only the raw Hub error. It now reuses_suggest_hf_model_ids. Since a typo in the org makes the fullorg/namesearch come back empty, the helper retries on just the model name so the user still gets a "did you mean".Verified end to end (live Hub)
Tests Added
Five new tests in
test_planning.py(mockinglist_models/model_info): download ranking, the org-typo name fallback, not-found suggestions at both the helper and theestimate_resourcesboundary, and graceful omission when the Hub errors.