Skip to content

Give LlamaCppProvider an inspect() so local fingerprints use /props - #2033

Closed
HarshRajSinghania wants to merge 2 commits into
MODSetter:devfrom
HarshRajSinghania:fix/llamacpp-provider-inspect
Closed

HarshRajSinghania wants to merge 2 commits into
MODSetter:devfrom
HarshRajSinghania:fix/llamacpp-provider-inspect

Conversation

@HarshRajSinghania

@HarshRajSinghania HarshRajSinghania commented Sep 29, 2026 •

Copy link
Copy Markdown

Summary

_collect() already calls provider.inspect(model_name) for a local model. LlamaCppProvider had no inspect(), so the AttributeError was caught and the fingerprint came from the filename only.

This adds inspect() on the adapter. It reads /props once at selection time and passes the payload to the existing from_llamacpp(), which prefers general.parameter_count and falls back to the filename only when the runtime states none.

Motivation

Fixes #1989. A local 70B renamed without a size in the filename was being tiered as compact.

Implementation

  • LlamaCppProvider.inspect() calls RouterClient.props() and from_llamacpp(). It does not write the capabilities cache; that cache exists for per-turn chat, and this runs once when a model is chosen.
  • FakeRouter can state a parameter_count on /props so the unit tests can drive a name with no size against a runtime that does state one.

The try/except safety net in _collect() is left in place so a failed /props read still falls back to from_name().

Testing

The new unit tests assert:

  • a model named renamed-weights with general.parameter_count = 70e9 fingerprints as 70.0B
  • inspect() and capabilities() each make their own /props call

I did not run the full uv run pytest -m unit suite in this environment (no project venv / llama.cpp test extras). The added tests follow the existing FakeRouter style in test_provider.py.

Please run from the issue:

cd surfsense_local/backend
uv run pytest tests/unit/llm/providers/llamacpp/test_provider.py -m unit
uv run ruff check .

High-level PR Summary

This PR fixes a fingerprinting issue where local LLM models without size information in their filename were being incorrectly tiered. It adds an inspect() method to LlamaCppProvider that reads parameter count from the llama.cpp runtime's /props endpoint, ensuring accurate model classification based on actual model properties rather than just filename parsing.

⏱️ Estimated Review Time: 5-15 minutes

💡 Review Order Suggestion
Order File Path
1 surfsense_local/backend/tests/unit/llm/providers/llamacpp/fake_router.py
2 surfsense_local/backend/tests/unit/llm/providers/llamacpp/test_provider.py
3 surfsense_local/backend/modules/llm/providers/llamacpp/provider.py

Need help? Join our Discord

Selection already calls provider.inspect() for local models, but
LlamaCppProvider had no such method. The AttributeError was swallowed
and fingerprints fell back to the filename.

inspect() reads /props once at selection time and hands the payload to
from_llamacpp(), which prefers general.parameter_count and only then
the name. It does not share the capabilities cache used by chat.

Fixes MODSetter#1989
@vercel

vercel Bot commented Sep 29, 2026

Copy link
Copy Markdown

@HarshRajSinghania is attempting to deploy a commit to the Rohan Verma's projects Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: b774622e-3535-4aca-9344-5d9d470d505b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@MODSetter

Copy link
Copy Markdown
Owner

Closing as superseded by #2034, which adds the same inspect() to LlamaCppProvider from the same branch name. Only one can land.

#2034 won on two points:

  • Docs. docs/architecture/local-models/selection.md currently states "LlamaCppProvider has no inspect(), so the call fails" in the body and carries a matching Known gap line. Your change makes both statements false but leaves them in place; fix(local): inspect llama.cpp model size at selection #2034 rewrites the body and deletes the Known gap in the same PR, which is what CONTRIBUTING asks for.
  • Imports. fix(local): inspect llama.cpp model size at selection #2034 uses the one re-export the package already provides, from modules.llm.profile import Fingerprint, from_llamacpp. This PR splits it into two submodule imports and puts from modules.llm.profile.types import Fingerprint after modules.llm.providers.types, which ruff's isort would reorder.

One other thing worth mentioning for future PRs: the diff escapes the quotes in an existing docstring (such as \"load now\") inside a """ string, where they do not need escaping. Unrelated to the fix and it makes the diff harder to read.

The props_calls == 2 test asserting inspect() does not share the capabilities cache was a nice touch, and a good instinct.

@MODSetter MODSetter closed this Sep 29, 2026
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.

2 participants