Skip to content

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

Merged
jatezzz merged 6 commits into
mainfrom
fix/ADFA-5187-dynamic-n-ctx
Aug 31, 2026
Merged

ADFA-5187 | Dynamically size n_ctx based on model metadata and available RAM#75
jatezzz merged 6 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.

Comment thread ai-agent-local/llama-impl/src/main/java/android/llama/cpp/LLamaAndroid.kt Outdated
…al load

Rescan for the architecture alone when the strict parse gives up, so an entry it rejects can no longer make an embedding model read as chat-capable. Free the context, batch, sampler and model when load() fails partway; they had no owner, so each retry leaked another. Reconcile the mmap comments with charging weights.

@itsaky-adfa itsaky-adfa 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.

Round 4 review of 6667a3a, with a re-check of every issue flagged in rounds 1-3.

All 10 previously flagged issues are fixed. Verified independently, not taken from the reply comments:

Round 1 (hal-eisen-adfa) - the KV budget now subtracts the model weights before dividing (ContextSizePolicy.kt:63-64), with tests pinning both the 4.4 GB-in-3.5 GB floor case and "a bigger model gets less context from the same RAM". The native clamp is floored at DEFAULT_N_CTX, so a 2048-trained model still gets 4096 and no previously-working prompt regresses, and reportEffectiveContextSize reads getContextSize() back and warns on a mismatch. The circular memory gate is broken by pricing the pre-flight KV cache at ContextSizePolicy.DEFAULT_CONTEXT_TOKENS.

Round 2 (itsaky-adfa) - the signed underflow is closed on both subtractions; re-derived from scratch, both operands are now non-negative so no wrap is representable, and three tests pin it. Free-RAM reading is back below the header parse and the backend reads availMem after the unload. contextLength moved to the last constructor position with a KDoc rule. The double metadata parse is one GgufHeaderReader.read inside Dispatchers.IO feeding both the guard and the sizing, and GgufModelInspector's duplicate parser is gone.

Round 3 (hal-eisen-adfa) - readArchitecture rescans with no entry-count ceiling and returns at the first general.architecture; the new GgufModelInspectorTest asserts read returns null and the kind is still EMBEDDING for both an unknown trailing value type and an overstated entryCount. The partial-load handle leak is fixed on both paths: Kotlin frees sampler -> batch -> context -> model and rethrows, and on the JNI path throw_java + return 0 surfaces at the assignment so context stays 0L and the catch frees the model. Both mmap comments are reconciled.

Also checked: the committed libs/v8/llama-v8-release.aar is genuinely rebuilt (native_configureContext is gone from libllama-android.so, classes.jar has load(String,int) and 4 free_* calls with an exception table in LLamaAndroid$load$3), :ai-agent-local:testReleaseUnitTest passes, every ThrowNew(FindClass(...)) is converted to throw_java, every completion_init exit releases jtext, no stale callers of the removed APIs remain, and chunked prefill's chunk_limit = min(2048, llama_n_batch) keeps common_batch_add's seq_id[n_tokens] probe in bounds despite this fork's new_batch dropping upstream's +1 sentinel.

New findings: none critical or high, all posted inline. Three SHOULD FIX (a latent overflow two KDocs still deny, an untested fix, a blocking stat outside the IO dispatcher), one SHOULD FIX on the PR description, and two NITPICK (a dead fail-open API, a pass-through wrapper) plus a comment-placement nit.

The description also still says the change "extends the GgufModelInspector to parse the <architecture>.context_length" - that parsing lives in GgufHeaderReader, and GgufModelInspector's own parser was deleted in this PR. Worth a pass over the body before merge; QA reads it.

@itsaky-adfa itsaky-adfa 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.

Approving. All 10 issues from rounds 1-3 are fixed and independently verified (details in my round 4 review above), and nothing in the new findings is critical or high.

@jatezzz please address the four SHOULD FIX items from that review before merge:

  1. ModelMemoryEstimator.kt:84 - clamp contextTokens, or move the bound into the estimate KDoc as a caller obligation. The overflow is not reachable from today's callers, but the two KDocs currently deny a wrap that is now possible.
  2. LocalLlmBackend.kt:297 - move the isFile stat inside an IO block; it is the one piece of blocking filesystem work left on the caller's dispatcher.
  3. LocalLlmSettingsViewModel.kt:288 - add the one test that pins the KV cache to DEFAULT_CONTEXT_TOKENS regardless of free RAM, so the circular gate cannot come back unnoticed.
  4. PR description - it points reviewers at the deleted LLamaAndroid.configureContext(...), credits GgufModelInspector with parsing that now lives in GgufHeaderReader, and omits both the chunked prefill and the regenerated libs/v8/llama-v8-release.aar. QA works from this text.

The three NITPICK comments are yours to take or leave.

Approving rather than blocking because none of these can produce a wrong result on the shipped paths: fix 1 is latent, 2 is a narrow window on an already-slow operation, 3 is regression insurance, 4 is documentation. I would still rather see 3 and 4 land here than as a follow-up, since both get much harder to justify once the PR is merged.

…n IO

Price the pre-flight through estimateForSelection, which takes no free-RAM figure at all; drop the dead classify overload and the resolver pass-through.
@jatezzz
jatezzz merged commit fa1f624 into main Aug 31, 2026
1 check passed
@jatezzz
jatezzz deleted the fix/ADFA-5187-dynamic-n-ctx branch August 31, 2026 18:34
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