Skip to content

ADFA-5187 | Dynamically size n_ctx based on model metadata and available RAM - #75

Open
jatezzz wants to merge 3 commits into
mainfrom
fix/ADFA-5187-dynamic-n-ctx
Open

ADFA-5187 | Dynamically size n_ctx based on model metadata and available RAM#75
jatezzz wants to merge 3 commits into
mainfrom
fix/ADFA-5187-dynamic-n-ctx

Conversation

@jatezzz

@jatezzz jatezzz commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Description

This PR updates the llama.cpp context size (n_ctx) to be dynamically calculated at model load, replacing the hardcoded 4096 value. It extends the GgufModelInspector to parse the <architecture>.context_length from the GGUF metadata header and checks the device's available system RAM. The optimal n_ctx is now computed as the minimum of the model's supported context length, the affordable context within the RAM budget, and a sane system ceiling. This change prevents unexpected Android low-memory kills on lower-end devices while unlocking the full context potential for models and devices that can handle > 4096 tokens. The implementation fails open, gracefully falling back to the default 4096 if metadata is missing or unreadable.

Details

Logic-related changes. Please review the Android logcat during model initialization; you will see logs indicating the parsed model context length, the available RAM snapshot, and the resulting computed n_ctx being passed to LLamaAndroid.configureContext(...) before context creation.

image
2026-08-21 11:12:52.065 28532-28789 AiAgentLoc...ma-android com.itsaky.androidide                I  model: loading from /data/user/0/com.itsaky.androidide/files/llm-models/1796906021_675710816_qwen2.5-0.5b-instruct-q8_0.gguf
2026-08-21 11:12:52.346 28532-28789 AiAgentLoc...ma-android com.itsaky.androidide                I  context: using 6 threads (batch=6)
2026-08-21 11:12:52.363 28532-28789 AiAgentLoc...ma-android com.itsaky.androidide                I  context: created with n_ctx = 16384 (requested 16384, model trained for 32768), n_batch = 2048
2026-08-21 11:12:56.272 28532-28789 AiAgentLoc...ma-android com.itsaky.androidide                I  prefill: n_len = 1024, n_ctx = 16384, n_kv_req = 2007
2026-08-21 11:12:56.278 28532-28789 AiAgentLoc...ma-android com.itsaky.androidide                I  prefill: 983 tokens (0 reused from cache) in 1 slice(s) of at most 2048

Ticket

ADFA-5187

Observation

The fallback mechanism is completely safe and mirrors the previous behavior (defaults to 4096) on any parse failure. The RAM snapshot excludes the currently loaded context to ensure the budget accurately reflects available memory.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

Tip: disable this comment in your organization's Code Review settings.

n_ctx was a fixed 4096. It is now chosen per load from the model's advertised context_length and free RAM (ContextSizePolicy, floor 4096, ceiling 16384), and the native prefill feeds the batch in slices so the larger context cannot overrun it.
@jatezzz
jatezzz force-pushed the fix/ADFA-5187-dynamic-n-ctx branch from bc75362 to b4efd66 Compare August 26, 2026 20:14

@hal-eisen-adfa hal-eisen-adfa 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.

Three findings from a review of the dynamic n_ctx change. All three are about the memory accounting around the new context sizing rather than the sizing itself.

Comment thread ai-agent-local/llama-impl/src/main/cpp/llama-android.cpp Outdated
Floor the native trained-context clamp at 4096 and log the n_ctx actually created; price the pre-flight warning at the floor to break its circularity.
@jatezzz
jatezzz requested a review from hal-eisen-adfa August 27, 2026 14:52
…rd sizes

Clamp both KV-budget subtractions so an oversized model can no longer underflow into the 16384 ceiling, read the metadata block once for both the embedding guard and the sizing, take free RAM after the parse, and append GgufHeader.contextLength.

@hal-eisen-adfa hal-eisen-adfa 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.

Three follow-ups on the latest round. The seven earlier threads all check out in 5d181b3 and the new tests cover the cases they promised, so nothing here reopens those. Two of these are consequences of the dedup fix and the new variable n_ctx respectively; the third is a doc inconsistency the weight-subtraction fix left behind.

