Skip to content

Reconcile the two provider-aware eval-config helpers (allowlist vs denylist diverge for an unknown provider) #181

Description

@markballew

Summary

Two provider-aware GenerateConfig helpers in mcp_common.testing.eval now coexist and gate the Together/vLLM-only enable_thinking extra_body ({"chat_template_kwargs": {"enable_thinking": False}}) using opposite strategies. They agree on every known provider but diverge for an explicitly-named unknown provider — the overlap flagged for consolidation in the #170 / #175 reviews.

  • src/mcp_common/testing/eval/model_configs.py — generate_config_for_tier(tier, *, reasoning_effort=, provider=, model=) (evals: make generate_config_for_tier provider-aware (Anthropic models-under-test get Together/vLLM-only extra_body) #170) uses an ALLOWLIST (_THINKING_TEMPLATE_PROVIDERS = {together, vllm}): the extra_body is included only for those providers. An explicitly-named unknown provider ⇒ extra_body dropped. (A genuinely absent provider — neither provider nor a prefixed model — preserves the historical Together-safe default and keeps it.)
  • src/mcp_common/testing/eval/provider_config.py — generate_config_for_provider_tier(tier, provider="together") (evals: reporting + write-safety helpers (#169, #156, #125) #175) uses a DENYLIST (_NON_VLLM_PROVIDERS = {anthropic, openai, azure(ai), google, vertex, mistral, bedrock, groq, cohere}): an unknown provider ⇒ extra_body kept (Together-safe default).

Divergence (today)

provider arg generate_config_for_tier (allowlist) generate_config_for_provider_tier (denylist) agree?
together / vllm keep keep yes
anthropic / openai / google / bedrock / mistral / … drop drop yes
absent (no provider, no prefixed model) keep (Together-safe default) n/a (defaults to together ⇒ keep) yes
vllm-openai drop (not in {together, vllm}) keep (in its vLLM set) no
explicitly-named unknown (e.g. fireworks, deepinfra, xai) drop keep no

So there are actually two mismatch rows: the headline unknown-provider case, plus vllm-openai, which provider_config already treats as vLLM-style but model_configs does not.

Chosen reconciliation (delegation + allowlist)

Make model_configs.generate_config_for_tier the single source of truth and have provider_config delegate to it, so the duplicate decision logic — and the edge-case divergence — cannot recur:

  1. generate_config_for_provider_tier(tier, provider) delegates: return generate_config_for_tier(tier, reasoning_effort=reasoning_effort, provider=provider). No second copy of the gating logic.
  2. Unknown-provider rule = allowlist (drop). An explicitly-named provider we don't recognize as Together/vLLM no longer receives the Together-only extra_body. Rationale: this is the safer rule against the exact failure evals: make generate_config_for_tier provider-aware (Anthropic models-under-test get Together/vLLM-only extra_body) #170 was about — some providers (e.g. Anthropic) reject unknown extra_body keys with an HTTP 400. For an explicitly-named unrecognized provider, dropping a vendor-specific field we can't confirm is accepted fails soft (thinking simply isn't disabled) rather than hard (a 400 that fails the whole eval). It also keeps the already-adopted generate_config_for_tier(provider=, model=) (netbox-mcp) behavior unchanged. The absent/None and default-together cases still keep the extra_body, so existing Together runners are unaffected.
  3. Unify the vLLM allowlist as the union {together, vllm, vllm-openai} (add vllm-openai to _THINKING_TEMPLATE_PROVIDERS) so delegation does not regress that genuinely-vLLM provider, and the public provider_uses_vllm_chat_template / VLLM_CHAT_TEMPLATE_PROVIDERS keep recognizing it.
  4. Reimplement provider_uses_vllm_chat_template and VLLM_CHAT_TEMPLATE_PROVIDERS against that same canonical allowlist so the public predicate and the config function can't drift; drop the now-dead _NON_VLLM_PROVIDERS denylist.

Net behavior change (intentional, locked by tests)

  • generate_config_for_provider_tier(tier, "<unknown>"): keep ⇒ drop.
  • provider_uses_vllm_chat_template("<unknown>"): True ⇒ False.
  • generate_config_for_tier(tier, provider="vllm-openai"): drop ⇒ keep (correctness: vLLM-OpenAI honors the chat-template switch).
  • Everything else (known providers, None/unspecified, default together, all other levers) is unchanged. The public generate_config_for_tier(provider=, model=) signature/behavior for known providers is preserved.

Out of scope / follow-up

Full consolidation would eventually deprecate generate_config_for_provider_tier in favor of generate_config_for_tier(tier, provider=...) (they become near-identical after delegation, differing only in arg shape). That is an opinionated public-API change and is deferred to a separate change; this issue does the minimal, conservative reconciliation that removes the divergence while keeping all three public symbols exported.

Constraints: worktree off main only; no version/tag bumps (semantic-release owns them); netbox-mcp untouched.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions