Skip to content

feat(llm): bound generation length, not only the wait (#422) - #439

Merged
jasonssdev merged 1 commit into
mainfrom
feat/422-bound-generation-length
Aug 6, 2026
Merged

feat(llm): bound generation length, not only the wait (#422)#439
jasonssdev merged 1 commit into
mainfrom
feat/422-bound-generation-length

Conversation

@jasonssdev

Copy link
Copy Markdown
Owner

Closes #422.

Unblocks the last open criterion of #379, the P0 validation gate: bounded cost cannot be measured honestly while a fraction of calls never return.

The problem

Extraction is bimodal — a generation either finishes fast or never terminates. Every successful run in the measured sweep finished under 60s (slowest 56.2s); 4 of 60 hit a 180s deadline and returned nothing. chat_timeout governs how long we wait; nothing governed how much the model may emit. So raising the deadline converts a 3-minute loss into a 10-minute one.

The fix

max_generation_tokens (default 8192), forwarded as options.num_predict at the OllamaClient.chat seam — the client seam, not per-verb, so curate, query, adjudicate, suggest-relations and contradictions are bounded too. Per-verb placement would repeat the exact drift defect #405's chat_timeout work was written to close.

When the knob is unset, options is omitted entirely and the request is byte-identical to before.

Why it raises instead of reporting

This is the design decision worth reading. extract_json_items returns [] on mid-JSON truncation — every fallback candidate requires a closing delimiter (parsing.py:23-40), so a truncated reply falls through to return [] at :95. That [] is structurally identical to "the model found nothing", and ExtractionReport(produced=0, retained=0) "renders no notice" per its own docstring.

So a bare cap would have traded today's loud failure (600s → OllamaUnavailableextraction_status: failed) for a silent one (fast → [] → nothing said). That is precisely the defect #381, #404 and #409 were each filed about.

Instead, chat() reads done_reason — present in every response and currently discarded at ollama.py:442 — and raises a new OllamaGenerationCapped(OllamaError) on "length". Because it subclasses OllamaError, every existing handler already covers it: extract_concept's docstring (concept.py:542) states any OllamaError-family exception propagates, and the CLI catches OllamaError for its Source-only fallback. No Protocol change, no stub churn, no client state, no heuristic.

An alternative that widened LLMBackend.chat from -> str was rejected: the Protocol is implemented structurally by every test double.

The default is calibrated, not guessed

Five extraction calls through the project's own _build_messages/_SYSTEM_PROMPT against local qwen3:8b on 17 KB real prose sources:

source eval_count done_reason
docs/architecture.md 4154 stop
docs/knowledge-object-model.md 1624 stop
docs/user-journey.md 962 stop
docs/architecture.md (repeat) 269 stop
docs/testing.md 107 stop

8192 is ~2x the largest legitimate completed reply. It is documented as a safety rail, not a quality-tuning knob — the distinction matters, and the codebase already has both kinds (_MAX_CANDIDATE_GROUPS = 50 is a rail; _MAX_OBJECTS_PER_SOURCE is a quality ceiling whose under-calibration at 5 discarded real material in 12/14 runs).

The same run reproduced #422's bimodality at the generation layer: docs/architecture.md produced 4154 tokens on one call and 269 on another — same document, same model, same prompt, a 15x spread.

Ollama's sentinels (0, -1, -2) are rejected in config validation. -1 means "unlimited" and would silently disable the rail.

Verified against a real backend

The unit tests inject urlopen, so they prove what the client sends — not that Ollama honours num_predict. That gap was closed outside the suite against local qwen3:8b: a 16-token ceiling raised OllamaGenerationCapped, an 8192 ceiling did not, and an unconfigured client did not.

pytest -q              -> 3726 passed in 165.43s
ruff check .           -> All checks passed!
ruff format --check .  -> 171 files already formatted
mypy . --strict        -> Success: no issues found in 171 source files

Review

Lineage review-f1c74eacac07e412, risk high (14 files, 687 lines) → canonical 4-lens review. 0 BLOCKER, 0 CRITICAL. All gates allow.

Known, accepted, and recorded rather than hidden:

  • WARNING (resilience) — the 8192 default was measured on extraction-shaped generations, and the client seam applies it to the other chat verbs too. Accepted: the knob is configurable and the failure is loud, not silent.
  • SUGGESTION (reliability) — the raise message interpolates the ceiling even when None. Unreachable from the CLI, where Config.max_generation_tokens is a non-optional int with a default. Follow-up.
  • A pre-existing all-or-nothing batch boundary in resolution/adjudication.py was observed but lies outside this candidate. Follow-up.

Extraction is bimodal: a generation either finishes fast or never terminates.
Every successful run in the measured sweep finished under 60s (slowest 56.2s),
while 4 of 60 hit a 180s deadline and returned nothing. Raising chat_timeout
converts a 3-minute loss into a 10-minute one; nothing governed how much the
model may emit.

- Add max_generation_tokens (default 8192), forwarded as options.num_predict
  at the OllamaClient.chat seam so every chat verb is bounded, not just
  extraction. When unset, options is omitted entirely and the request is
  byte-identical to before.
- Add OllamaGenerationCapped(OllamaError), raised when the response reports
  done_reason == length. A truncated reply is unusable -- extract_json_items
  returns [] on mid-JSON truncation -- so raising keeps the failure loud
  instead of degrading to a silent zero-object result. Subclassing OllamaError
  means every existing handler already covers it: extraction records the
  source as failed exactly as a hung call does today.
- Reject Ollama's num_predict sentinels (0, -1, -2) in config validation. -1
  means unlimited and would silently disable the rail this change installs.
- Wire both OllamaClient construction sites, pinned by an AST drift guard
  mirroring the existing chat_timeout one.

The default is calibrated, not guessed: five extraction calls through the
project's own prompt against qwen3:8b on 17KB sources produced eval_count of
4154, 1624, 962, 269 and 107, all done_reason stop. 8192 is ~2x the largest
legitimate reply. It is a safety rail, not a quality-tuning knob.

Closes #422
@jasonssdev
jasonssdev merged commit 3594523 into main Aug 6, 2026
6 checks passed
@jasonssdev
jasonssdev deleted the feat/422-bound-generation-length branch August 6, 2026 11:49
jasonssdev added a commit that referenced this pull request Aug 9, 2026
`openspec/changes/` held three folders containing only `exploration.md`:
`bound-generation-length` (#422), `reach-same-verdicts` (#427) and
`surface-merged-body-contradictions` (#409). All three issues shipped and
closed WITHOUT an SDD cycle -- #422 via PR #439, #427 via PR #438, #409
via PRs #442/#443 -- so these are abandoned exploration notes, not parked
work.

They are deleted rather than archived: every one of the 30+ entries under
`openspec/changes/archive/` is a completed SDD cycle carrying
proposal/design/tasks/specs plus an archive report, and none holds a
single file. Moving exploration-only stubs there would redefine the
archive from "finished cycle" to "folder I stopped using". Git history
keeps the notes if they are ever wanted.

`openspec/changes/` now holds only `archive/`.
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.

bug: extraction is bimodal — a call either finishes under a minute or never returns, and only the deadline bounds it

1 participant