// working chat model, so a wrong selection never tears down a good one. See ADFA-4388.
val kind = GgufModelInspector.classify(resolvedPath)
if (kind.isEmbeddingOnly) {
if (GgufModelInspector.classify(header).isEmbeddingOnly) {

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 encoder-only crash guard now fails open in more cases than it used to.

Folding the two metadata parses into one is the right call, but it also widened what can defeat this guard, and that wasn't priced in.

Before 5d181b3, GgufModelInspector.classify(path) used readArchitecture, which returned the moment it saw general.architecture — conventionally the first KV entry. Nothing later in the metadata block could affect classification, and that parser had no caps at all.

It now consumes GgufHeaderReader.read, which must walk every KV entry to completion. Any of these makes it return null: an unknown value type, entryCount > MAX_METADATA_ENTRIES (4096), an array declaring more than MAX_ARRAY_ELEMENTS (2^22), a block over MAX_METADATA_BYTES (64 MB), or a key over MAX_STRING_BYTES (1 MB). A null header classifies as ModelKind.UNKNOWN, and isEmbeddingOnly treats that as chat-capable.

So an embedding GGUF (BERT / nomic-bert family) produced by a converter that emits any construct this stricter parser rejects now passes the guard, llama_decode runs causal attention on an encoder-only model, and llama.cpp abort()s — SIGABRT, no Kotlin try/catch can intercept it, whole IDE process gone. That is exactly what ADFA-4388 added this guard to prevent.

One parse in the happy path is still worth keeping. Falling back to an architecture-only scan when read returns null would restore the old robustness without reintroducing the second parse for well-formed files.

Comment on lines 255 to 259
val model = load_model(pathToModel)
if (model == 0L) throw IllegalStateException("load_model() failed")

val context = new_context(model)
val context = new_context(model, nCtx)
if (context == 0L) throw IllegalStateException("new_context() failed")

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.

load() leaks the model handle when new_context fails — and this PR is what makes that reachable.

If new_context returns 0, the throw on line 259 leaves model — an already-mmap'd multi-GB llama_model — with no owner: there is no try / free_model here, and threadLocalState stays Idle. LocalLlmBackend turns the exception into a ModelLoadException, the user retries, and every retry mmaps and leaks another model for the process lifetime. new_batch / new_sampler below have the same problem and leak the context too.

The leak itself predates this PR. What changes is reachability: n_ctx was an unconditional 4096, so llama_init_from_model returning null was near-impossible. It can now ask for up to 4x the KV cache, sized from an availMem snapshot taken before llama.load() spends seconds mmapping and paging a multi-GB file. That moves this branch from theoretical to plausible on a loaded device — and retrying after a "model failed to load" message is precisely what a user does next.

A try / catch that frees whatever has been allocated so far before rethrowing would keep the failure recoverable instead of cumulative.

// Weights first, then the compute buffers, each clamped at zero rather than left to run
// negative: an unclamped Long would underflow on an absurd size and wrap to a huge
// positive budget, turning "no RAM at all" into the ceiling instead of the floor.
val afterWeights = (freeBytes - weightBytes).coerceAtLeast(0L)

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.

This subtraction and the module's own mmap comments now state opposite memory models.

Charging the weights against free RAM is what I asked for and the reasoning still holds — the weights page in from the same pool the KV cache is allocated from. But two comments in neighbouring files on the same load path still assert the opposite, and they were not updated:

  • ModelLoadDiagnostics.kt:41-43 — "Weights are mmap'd, so the file need not fit in free RAM — only the KV cache and compute buffers must be resident."
  • ModelMemoryEstimator.kt:8 (@property loadBytes) — "the weights. mmap'd, so they need not fit"

Neither line is in this diff, which is how they survived. The result is that the next person to touch memory accounting gets contradictory guidance depending on which file they open first, and both of those comments are load-bearing explanations rather than throwaway notes.

Worth reconciling here: either say at this line why the residency assumption differs for context sizing (the snapshot is taken before a load that then pages the weights in), or soften the other two. They should not keep claiming the file need not fit while this line requires exactly that.

One behavioural consequence to confirm is intended rather than incidental: a 4.4 GB Q4 7B advertising 32768 context on a device reporting 3.5 GB availMem clamps afterWeights to 0 and lands on the 4096 floor. The large models where a bigger context helps most are the ones that never get one. If that is the deliberate conservative outcome, fine — flagging it so it is a decision on the record.

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.

3 participants