[Feat]: forward all samplers and parse full /props for llama.cpp - #895
[Feat]: forward all samplers and parse full /props for llama.cpp#895a-ghorbani wants to merge 24 commits into
Conversation
resolveRemoteProps had no direct test: only samplerDefaults was reached, so a dropped slotCount, buildInfo, modelAlias or chatTemplateCaps passthrough would have been silent. The pal sheet's server-default test asserted a first render, which passes without observer; a probe that lands while the sheet is open does not.
A misspelled control name indexed a Partial<Record<SamplerParam, number>>
as a plain string, which yields `any` under the project's noImplicitAny
setting: the server-default indicator would silently never render for that
control, with no error anywhere. Naming the parameter surfaces the typo, and
surfaced two latent errors the loose index had been hiding.
capsMatchBinding took a structural `{probedUrl?: string}`, which any of the
three tiers satisfies, defeating the discriminant that is supposed to keep a
descriptive record out of a capability position.
The mock re-implemented the lookup verbatim, so the two could drift without a test noticing. Both now call the same function. The name says what the value is: the last observation, never expired here, so a consumer needing freshness bounds it against `at` itself.
|
Posted as a comment rather than a formal Request changes review: the bot identity was unavailable ( Independent reviewverdict: REQUEST_CHANGES No blocker in the code. One blocking process condition, seven concerns, and a BlockingBL1 — No visual evidence on the PR. The cause is environmental, not authorial: It is worse than a missing upload, because of C2 below: the four captures cover ConcernsC1 — "Reset to server default" writes a value the app's own validator then Found independently by five reviewers. The reported default is passed to The chip renders and presses; Save then aborts with an alert naming a parameter Fix: return C2 — The "remote turn footer" change fires on every local turn, undescribed, Raised by two reviewers; I verified it directly and both understated it. The So every local turn footer gains two new metrics, not one. Both new Fix: decide deliberately — either gate on origin, or accept the local change, C3 — Server sampler defaults are rendered on, and reset into, two scopes that
C4 — The volatile tier is structurally assignable into both persisted tiers.
C5 — Unbounded wire strings are persisted, and on Android one probe can break
Two qualifications from the refutation, both material: the consequence is C6 — The new probe coalescing is a PR-introduced regression on two recovery
Impact is narrower than first reported: the reprobe fires on every foreground C7 — Two hand-maintained lists sit directly under a comment reading "Enumerated once Suggestions
Verified clean — recorded so a follow-up pass does not re-derive it
Verification
Residual risk
Open questions for the author
Eight role reviewers plus an adversarial refutation pass, none of them the implementer, against a separate worktree built from the PR ref. Two single-sourced concerns were downgraded by the refutation and one had its impact corrected; the surviving ones are recorded with what was done to try to break them. Generated by PocketPal Dev Team |
llama.rn reports prompt_per_second and cache_n on local completions too, and both are non-optional there, so gating on presence silently added two metrics to every local turn footer. Origin gates them instead. The test asserts both arms from one timings object: suppressed on a local turn, present on a remote one.
A server run with `--top-k 0` reports a default outside this app's range, so the reset offered a one-tap write that saving then rejected with an alert naming a parameter the user never typed.
Two review findings fixedThe footer parts are now gated on origin, not presence ( llama.rn declares both The test asserts both arms from one "Reset to server default" no longer offers a value saving would reject
Both fixes were checked by reverting them: exactly two tests fail, one per fix, Gates: 274 suites, 4448 passed, 2 skipped. Typecheck and lint clean. The remaining review findings are unaddressed and still stand — in particular the Visual evidence is still not posted; that is an environment blocker, not an Generated by PocketPal Dev Team |
![pocketpal-dev-team[bot]](https://avatars.githubusercontent.com/in/4815089?s=60&v=4){kind=small}
There was a problem hiding this comment.
REQUEST_CHANGES — formalising the verdict already reached. Findings and
evidence are in the full review;
this review does not restate them.
Current state, not the state at the time of that review:
- 1 blocking — no visual evidence posted. Environment, not authoring: the
upload tool has no session token. The captures that exist cover the remote
surfaces only, as the PR body now states. - 2 of 7 concerns closed in round 2
— the footer parts now gate on origin, and the reset no longer offers a
default that saving would reject. Both checked by reverting them. - 5 concerns open. One (server defaults presented on sheets configuring a
different model) is deliberately deferred to a follow-up. Four are
unaddressed: the volatile tier is structurally assignable into both persisted
tiers;build_info/model_aliasare unbounded persisted strings against a
6 MB Android store ceiling; the new probe coalescing regresses two recovery
paths; andPROPS_SCALAR_FIELDSreintroduces the drift its neighbour exists
to prevent.
No code blocker. Eight role reviewers plus an adversarial refutation pass, none
of them the implementer.
Generated by PocketPal Dev Team
Visual evidence — remote surfaces
Generated by PocketPal Dev Team |
|
Verdict status — kept current as rounds close. Visual evidence is posted (Edited twice. An earlier revision said the upload "was never actually blocked" Open: 1 of the original 7 concerns. Server sampler defaults are shown on, and reset into, Pal and preset sheets that Closed: 6. The footer parts gate on origin rather than presence; the reset The Generated by PocketPal Dev Team |
… one Every field of both persisted tiers is optional, so without a discriminant the presence record was structurally assignable into either, and a mistyped write would have persisted a reading of the moment as a settled fact.
A 4MB build_info produced an 8.4MB persisted blob against Android's 6MB store ceiling, and the store is written as one record, so servers and the privacy acknowledgement go down with it. Nothing read either field. Deleting them buys an invariant a cap does not: every persisted field is now a number, a boolean, or the url the user typed. No server-supplied free text is stored at all, which is checkable by inspection and needs no constant. A cap would also have left the aggregate open, since the map is keyed per model and the model list is the server's to choose. The parse helper went with them; they were its only callers.
The in-flight map was keyed without the url, so a refresh issued after a server edit joined the pre-edit request and reported its result for the new backend. It also outlived backgrounding. The foreground reprobe exists because the first probe is the request that raises the local-network prompt, so a grant always arrives after it has failed; joining that request returns the failure it was meant to replace. The no-op write check now derives its scalar set from the field list rather than restating it, so a field added to one cannot be skipped by the other.
Four review concerns fixedEach was a defect this change introduced, rather than an improvement it declined Unbounded server strings are no longer persisted ( They were deleted rather than capped, on an independent reading. Deletion buys The volatile tier can no longer be assigned into a persisted one Probe coalescing no longer answers the wrong question ( The same commit derives the no-op write check's scalar set from the field list Checks274 suites, 4450 passed, 2 skipped. Typecheck and lint clean. Each fix was checked by reverting it. Re-adding the string parse fails the test The one exception is stated rather than implied: the derived scalar set has no No visual change, so the posted captures are unaffected. Generated by PocketPal Dev Team |
|
Scope of the independent review, stated because it is narrower than it reads. The review above was conducted against Each was written in response to a finding from that review, each was checked by "Independent review completed" is a claim about a specific tree. It is true of Generated by PocketPal Dev Team |
Clearing the in-flight map leaves earlier probes pending, so deleting by key let a late settle evict the replacement registered under that key — defeating coalescing for exactly the reprobe the clear exists to allow. On iOS that is the likely ordering rather than a corner: the abort timer is frozen while suspended and fires on resume. The merge now carries forward only declared fields. Spreading the hydrated entry meant a field removed from the schema was copied forward for the life of that entry, so a removal could never take effect. A scalar set derived from the field list stops a new field being skipped, but not a new object-valued one being compared by identity — which, against a body parsed fresh each probe, is never equal and rewrites the map every time. That is now a compile error.
Without a remote turn snapshot the parts were withheld by the origin gate, not by the server's silence, so the test passed whether the gate existed or not and nothing covered a remote turn on a build that omits prompt speed.
Delta review of the fix commits, and what it foundThe review posted earlier covered It found a bug in one of the fixes, which is the outcome that justified A per-commit check could not have caught it. Each of those commits was verified Three smaller findings, all fixed:
Two tests also falsified by timeout rather than by assertion. A hang reads as Every fix carries its own check: reverting the identity delete reproduces the 274 suites, 4451 passed. No visual change, so the posted captures are unaffected. Generated by PocketPal Dev Team |
1 participant
Remote llama.cpp servers currently receive only
temperature,top_p,max_completion_tokens,stop,stream,tools,tool_choice,response_formatand the reasoning kwargs. Every other sampler the UI exposes isdropped on the floor, so those controls lie.
/propsis read forn_ctxandmodalities.visiononly. Remote turns show no timings.This changes three things.
1. Forward the full sampler set
top_k,min_p,typical_p,xtc_probability,xtc_threshold,mirostat,mirostat_tau,mirostat_eta,seed,n_probsand the four penalties nowreach the server.
Read and write are deliberately separate:
PARAM_WIRE_NAME— one table, all sixteen numeric controls, used to readserver defaults out of
/props.FORWARD_ALLOWLIST— per server type, decides what is written into arequest body.
One name table, two policies. A server type that has not been measured gets a
smaller allowlist without needing a second copy of the names.
Four of the sixteen need renaming on the wire. PocketPal's internal
penalty_last_n/penalty_repeat/penalty_freq/penalty_presentarenot llama-server field names — they are silently ignored. The correct names
are
repeat_last_n/repeat_penalty/frequency_penalty/presence_penalty.2. Parse the whole
/propsresponseThe response is split into three tiers by lifetime, not by subject:
is_sleeping, slot countsPersisting a volatile field is how a capability model goes stale without
anything reporting it, so presence is deliberately not persisted.
The sampler settings sheets now show the server's own default next to each
control, with a per-parameter reset. The router placeholder response
(
role: "router",model_path: "none",params: null,n_ctx: 0) isrecognised and rejected rather than parsed as if it described a model.
3. Per-turn timings for remote turns
The footer renders
predicted_per_second,prompt_per_secondandcache_nfromthe streaming finish chunk. A reported zero is kept distinct from an absent
value —
0 cachedand "no number" are different facts and must not collapse.Wire verification
Every claim below was measured against a running
llama-server, not read fromdocumentation. Six raw response bodies were captured verbatim, and every
wire-shape fixture in these tests is copied from one of them character for
character — none is hand-authored or transcribed from a README. A wire name
hand-authored in both the code and the fixture lets a typo pass in both: the test
then proves the parser agrees with itself rather than with the server. This
server was measured contradicting its own documentation more than once, so a
doc-derived fixture would have encoded that error and gone green.
Environment:
llama-serverin router mode, buildb9976-e3546c794, modelbartowski/Qwen_Qwen3-1.7B-GGUF:Q4_K_M, 4 slots,n_ctx8192.Samplers that landed
Sent with distinctive values, then read back from
GET /slots?model=<id>on theslot with the highest
id_task. A value differing from the server default provesthe request name was honoured.
top_k: 11→11·min_p: 0.11→0.10999999940395355·typical_p: 0.91→0.9100000262260437·xtc_probability: 0.21→0.20999999344348907·xtc_threshold: 0.31→0.3100000023841858·mirostat: 2→2·mirostat_tau: 4.1→4.099999904632568·mirostat_eta: 0.21→0.20999999344348907·seed: 12345→12345·n_probs: 2→2·temperature: 0.33→0.33000001311302185·top_p: 0.77→0.7699999809265137Samplers that did NOT land — the negative result
Sent as
penalty_last_n: 41,penalty_repeat: 1.11,penalty_freq: 0.41,penalty_present: 0.51, the slot kept its defaults (64,1.0,0.0,0.0)and the request still returned
200. Re-sent under llama-server's own names,seconds apart, same instance, same model, same slot pool, same body shape — only
the four field names differed — all four landed:
41,1.1100000143051147,0.4099999964237213,0.5099999904632568.The second request is what makes the first one mean something. Without it, the
unchanged defaults would only show that something went wrong — a typo, a wrong
endpoint, a server that ignores penalties entirely. The control rules those out.
Reasoning: one scope line was removed because it does nothing
reasoning_content"Okay, the"chat_template_kwargs:{enable_thinking:false}nullreasoning_effort:"none""Okay, the"reasoning_effort:"none"+reasoning_format:"auto""Okay, the"reasoning_budget_tokens: 512"Okay, the"totally_bogus_key_xyz: 123"Okay, the"reasoning_effortis not a llama-server request field — it is absent fromtools/server/server-schema.cppupstream and is ignored here. Shipping it as anoff switch would have been green and done nothing.
enable_thinkingremains thesole carrier of on/off.
reasoning_budget_tokensis the correct upstreamname (
-1 .. INT32_MAX,-1disables); it is inert on this build and honouredon newer ones, and an ignored numeric field cannot break a request.
The last row of that table also sets the failure mode for this server type: a
silent no-op, never a rejection.
Timings
The finish chunk of a
stream: truecompletion carriestimingswithpredicted_per_second,prompt_per_secondandcache_n— all three present.No
usageobject appears anywhere in the stream without an explicitinclude_usageopt-in, which is why cached tokens are sourced fromtimings.cache_nand notusage.cached_tokens.Why the read-back is a required step and not a note
A wrong wire name produces no error, no log line, and nothing a user could tell
apart from ordinary model variance. The consequence is silent and no symptom
will ever surface it, so this observation is the only evidence that will ever
exist. No amount of additional upstream testing substitutes for it: a test
appearing tomorrow would raise confidence that the names are right, and leave
the check that they landed exactly as unsubstitutable.
Two nearby signals are not weaker versions of it — they are coincidences that
read as confirmation:
200." Measured above: unknown body keys return200. A status code says nothing about any field.produces that with no sampler applied at all, and a seeded comparison would
require
seeditself to have landed — one of the sixteen names in question.Three claims not to make about this evidence
Each was written, believed, and corrected while building this. Kept in the wrong
form on purpose: a reader arriving with the wrong version already in their head
recognises it and stops, where they might read past the right version without
noticing they disagree.
upstream assertions are on
POST /completion, read non-streaming fromres.body. The footer reads the OpenAI-compatible/v1/chat/completionsSSEfinish chunk — a different endpoint and a different delivery mode.
total_slotsfor the call this code makes."Upstream hits bare
GET /propson a direct single-model server. This codeissues
/props?model=<id>against a router, where the bare form returns theplaceholder.
not —
normalizeUrlstrips trailing slashes at all four request-constructionsites. This was measured against the server directly, which bypassed the app's
own normalisation. A
/v1suffix is fatal and is a separate fact.The shape common to all three: the measurement was correct and the scope of the
claim was not. Check the endpoint, the accessor, the delivery mode, and whether
the app's own code sits between you and the wire.
Limits
Every fact above was measured on one build,
b9976-e3546c794. Generalisationacross builds is unverified. Users point the app at whatever build they run.
Most of it fails safe: an absent
/propskey leaves the field unknown byconstruction, absent timings fields drop that part of the footer, and an ignored
numeric field is inert. The sixteen sampler names are the exception — a
rename would stop them landing, silently. That degrades to today's behaviour
rather than regressing past it, but it is not safe, which is why the read-back is
a step rather than a footnote.
Still unverified, labelled as such rather than assumed:
reasoning_budget_tokensandchat_template_caps.supports_thinkingon buildsnewer than
b9976. An absent capability key reads unknown, neverfalse.top_k/min_p/repetition_penalty.What the captures cover, and what they do not
Four captures are posted on this PR.
They evidence the remote surfaces only:
the two sampler sheets showing the server-default indicator and its reset, and
the remote turn footer cold and warm — the warm one showing prompt speed
150 → 1406 tok/s with
0 cached→12 cached, which demonstrates thereported-zero-vs-absent rule inside a single capture.
They do not cover the local turn footer. The footer parts here are gated on
origin, so a local turn must render neither of them, and there is no capture of
that. It is carried by a paired test instead, deliberately: the claim is an
absence, and a screenshot of an absence cannot distinguish "correctly
suppressed" from "the build was stale" or "that turn never populated timings".
The test asserts both arms from one
timingsobject under one harness —suppressed on a local turn, present on a remote one — because a test asserting
only the local absence would pass equally well if the whole footer were broken.
Stated because an evidence set with an unstated boundary gets read at its widest,
and a capture cannot correct for that itself.
These captures were blocked from upload for several hours, by two independent
faults, either of which was sufficient on its own. Recorded here rather than
dropped, because a public artefact that names one cause for a two-cause failure
is its own defect:
profiles live under
~/snap/..., while the upload tool searches only theclassic paths. It could not read a cookie store at all. Fixed by bridging the
snap profile into
~/.mozilla/firefox— and a directory symlink was notenough, because the tool walks directories without descending into symlinked
ones; it needed file-level links.
--repoto a subcommand that rejectsit, read the resulting non-zero exit as "no valid credential", and discarded
the stderr that said otherwise.
Both were live the whole time, and the environment one came first — so fixing
either alone would have changed nothing. An earlier revision of this section
named only the second, which understated it.
This was not a false alarm. The gate was right that the upload could not
proceed — it was wrong about why, and it destroyed the message that would have
said so. Two live faults produced one identical symptom, and the only text that
distinguished them was generated and discarded on every run. Re-running the
check would not have helped; reading its stderr would have.
Capture provenance
The captures were taken at 07:40-07:41 from an APK built at 07:38, which is the
right order — but two later commits (10:06) touch exactly the two captured
surfaces, so the captures predate them and were deliberately not retaken.
That carry-forward is not assumed, it is shown: current code was rendered under
each capture's own conditions and reproduces them — the sampler sheet still
offers
Server default: 0.8 · Reseton temperature with the other three readingas on-default, and the warm footer still produces
7ms/token, 135.81 tokens/sec, 1406.35 prompt tokens/sec, 12 cached, 107ms TTFT.The check discriminates: substituting an out-of-range server default makes the
sampler assertion fail.
Worth stating because a stale capture is harder to catch than a stale build. A
stale build fails a content check — the new string is simply absent. A stale
capture is a real screenshot of a real build, with the right name, surface and
resolution; nothing about the file is wrong except which build it depicts, and
no inspection of the image can reveal that.
Reading the diff
ChatGenerationSettingsSheet.tsxandPalGenerationSettingsSheet.tsxshow ~830changed lines but are 33 insertions / 12 deletions under
git diff -w. Therest is a forced reindent from wrapping both in
observer(...). Read those twowith
-w.Tests
274 suites, 4446 passed, 2 skipped. Lint and typecheck clean against baseline;
l10n:validatepasses. No native changes.A misspelled sampler name used to index a
Partial<Record<SamplerParam, number>>as a plain
string, which yieldsanyunder this project'snoImplicitAnysetting — the server-default indicator would have silently never rendered for
that control. Naming the parameter makes it a compile error, and doing so
surfaced two further errors the loose index had been hiding.
Docs
The architecture record for remote servers is updated in the same round as this
code, in
context/architecture/remote-servers.md.Generated by PocketPal Dev Team