feat(model-switch): add excluded_providers config + fix custom provider grouping for shared-endpoint proxies - #28218
Conversation
714ece4 to
a66bd2d
Compare
|
Supersedes #21544 for the custom provider grouping fix, and adds the new |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the contribution — both use cases are real, but this needs rework against current main before it is safe to salvage.
Problems
model_catalog.excluded_providersis loaded only throughhermes_cli/inventory.pyin this PR, while the gateway/modelpath still callslist_picker_providers/list_authenticated_providersdirectly atgateway/slash_commands.py:1019andgateway/slash_commands.py:1168, so those picker surfaces would ignore the new config.- The grouping fix is based on the older
(api_url, api_key)shape. Current main groups bycredential_identityandapi_modeathermes_cli/model_switch.py:1850andhermes_cli/model_switch.py:1868; dropping those dimensions would regress same-URL entries with different env-backed credentials or transports, covered bytests/hermes_cli/test_model_switch_custom_providers.py:470. - Current main intentionally uses live
/modelsby default for API-key custom providers and usesdiscover_models: falseas the scoped-subset opt-out (hermes_cli/model_switch.py:1985, tests attests/hermes_cli/test_model_switch_custom_providers.py:614and:675).
Suggested changes
- Extend the current grouping key rather than replacing it: endpoint + credential identity + API mode + display prefix.
- Wire excluded providers through all picker/list call paths and add regression tests/docs for the new config key.
Automated hermes-sweeper review.
| @@ -96,12 +97,14 @@ def load_picker_context() -> ConfigContext: | |||
| current_provider = "" | |||
There was a problem hiding this comment.
This only feeds inventory consumers. The gateway /model path still calls list_picker_providers / list_authenticated_providers directly, so the new config would not hide providers on that picker surface unless those call paths also receive the exclusion list.
| @@ -1590,7 +1603,14 @@ def _has_aws_sdk_creds_for_listing(slug: str) -> bool: | |||
| continue | |||
There was a problem hiding this comment.
On current main this needs to preserve the newer grouping dimensions too: credential_identity and api_mode. Adding the display prefix is useful, but replacing the key with (api_url, api_key, prefix) would merge entries that differ by key_env or transport.
| @@ -1706,7 +1745,7 @@ def _has_aws_sdk_creds_for_listing(slug: str) -> bool: | |||
| # - Without an api_key AND no explicit models, fall through to | |||
There was a problem hiding this comment.
Current main uses discover_models: false as the explicit opt-out from live discovery. Skipping discovery for every API-key custom provider with a models dict would regress the default gateway/Bifrost case where the live catalog should replace a stale configured subset.
… grouping Reworks NousResearch#28218 against current main per review feedback (hermes-sweeper, PRR_kwDOPRF1G88AAAABDBIirw). Two independent improvements to the /model picker substrate. ### 1. `model_catalog.excluded_providers` config key Hide specific providers from the /model picker even when valid credentials exist (e.g. a stale Copilot/OpenRouter token cached in auth.json or found via the `gh` CLI). Matched case-insensitively against every key a provider can surface under — hermes_id / mdev_id (section 1), pid / hermes_slug (section 2), canonical slug (section 2b) — so `copilot` hides the provider regardless of which section emits it. Wired through ALL picker/list call paths, not just the inventory substrate: - hermes_cli/inventory.py: ConfigContext.excluded_providers, read from `model_catalog.excluded_providers` in load_picker_context(); forwarded in build_models_payload() (TUI gateway picker surface). - hermes_cli/model_switch.py: list_authenticated_providers() and list_picker_providers() accept excluded_providers; prewarm_picker_cache forwards it so the warm cache matches what the picker will show. - gateway/slash_commands.py: the gateway /model path (both the list_picker_providers interactive picker at ~L1132 and the list_authenticated_providers text fallback at ~L1351) now reads model_catalog.excluded_providers from config and passes it through. ### 2. Fix custom-provider grouping for shared-endpoint proxies Problem: multiple custom_providers entries sharing the same base_url (e.g. an Aperture/LiteLLM proxy fronting cerebras, groq and perplexity at one URL) collapsed into a single picker row under the first provider's name. Per the review, the grouping key is EXTENDED rather than replaced. Current main groups by (api_url, credential_identity, api_mode); this adds the display-name prefix as a fourth dimension: (api_url, credential_identity, api_mode, display_prefix) so same-URL different-name proxies each get their own row, while: - same-host entries with different key_env / api_mode stay separate (credential_identity / api_mode preserved — no regression to test_..._same_url_different_key_env_and_api_mode_stay_separate); - per-model suffix entries sharing a prefix ("Ollama — A", "Ollama — B") still collapse into one row. The display-name prefix is computed once and reused as the row's display name (the prior inline suffix-stripping is deduplicated against it). ### Not carried over from the original PR (per review) - The original replaced the grouping key with (api_url, api_key, prefix), dropping credential_identity / api_mode — that would have regressed same-URL entries with different env-backed credentials or transports. - The original skipped live /models discovery whenever a `models:` dict was present, regressing the default Bifrost/gateway case where the live catalog should replace a stale configured subset. Current main already uses `discover_models: false` as the explicit scoped-subset opt-out, so the proxy-subset use case is handled by telling users to set `discover_models: false` on those entries. No change to should_probe. - The original's slug-assignment rework targeted an older base where section 4 reused current_provider as the slug; current main already uses custom_provider_slug(display_name) with no current_provider reuse, so that change is obsolete. ### Tests (tests/hermes_cli/test_model_switch_custom_providers.py) - test_excluded_providers_hides_builtin_row: openrouter row appears with OPENROUTER_API_KEY set, disappears with excluded_providers=["openrouter"]. - test_excluded_providers_empty_is_noop: [] does not change output. - test_shared_url_different_display_names_are_separate_rows: three entries sharing base_url+api_key+api_mode but different names → three rows. - test_shared_url_per_model_suffix_still_collapses: "Ollama — A"/"Ollama — B" still collapse into one "Ollama" row. ### Docs (website/docs/reference/model-catalog.md) Document `model_catalog.excluded_providers` under the Config section. ### Known limitation / follow-up The interactive `hermes model` CLI picker (hermes_cli/main.py:: select_provider_and_model) builds its provider rows through a separate code path that does not use list_authenticated_providers / build_models_payload, so excluded_providers does not yet hide rows there. Wiring it in is a larger, separate change and out of scope for this review rework.
a66bd2d to
93ec97d
Compare
|
Thanks for the review — reworked against current 1. Excluded providers wired through all picker/list paths
Exclusion is matched case-insensitively against every key a provider can surface under: 2. Grouping key extended, not replacedPer the suggestion, the current key is preserved and the display prefix added as a fourth dimension: Same-URL different-name proxies (e.g. an Aperture proxy fronting cerebras/groq/perplexity at one URL) now each get their own row, while:
3. No live-discovery regressionThe original PR's TestsAdded to
Docs
Known follow-up (out of scope here)The interactive |
…I picker Follow-up to the excluded_providers rework: wire the config through the interactive `hermes model` CLI picker (hermes_cli/main.py:: select_provider_and_model), which builds its provider menu from CANONICAL_PROVIDERS via group_providers — a separate code path from list_authenticated_providers / build_models_payload that the prior commit did not cover. A canonical provider is hidden from the CLI menu if its slug OR any of its aliases (_PROVIDER_ALIASES) appears in model_catalog.excluded_providers (case-insensitive), matching list_authenticated_providers' matching against hermes_id / alias / canonical slug. The filtered slug list is passed to group_providers, so excluded members also drop out of multi-member group rows. Custom providers are intentionally not filtered (parity with list_authenticated_providers, which does not exclude section-4 custom rows). Tests (tests/hermes_cli/test_model_picker_excluded_providers.py): - test_cli_picker_hides_excluded_provider: excluded_providers=["openrouter"] removes the OpenRouter row from the provider menu. - test_cli_picker_hides_excluded_provider_by_alias: excluding by an alias (not the canonical slug) also hides the provider. - test_cli_picker_empty_excluded_is_noop: [] does not change the menu. Docs: model-catalog.md updated to note every /model picker surface (gateway, TUI, and `hermes model` CLI) now honors the key.
|
Follow-up: the "known limitation" noted in the previous comment is now resolved (commit
A canonical provider is hidden from the CLI menu if its slug or any of its aliases ( Tests added in
Every |
Summary
Two independent improvements to
list_authenticated_providersand the/modelpicker.1.
model_catalog.excluded_providersconfig keyAllows users to hide specific providers from the
/modelpicker even when valid credentials exist. Useful when credentials are present for legacy/testing providers that shouldn't appear in normal use (e.g. an old Copilot or OpenRouter token still cached inauth.jsonor discovered via theghCLI).Config:
```yaml
model_catalog:
excluded_providers:
- copilot
- openrouter
- openai
```
The exclusion is applied across all three credential-detection sections:
hermes_idandmdev_idpidand resolvedhermes_slugslugPropagated via a new
excluded_providersfield onConfigContextininventory.py, read frommodel_catalog.excluded_providersinload_picker_context().2. Fix custom provider grouping for shared-endpoint proxies
Problem: When multiple
custom_providersentries share the samebase_url(e.g. an Aperture/LiteLLM proxy fronting cerebras, groq, and perplexity at a single URL), they were collapsed into one picker row under the first provider's name with all models merged together.Root cause: Section 4 grouped entries by
(base_url, api_key)— identical for all entries behind a shared proxy — so only one row appeared.Fix: Include the display name prefix in the group key:
(base_url, api_key, name_prefix). Entries with the same URL but different names now each produce their own picker row. The existing"Provider — ModelName"suffix-stripping behavior for Ollama-style per-model entries is preserved (those share the same name prefix and still collapse correctly).Additional fixes in section 4:
current_provideras the slug when the provider name also matches, preventing all shared-URL entries from inheriting the active provider's slug (which causedgroqandperplexityto appear ascustom:cerebras-2,custom:cerebras-3, etc.)models:dict — the user has intentionally defined per-provider model lists on a proxy and the rows should always surface/v1/modelsdiscovery: Skip whenmodels:is explicitly defined in config — the proxy's endpoint returns all models across all backends, not just the ones scoped to this provider entryTest plan
excluded_providers: [copilot]tomodel_catalogconfig; verify Copilot row disappears from/modelpicker even withghCLI authenticatedcustom_providersentries pointing at the samebase_urlwith different names andmodels:dicts; verify both appear as separate rows with correct model counts"Provider — ModelName"suffix-stripping grouping still works (entries sharing URL + key + name prefix still collapse into one row)excluded_providers: [](empty) has no effect on picker outputGenerated with Devin