Skip to content

implement dropdown when hf inference provider is selected.#14

Merged
tarekziade merged 3 commits into
mainfrom
model-ckpt-dropdown
Jun 8, 2026
Merged

implement dropdown when hf inference provider is selected.#14
tarekziade merged 3 commits into
mainfrom
model-ckpt-dropdown

Conversation

@sayakpaul

Copy link
Copy Markdown
Member

Fixes #3

@sayakpaul

Copy link
Copy Markdown
Member Author

@askserge could you review the PR?

@sergereview sergereview Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤗 Serge says:

The PR adds a dropdown of HF Inference Provider tool-capable models when the "hf" provider is selected, falling back to a free-text input for other providers or when the router is unreachable. The implementation is mostly solid, but there's a correctness bug in the Python caching logic that prevents empty (but valid) fetch results from being cached, causing repeated network requests.

Correctness

  • _fetch_hf_router_models in reviewbot/webapp.py uses if cached and ... to decide whether to return the cached list. Because an empty list is falsy in Python, a successful fetch that returns no tool-capable models (or an empty list seeded in tests) will never be cached and will trigger a fresh HTTP request on every call. The check should use fetched_at > 0 (or cached is not None with a sentinel) to distinguish "never fetched" from "fetched and empty".

Security / Trust boundaries

  • The new /llm-options/hf-models endpoint correctly gates behind _require_user, so unauthenticated callers cannot enumerate models.
  • No secrets are logged or exposed in the new fetch path.

Client-side

  • The JS correctly guards against provider-switch races by re-checking providerEl.value === "hf" after the async fetch.
  • The text input remains the named form control and stays in sync with the hidden <select> via change listeners, so submitted values are correct.

Tests

  • Tests cover the payload parser, garbage tolerance, and the endpoint with a seeded cache. They do not exercise the TTL expiry or the "empty list should be cached" path.

8 LLM turns · 9 tool calls · 416.4s · 92348 in / 22161 out tokens

Comment thread reviewbot/webapp.py Outdated
now = time.monotonic()
with _hf_models_lock:
cached = _hf_models_cache["models"]
if cached and now - _hf_models_cache["fetched_at"] < _HF_MODELS_TTL_SECONDS:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cache check if cached and ... treats an empty list as a cache miss. This means a successful fetch that returns [] (or an empty cache seeded in tests) will never be served from cache and will trigger a fresh HTTP request every time.

Use the timestamp to determine whether the cache has been populated instead:

Suggested change
if cached and now - _hf_models_cache["fetched_at"] < _HF_MODELS_TTL_SECONDS:
with _hf_models_lock:
cached = _hf_models_cache["models"]
if _hf_models_cache["fetched_at"] > 0 and now - _hf_models_cache["fetched_at"] < _HF_MODELS_TTL_SECONDS:
return cached

@tarekziade

Copy link
Copy Markdown
Collaborator

What Serge says :)

@sayakpaul

Copy link
Copy Markdown
Member Author

@askserge could you review the PR?

@sergereview sergereview Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤗 Serge says:

The PR cleanly implements a dropdown of tool-capable HF Inference Provider models when the "hf" provider is selected, falling back to a free-text input for other providers or when the router is unreachable. The server-side parsing, caching, and endpoint are well-tested, and the client-side sync between the hidden text input and the visible select keeps form submission straightforward. No correctness or security issues were found.

6 LLM turns · 9 tool calls · 79.5s · 62779 in / 3908 out tokens

@sayakpaul

Copy link
Copy Markdown
Member Author

@tarekziade

@tarekziade tarekziade merged commit eccdbac into main Jun 8, 2026
1 check passed
@tarekziade tarekziade deleted the model-ckpt-dropdown branch June 8, 2026 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

allow passing provider + model to the github app

2 participants