LCORE-1569 LCORE-1570: token estimation + compaction core modules#1718
Conversation
Add tiktoken (>=0.8.0) to project dependencies. tiktoken is the tokenizer library used by OpenAI for the cl100k_base encoding family (GPT-3.5, GPT-4, GPT-4o). It runs entirely on CPU with no network calls, and tokenization of a 10K-token conversation typically takes under 5 ms. This dependency is the foundation for pre-LLM-call token estimation which the conversation compaction trigger needs in order to decide when older conversation turns must be summarized before the request exceeds the model's context window. The estimator module itself lands in a follow-up commit. uv.lock is regenerated to record the resolved tiktoken version and its transitive dependencies (regex).
Add a `context_windows` field to `InferenceConfiguration` that maps a fully-qualified model identifier (e.g., "openai/gpt-4o-mini") to its context window size in tokens. The conversation compaction trigger needs this map to know how many tokens a given model can accept before the request must be rejected or compacted. Storing the map on `InferenceConfiguration` (rather than on a new top-level field) co-locates it with the other inference defaults — the same section that already names the default model. Type is `dict[str, PositiveInt]`, defaulting to an empty dict. Pydantic's `PositiveInt` rejects zero and negative values at parse time, so a misconfiguration surfaces immediately rather than at trigger time. A model absent from the map has no registered window — callers decide whether to skip the token-based trigger or fall back to a hard-coded default. Tests cover the empty default, a populated map, and the non-positive / negative rejection paths.
Add the token-estimation module that the conversation compaction
trigger depends on. The module wraps tiktoken with three concerns:
* estimate_tokens(text, encoding_name) — single-text count.
* estimate_conversation_tokens(messages, system_prompt, encoding_name)
— sum across a chat history. Accepts both Llama Stack
conversation-item objects (.type/.role/.content) and OpenAI-style
{"role", "content"} dicts in the same list; non-message items are
skipped. The duck-typed shape avoids forcing the caller to adapt
whatever its local code path already produces.
* get_context_window(model, inference_config) — public retrieval
helper for the per-model context-window registry added on
InferenceConfiguration. Returns None when the model is not
configured, so the caller — not this module — decides on a
fallback policy.
tiktoken encodings are memoized at module level via lru_cache so the
BPE merge tables only load once per encoding per process.
Tests cover:
* The JIRA acceptance criterion (positive integer for "hello world").
* Hardcoded reference counts matching the cl100k_base tokenizer for
several known phrases (interpretation of the "within 5% of actual"
AC: tiktoken is the reference for OpenAI-family models, so the
test compares the public surface to a direct tiktoken call on the
same text and asserts the deviation is at most 5% — establishing
that the wrapper introduces no error of its own).
* Both message shapes (objects and dicts) and a mixed list.
* The non-message item skip path.
* Context-window retrieval: known model, unknown model, and empty
map all behave as documented.
Add the ConversationSummary Pydantic model. One instance represents a single compaction-produced summary chunk — additive compaction (decision 2 of the spike) keeps every chunk as a separate record and only collapses them via the recursive-resummarize fallback when their total token count itself approaches the context window. Fields, all required: * summary_text — the natural-language summary that becomes input context for subsequent requests. * summarized_through_turn (NonNegativeInt) — running total of conversation items consumed by this and all preceding summaries, so the caller can advance the partition boundary on the next compaction without re-counting from zero. * token_count (PositiveInt) — tokens in summary_text. Pre-computed so the recursive fallback can decide when the cumulative summary size approaches the limit without re-tokenizing. * created_at — ISO 8601 string. Match the cache schema's existing convention (strings, not datetime objects) so persistence in LCORE-1571 is straightforward across SQLite, Postgres, and in-memory backends. * model_used — fully-qualified model identifier. Preserved for audit and for diagnostics when summary quality varies by model. The model lives in src/models/compaction.py (not in cache_entry.py) so that the cache extension landing in LCORE-1571 can import it as a separate concern from CacheEntry. Tests cover the happy path, NonNegativeInt and PositiveInt boundary behavior, and that every required field is in fact required.
Add the configuration class that controls when conversation compaction triggers and how much recent context is kept verbatim. Disabled by default so the addition is a pure no-op until an operator opts in. Fields and defaults match the spec doc exactly: * enabled: bool — false. The master switch; everything below is inert when this is off. * threshold_ratio: float — 0.7. Trigger when estimated input tokens exceed this fraction of the configured context window. field_validator clamps the value to [0, 1] inclusive so a misconfiguration surfaces at parse time, not at first request. * token_floor: NonNegativeInt — 4096. Floor below which compaction cannot trigger, regardless of threshold_ratio. Protects very small context windows from spurious triggers. Zero is allowed (caller may want pure ratio-based triggering). * buffer_turns: NonNegativeInt — 4. Initial size of the recent-turns buffer the runtime keeps verbatim. The runtime applies a degrading guard (4 -> 3 -> 2 -> 1 -> 0) when these turns themselves exceed the available budget, so zero is a legitimate fallback state. * buffer_max_ratio: float — 0.3. Hard cap on the fraction of the context window the buffer zone may occupy. The configuration is wired onto the root Configuration class as the `compaction` field with a default_factory, so existing deployments that have not configured a `compaction:` block in YAML continue to parse unchanged and silently inherit the disabled-by-default behavior. Tests cover every field's default, the boundary values for both ratio validators, NonNegativeInt's negative-rejection on token_floor and buffer_turns, the extra='forbid' inherited from ConfigurationBase, and that the root Configuration declares the compaction field with the correct default-factory. Tests verify the default-factory inspection-style to avoid having to construct a full Configuration with all its other required sub-configurations.
…uard Add the partitioning step of conversation compaction. Given an ordered list of conversation items, return (old, recent) where "old" is the chunk that will be summarized and "recent" is the buffer kept verbatim. The buffer is sized in turn pairs (one user message + one assistant message). partition_conversation applies the *degrading guard* of spike decision 9: start at buffer_turns pairs and shrink one pair at a time until the buffer's estimated token count is at or below the available budget. If even a single pair would not fit, the buffer degrades all the way to zero and the entire conversation becomes "old". This handles the pathological case in the spec where a few very large tool-result turns would themselves overflow the context window. Non-message items (function calls, tool results) are carried along with whichever chunk their bracketing messages land in — the partition boundary is always the start of a user/assistant pair, so tool-call control flow is never orphaned mid-turn. The module also exposes the three small duck-type helpers: is_message_item, extract_message_text, format_conversation_for_summary. These are written fresh (not ported from the prior PoC) to keep the production module independent of design-divergent code, but the behavior is shape-compatible with the PoC where the shape work was correct. format_conversation_for_summary will be used by the summarization step in the next commit. The function takes a token estimator dependency by reaching into utils.token_estimator inside the function body to avoid the prospective top-level circular import once LCORE-1572 wires the request-flow integration that has to import both modules. Tests cover the JIRA acceptance criterion (20+ turn conversation yields non-empty old AND recent partitions), the full degrading guard ladder (generous budget keeps 4 pairs; tight budget degrades to 1 pair; impossibly tight budget degrades to 0), edge cases (empty conversation, fewer-than-buffer conversation), the disjoint-and-covering invariant, and the duck-type helpers under both Llama Stack and OpenAI shapes.
Add the additive summarization primitive of compaction (spike
decision 2). One call to summarize_chunk produces one
ConversationSummary; the caller accumulates them. The function does
not mutate the conversation, does not write to the cache, does not
acquire any lock — those side effects live in LCORE-1571 / 1572.
SUMMARIZATION_PROMPT is a module constant carrying the five
preservation directives required by the spec doc and the JIRA AC
(user question + environment; errors + commands + outcomes;
decisions + rationale; resolved vs open; user-vs-assistant
attribution). It is scoped to Red Hat product support so the LLM
keeps the operator-relevant signal that a generic summarizer
typically prunes. Exposing it as a constant lets tests assert
directive presence and centralizes future tuning.
summarize_chunk takes summarized_through_turn from the caller
rather than deriving it from old_items alone — additive compaction
spans multiple invocations whose chunk boundaries do not start at
zero, and the running total is the caller's bookkeeping.
The function uses store=False on the responses.create call. The
summarization request is a one-shot; its output is not stored as a
conversation item by Llama Stack. Injecting the produced summary
into the user's conversation under some marker scheme is LCORE-1572's
responsibility — this module just produces the value.
A LLM call that returns no extractable text raises ValueError
rather than constructing an empty ConversationSummary. The
PositiveInt token_count would refuse a zero-token summary anyway,
and a useless context wrapper is worse than a clear error.
Tests cover:
* All five JIRA-required preservation directives present in the
prompt constant.
* Prompt body construction concatenates prompt and transcript.
* Happy path: ConversationSummary is fully populated; token_count
is positive; model_used is preserved; created_at is non-empty.
* The LLM call uses store=False and stream=False.
* Empty LLM response raises ValueError.
* Additive AC ("second compaction appends a new summary chunk,
does not re-summarize the first"): two sequential calls produce
two independent records and the second call's prompt does not
include the first chunk.
Tests for partition_conversation now also run alongside the new
ones in the same module.
Add recursively_resummarize — the spec's escape hatch when additive compaction's per-chunk summaries themselves grow large enough to threaten the context window. Without it, the additive design grows its summary set unboundedly across a long-running conversation; the fold collapses every chunk into a single ConversationSummary. The function takes a list of existing summaries (oldest first) and calls the LLM once with a prompt that lists each chunk in order and asks for a combined summary preserving the same five Red Hat support directives. The new summary inherits summarized_through_turn from the most recent input — no new conversation turns were summarized by this call; the running total has not advanced. Folding a single summary is a no-op and raises ValueError so a caller that only has one chunk does not silently make an unnecessary LLM call. Same for an empty list. A separate RECURSIVE_RESUMMARIZATION_PROMPT constant keeps the fold's prompt distinct from the additive prompt (the input is qualitatively different — already-summarized text rather than raw transcript — though the preservation directives are the same). The function intentionally does not decide *when* to fold. The caller (LCORE-1572) compares the cumulative summary token count against some configured fraction of the context window and invokes this only at the threshold. Tests cover: * Multi-summary fold returns one ConversationSummary inheriting the most recent through-turn count. * Prompt body contains every input summary's text and the fallback prompt constant. * store=False / stream=False on the LLM call. * Single-summary and empty-list inputs raise ValueError. * Empty LLM response raises ValueError (mirrors summarize_chunk).
Whitespace-only reflow produced by `uv run make format` (black with line length 88). Touches the four files added or extended in the preceding 1569/1570 commits — the formatter prefers single-line exception-construction calls and tighter argument layout where the arguments fit under the line limit, which my hand-written layout did not match. No semantic change. Kept as one commit at the tip rather than folded back into each per-ticket commit so the per-ticket diffs remain reviewable on their own without an interleaved cosmetic churn.
Fix-ups discovered by `uv run make verify`. No behavior change. * utils/compaction: hoist `from utils.token_estimator import ...` to the module top. The original local imports were a paranoid hedge against a hypothetical circular import that does not actually exist (token_estimator does not import compaction). Top-level imports also let pylint's import-outside-toplevel rule pass cleanly. * utils/compaction: switch `timezone.utc` to the Python 3.13 `datetime.UTC` alias per ruff UP017. * tests: `import tiktoken` at module top instead of inside a test method (test_token_estimator.py). * tests: `# pylint: disable=too-few-public-methods` at module top for the test files that use one-attribute stand-in classes for message and content-part fixtures. These are intentional duck-type stubs, not real abstractions, and adding methods just to satisfy a count would obscure the intent. * tests: `Configuration.model_fields.get(...)` instead of `"compaction" in Configuration.model_fields`. The dict accessor pylint cannot infer; the .get() form sidesteps the unsupported-membership-test / unsubscriptable-object false positives without weakening the assertion. * models/config: `compaction` Field's `default_factory` now constructs CompactionConfiguration with every field passed explicitly. Matches the established pattern used by `conversation_cache` and `quota_handlers` on the same Configuration class, and resolves pyright reportCallIssue: the bare class form trips pyright's "constructor requires positional args" check because pyright treats the kwargs-with-defaults init as zero-positional but more-than-zero-required-keyword.
…elds
The five tests in test_dump_configuration.py compare
Configuration.model_dump() output against hard-coded expected
dictionaries. Adding `context_windows` to InferenceConfiguration
(LCORE-1569) and `compaction` to the root Configuration
(LCORE-1570) shifts the dump shape, so the expected fixtures need
matching entries.
Update every dump-test expected dict to include:
* `inference.context_windows: {}` — the empty-by-default per-model
context-window registry.
* `compaction: { enabled: False, threshold_ratio: 0.7,
token_floor: 4096, buffer_turns: 4, buffer_max_ratio: 0.3 }` —
the defaults baked into the root Configuration's default_factory.
Fixture-only change. No production-code change. Caught by running
the full unit-test suite (`uv run pytest tests/unit/`) — the per-file
test runs I did originally only covered the new tests, which missed
this snapshot-shape coupling.
WalkthroughThis PR introduces conversation compaction infrastructure for managing long-running conversations within token budgets. It adds token estimation utilities, LLM-powered summarization with configurable partitioning, and integrates configuration throughout the system with comprehensive test coverage. ChangesConversation Compaction Pipeline
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
…elds The CI `spectral` job verifies that the committed docs/openapi.json matches what `scripts/generate_openapi_schema.py` produces from the live Pydantic models. The new `compaction` field on Configuration (LCORE-1570) and the new `context_windows` field on InferenceConfiguration (LCORE-1569) added schema entries that were missing from the committed JSON, so the CI diff check failed. Regenerated via `uv run scripts/generate_openapi_schema.py docs/openapi.json`. The diff only adds the new schemas (CompactionConfiguration definition, compaction property on Configuration, context_windows property on InferenceConfiguration); no existing schema is altered. Caught only after pushing — `make verify` runs the spectral ruleset against docs/openapi.json but does not run the matches- generator check, so the local pre-push pass was clean.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/compaction.py`:
- Around line 63-117: The two duplicated helpers (is_message_item /
extract_message_text in compaction.py vs. _is_message / _extract_message_text in
token_estimator.py) must be consolidated to a single canonical implementation
and imported where needed: pick one source (prefer making token_estimator's
helpers public or move both into a new shared module like
utils/message_helpers), remove the duplicate definitions from compaction.py,
update imports in compaction.extract_message_text and any callers to use the
consolidated functions (e.g., is_message_item or _is_message ->
message_helpers.is_message and extract_message_text/_extract_message_text ->
message_helpers.extract_message_text), and ensure the chosen behavior for
unknown content types (decide whether to keep the str(content) fallback) is
applied consistently and reflected in tests.
- Around line 325-337: The prompt currently bundles system-style directives and
the transcript into a single input for client.responses.create; update
summarize_chunk (which uses _build_summarization_prompt_body) and
recursively_resummarize to pass system instructions via the responses API's
instructions= parameter and keep the conversation/transcript in input= (user
content) so system directives are treated as a separate system message; modify
the client.responses.create calls in those functions to use instructions=<system
string> and input=<user transcript string> (or array as the SDK expects),
preserving model, stream, and store options, and ensure any helper that builds
the combined prompt (_build_summarization_prompt_body) is split into a
system-instructions string and a user-input string accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5b2735b1-4485-47bb-b088-87d0185b755d
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
pyproject.tomlsrc/models/compaction.pysrc/models/config.pysrc/utils/compaction.pysrc/utils/token_estimator.pytests/unit/models/config/test_compaction_configuration.pytests/unit/models/config/test_dump_configuration.pytests/unit/models/config/test_inference_configuration.pytests/unit/models/test_compaction.pytests/unit/utils/test_compaction.pytests/unit/utils/test_token_estimator.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build-pr
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Llama Stack imports: Usefrom llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules must start with descriptive docstrings explaining purpose
Uselogger = get_logger(__name__)fromlog.pyfor module logging
All functions must have complete type annotations for parameters and return types, use modern syntax (str | int), and include descriptive docstrings
Use snake_case with descriptive, action-oriented names for functions (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying function parameters
Useasync deffor I/O operations and external API calls
Use standard log levels with clear purposes:debug()for diagnostic info,info()for program execution,warning()for unexpected events,error()for serious problems
All classes must have descriptive docstrings explaining purpose and use PascalCase with standard suffixes:Configuration,Error/Exception,Resolver,Interface
Abstract classes must use ABC with@abstractmethoddecorators
Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes
Files:
src/models/compaction.pysrc/models/config.pysrc/utils/token_estimator.pysrc/utils/compaction.py
src/models/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Pydantic models must use
@model_validatorand@field_validatorfor validation and complete type annotations for all attributes, avoidingAnytype
Files:
src/models/compaction.pysrc/models/config.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Use pytest for all unit and integration tests; do not use unittest
Usepytest.mark.asynciomarker for async tests
Files:
tests/unit/models/config/test_compaction_configuration.pytests/unit/models/test_compaction.pytests/unit/models/config/test_dump_configuration.pytests/unit/models/config/test_inference_configuration.pytests/unit/utils/test_token_estimator.pytests/unit/utils/test_compaction.py
🧠 Learnings (2)
📚 Learning: 2026-01-12T10:58:40.230Z
Learnt from: blublinsky
Repo: lightspeed-core/lightspeed-stack PR: 972
File: src/models/config.py:459-513
Timestamp: 2026-01-12T10:58:40.230Z
Learning: In lightspeed-core/lightspeed-stack, for Python files under src/models, when a user claims a fix is done but the issue persists, verify the current code state before accepting the fix. Steps: review the diff, fetch the latest changes, run relevant tests, reproduce the issue, search the codebase for lingering references to the original problem, confirm the fix is applied and not undone by subsequent commits, and validate with local checks to ensure the issue is resolved.
Applied to files:
src/models/compaction.pysrc/models/config.py
📚 Learning: 2026-02-25T07:46:33.545Z
Learnt from: asimurka
Repo: lightspeed-core/lightspeed-stack PR: 1211
File: src/models/responses.py:8-16
Timestamp: 2026-02-25T07:46:33.545Z
Learning: In the Python codebase, requests.py should use OpenAIResponseInputTool as Tool while responses.py uses OpenAIResponseTool as Tool. This difference is intentional due to differing schemas for input vs output tools in llama-stack-api. Apply this distinction consistently to other models under src/models (e.g., ensure request-related tools use the InputTool variant and response-related tools use the ResponseTool variant). If adding new tools, choose the corresponding InputTool or Tool class based on whether the tool represents input or output, and document the rationale in code comments.
Applied to files:
src/models/compaction.pysrc/models/config.py
🪛 GitHub Actions: Unit tests / 0_unit_tests (3.12).txt
src/models/config.py
[warning] 948-948: FutureWarning: Possible nested set at position 1 (re.compile(self.value))
🪛 GitHub Actions: Unit tests / unit_tests (3.12)
src/models/config.py
[warning] 948-948: FutureWarning: Possible nested set at position 1 (from re.compile(self.value)).
🔇 Additional comments (19)
src/models/config.py (4)
948-948: Note: Pipeline warning is unrelated to this PR.The
FutureWarningabout a possible nested set in the regex pattern occurs in existing code (JwtRoleRule.check_regex_pattern) and predates this PR's changes. This warning should be addressed separately but is not a blocker for the compaction configuration changes.
1430-1439: LGTM!The
context_windowsfield is well-designed with appropriate types (dict[str, PositiveInt]), a clear description, and a default factory. The use ofPositiveIntcorrectly enforces non-negative window sizes.
1463-1535: LGTM!Excellent
CompactionConfigurationdesign with comprehensive documentation, appropriate field types, and validators that correctly enforce inclusive[0.0, 1.0]bounds on ratio fields. The use ofNonNegativeIntfortoken_floorandbuffer_turnsallows zero values as intended.
2009-2022: LGTM!The
compactionfield is properly integrated into the rootConfigurationwith a default factory that produces a disabled instance, ensuring backward compatibility with existing configurations.pyproject.toml (1)
79-80: LGTM!The tiktoken dependency addition is clean, properly commented with the ticket reference, and uses an appropriate minimum version constraint.
src/models/compaction.py (1)
1-70: LGTM!Clean Pydantic model with complete type annotations, descriptive docstrings, and appropriate field constraints. The choice to use
strforcreated_atis well-documented as aligning with cache schema conventions.tests/unit/models/test_compaction.py (1)
1-76: LGTM!Comprehensive test coverage for the
ConversationSummarymodel, including validation of field constraints (NonNegativeIntvsPositiveInt), required field enforcement, and round-trip construction. Tests follow pytest conventions correctly.tests/unit/models/config/test_inference_configuration.py (1)
61-92: LGTM!The new tests comprehensively validate the
context_windowsfield behavior: default empty dict, accepting valid model-to-size mappings, and rejecting non-positive values per thePositiveIntconstraint.tests/unit/models/config/test_dump_configuration.py (1)
171-171: LGTM!Consistent updates to all configuration dump test expectations. The new
context_windowsandcompactionfields are added uniformly across all test cases with correct default values.Also applies to: 194-200, 522-522, 545-551, 765-765, 788-794, 988-988, 1011-1017, 1201-1201, 1224-1230
tests/unit/models/config/test_compaction_configuration.py (1)
1-111: LGTM!Excellent test coverage for
CompactionConfiguration, including default values, boundary conditions for ratio fields,NonNegativeIntconstraints,extra='forbid'enforcement, and verification of proper wiring into the rootConfigurationmodel with a default factory.src/utils/token_estimator.py (7)
34-53: LGTM!The
_get_encodingfunction correctly uses@lru_cacheto memoize encoding instances, preventing repeated loading of BPE tables. Themaxsize=8is appropriate for the number of tiktoken encodings.
56-76: LGTM!Clean implementation with early return for empty strings and proper use of the cached encoding. Type annotations and docstring are complete.
79-119: LGTM!Robust message text extraction that handles multiple content shapes (string, list of parts with
.text, list of dicts with"text"key) with appropriate fallbacks. The defensive approach of returning empty string for None and usingstr(...)as a final fallback is sound.
122-130: LGTM!Simple and effective duck-typing check that recognizes both OpenAI-style dicts and Llama Stack conversation items. The implementation correctly uses
getattrwith a default to avoidAttributeError.
133-167: LGTM!Clean implementation that correctly sums token counts across system prompt and messages, skipping non-message items as intended. The separation of message identification (
_is_message) and text extraction (_extract_message_text) is well-designed.
170-188: LGTM!Simple and correct lookup from the
context_windowsconfiguration dict. ReturningNonefor missing models allows callers to decide their fallback behavior, which aligns with the design mentioned in the PR description.
20-27: Tiktoken API usage is correct. Bothtiktoken.get_encoding(name)andencoding.encode(text)are supported methods in tiktoken>=0.8.0 and compatible with the version constraint.tests/unit/utils/test_token_estimator.py (1)
1-274: LGTM — comprehensive coverage of estimator and helpers.The test suite is well-organized into focused classes, covers empty/non-empty inputs, both Llama-Stack and OpenAI dict shapes, mixed lists, skipped non-message items, and the three context-window outcomes. Use of
pytest(nounittest) and class/method docstrings is consistent with the repo's testing conventions.tests/unit/utils/test_compaction.py (1)
1-631: LGTM — thorough coverage of partition, prompt, and async summarization paths.Particularly good points:
- The degrading-guard tests exercise the full ladder: generous budget, tight budget that forces shrinking to one pair, infeasible budget that drives buffer to zero,
buffer_turns=0, empty conversation, and short conversations.test_additive_second_call_does_not_resummarize_firstdirectly encodes the JIRA AC by asserting the second call's prompt does not contain the first chunk's content._make_summary_responsecorrectly mirrors the Responses API shape (output → content parts →.text), and bothstore=False/stream=Falseare explicitly verified forsummarize_chunkandrecursively_resummarize.- Both error branches (
ValueErroron empty extracted text,ValueErrorfor fewer than 2 summaries) are covered, including the edge case of an empty list.
Rename `_is_message` -> `is_message_item` and `_extract_message_text` -> `extract_message_text`. CodeRabbit flagged duplication between these helpers and the same-shape pair in utils/compaction.py. The token_estimator helpers were marked private precisely because compaction.py kept its own copies; the two copies disagree on nothing today but each future fix has to land twice, which is the wrong amount of work for a duck-type adapter over an externally-defined Llama Stack item shape. Promoting them here (and consuming them from compaction.py in the next commit) gives a single canonical implementation. No behavior change — the function bodies are unchanged. Tests in test_token_estimator.py are updated to the new names, including the docstrings that previously called them "the private ..." check / extractor. ruff fixed the import-order on its own.
…prompt
Two related CodeRabbit findings, both about utils/compaction.py.
1. Drop the local is_message_item / extract_message_text /
format_conversation_for_summary copies and import the first
two from utils/token_estimator (now public — see prior commit).
format_conversation_for_summary stays here because it composes
the two and is specific to the compaction transcript shape.
Removes ~55 lines of duplicated code.
2. Switch summarize_chunk and recursively_resummarize from
`input=<directives + transcript>` to
`instructions=<directives>, input=<transcript>`. Three reasons:
a. Prompt-injection resistance: a summarized user message
containing "ignore the above instructions" no longer sits
in the same channel as the directives.
b. The Responses API treats `instructions` as a system-style
directive — adherence is empirically better when the
directives are in their own channel rather than appended to
the user turn.
c. Codebase convention: utils/responses.py:get_topic_summary
already calls responses.create this way; matching it makes
the compaction module read as a peer rather than an
exception.
The llama-stack-client 0.6.0 SDK installed in this project
exposes `instructions: Optional[str]` as a typed kwarg on
responses.create (verified at
.venv/.../llama_stack_client/resources/responses/responses.py:97).
_build_summarization_prompt_body is removed — it existed solely
to produce the now-obsolete combined-channel string. The
corresponding test class TestBuildSummarizationPromptBody is
removed; the summarize_chunk and recursively_resummarize tests
now assert on the `instructions` kwarg directly, including the
inverse assertion that the prompt is NOT in `input`.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/utils/test_compaction.py`:
- Around line 11-21: Update the test imports to pull message helper utilities
directly from their defining module: replace importing extract_message_text and
is_message_item from utils.compaction with direct imports from
utils.token_estimator (i.e., import extract_message_text, is_message_item from
utils.token_estimator) so the test references the original source of these
helpers and avoids transitive coupling via compaction.py.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ba7be93d-21ae-4fc9-bc4f-d5caeeef40e6
📒 Files selected for processing (5)
docs/openapi.jsonsrc/utils/compaction.pysrc/utils/token_estimator.pytests/unit/utils/test_compaction.pytests/unit/utils/test_token_estimator.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 2
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Llama Stack imports: Usefrom llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules must start with descriptive docstrings explaining purpose
Uselogger = get_logger(__name__)fromlog.pyfor module logging
All functions must have complete type annotations for parameters and return types, use modern syntax (str | int), and include descriptive docstrings
Use snake_case with descriptive, action-oriented names for functions (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying function parameters
Useasync deffor I/O operations and external API calls
Use standard log levels with clear purposes:debug()for diagnostic info,info()for program execution,warning()for unexpected events,error()for serious problems
All classes must have descriptive docstrings explaining purpose and use PascalCase with standard suffixes:Configuration,Error/Exception,Resolver,Interface
Abstract classes must use ABC with@abstractmethoddecorators
Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes
Files:
src/utils/token_estimator.pysrc/utils/compaction.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Use pytest for all unit and integration tests; do not use unittest
Usepytest.mark.asynciomarker for async tests
Files:
tests/unit/utils/test_token_estimator.pytests/unit/utils/test_compaction.py
🪛 GitHub Actions: OpenAPI (Spectral) / 0_spectral.txt
docs/openapi.json
[error] 1-1: docs/openapi.json is out of date. diff against /tmp/openapi-generated.json failed. Regenerate with: uv run python scripts/generate_openapi_schema.py docs/openapi.json
🪛 GitHub Actions: OpenAPI (Spectral) / spectral
docs/openapi.json
[error] 1-1: CI validation failed: docs/openapi.json is out of date (generated schema differs from /tmp/openapi-generated.json). Regenerate with: 'uv run scripts/generate_openapi_schema.py docs/openapi.json'.
🔇 Additional comments (5)
docs/openapi.json (1)
11901-11940: ⚡ Quick win[Your rewritten review comment text here]
[Exactly ONE classification tag]src/utils/token_estimator.py (1)
1-188: Excellent implementation of token estimation utilities!This module is well-structured with comprehensive docstrings, proper type annotations, and efficient use of
lru_cachefor encoding memoization. The duck-typed message handling correctly supports both Llama Stack conversation items and OpenAI-style dictionaries, making it flexible for various code paths. The token counting logic is sound and the context window lookup provides a clean interface for model-specific configuration.tests/unit/utils/test_token_estimator.py (1)
1-274: Comprehensive test coverage with excellent edge case handling!The test suite thoroughly validates token estimation behavior including empty strings, known token counts, encoding validation, duck-typed message shapes, conversation-level estimation, and the 5% accuracy requirement from JIRA ACs. The use of helper classes to mock Llama Stack shapes is clean, and the direct tiktoken reference comparison provides strong validation of the estimator's accuracy.
src/utils/compaction.py (1)
1-396: Excellent implementation with past review feedback fully addressed!The compaction utilities are well-designed and all previous review comments have been resolved:
✅ Duplication eliminated:
is_message_itemandextract_message_textare now imported fromutils.token_estimator(lines 37-42), establishing a single canonical source for message-shape helpers.✅ Instructions/input separation: Both
summarize_chunk(lines 264-275) andrecursively_resummarize(lines 376-383) correctly useinstructions=for system-style directives andinput=for user content, protecting against prompt injection and matching codebase conventions.The module is pure (no side effects), well-documented, and the degrading buffer logic in
partition_conversationcorrectly handles edge cases down to zero buffer turns. Error handling for empty LLM responses is appropriate, and the additive summarization design is clean.tests/unit/utils/test_compaction.py (1)
1-613: Thorough test coverage validating all compaction behaviors!The test suite comprehensively validates the compaction utilities including message-shape duck-typing, transcript formatting, degrading buffer partitioning (including edge cases with zero buffer and empty conversations), prompt directive presence, async summarization with proper
store=False/stream=Falseparameters, additive behavior, recursive fold logic, and appropriate error handling for empty LLM outputs. The tests correctly verify the instructions/input separation pattern and validate JIRA acceptance criteria.
| from utils.compaction import ( | ||
| RECURSIVE_RESUMMARIZATION_PROMPT, | ||
| SUMMARIZATION_PROMPT, | ||
| _extract_response_text, | ||
| extract_message_text, | ||
| format_conversation_for_summary, | ||
| is_message_item, | ||
| partition_conversation, | ||
| recursively_resummarize, | ||
| summarize_chunk, | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider importing message helpers directly from their source module.
Lines 15-17 import extract_message_text and is_message_item from utils.compaction, but these functions are defined in utils.token_estimator and only re-exported through compaction.py's imports. While this works, importing them directly from token_estimator would make the dependency chain clearer and reduce transitive coupling, especially since:
- These helpers are already tested comprehensively in
test_token_estimator.py - They're utility functions, not core compaction logic
- Direct imports make the module relationships more explicit
♻️ Optional import clarification
from utils.compaction import (
RECURSIVE_RESUMMARIZATION_PROMPT,
SUMMARIZATION_PROMPT,
_extract_response_text,
- extract_message_text,
format_conversation_for_summary,
- is_message_item,
partition_conversation,
recursively_resummarize,
summarize_chunk,
)
from utils.token_estimator import (
DEFAULT_ENCODING_NAME,
estimate_conversation_tokens,
+ extract_message_text,
+ is_message_item,
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/utils/test_compaction.py` around lines 11 - 21, Update the test
imports to pull message helper utilities directly from their defining module:
replace importing extract_message_text and is_message_item from utils.compaction
with direct imports from utils.token_estimator (i.e., import
extract_message_text, is_message_item from utils.token_estimator) so the test
references the original source of these helpers and avoids transitive coupling
via compaction.py.
…-1570-token-estimation-and-compaction-module LCORE-1569 LCORE-1570: token estimation + compaction core modules
…-1570-token-estimation-and-compaction-module LCORE-1569 LCORE-1570: token estimation + compaction core modules
2 participants
LCORE-1569,LCORE-1570: token estimation + compaction core modules
Description
First two tickets of the conversation-compaction epic (LCORE-1631). Lands the two foundation modules that the rest of the epic builds on: pre-LLM-call token estimation (LCORE-1569) and the conversation-summarization core (LCORE-1570).
Both modules are isolated — they do not wire into the request flow, the conversation cache, or the response model. Those concerns belong to LCORE-1571 / LCORE-1572 / LCORE-1573 and are deliberately untouched.
LCORE-1569 — Add token estimation
tiktoken>=0.8.0dependency inpyproject.toml.src/utils/token_estimator.py:estimate_tokens(),estimate_conversation_tokens(),get_context_window(). Encoding is memoized at module level (lru_cache) so BPE tables load once per process.estimate_conversation_tokensaccepts both Llama Stack-shaped items (.type/.role/.content) and OpenAI-style{"role", "content"}dicts in the same list — the duck-typed shape avoids forcing callers to adapt whatever their local code path already produces.context_windows: dict[str, PositiveInt]field onInferenceConfiguration— per-model context-window registry the compaction trigger reads. Defaults to{}.PositiveIntrejects zero / negative window sizes at parse time.LCORE-1570 — Implement conversation summarization module
src/models/compaction.py:ConversationSummaryPydantic model (summary_text, summarized_through_turn, token_count, created_at, model_used).CompactionConfigurationPydantic model onmodels/config.py, wired onto the rootConfigurationas thecompactionfield with a default-factory. Defaults match the spec (disabled, 0.7 threshold ratio, 4096 token floor, 4 buffer turns, 0.3 max buffer ratio).threshold_ratioandbuffer_max_ratioare clamped to[0, 1]at parse time.src/utils/compaction.py:partition_conversation(...)— splits conversation items into(old, recent)with the degrading guard of spike decision 9: starts atbuffer_turnsturn pairs and shrinks to zero until the buffer fits the budget.summarize_chunk(...)— the additive primitive of spike decision 2. One LLM call → oneConversationSummary. Usesstore=False. Caller tracks the runningsummarized_through_turntotal across additive invocations.recursively_resummarize(...)— fallback that folds multiple existing summaries into one when their cumulative size approaches the context window.SUMMARIZATION_PROMPTandRECURSIVE_RESUMMARIZATION_PROMPTexposed as module constants. The former carries all five spec-mandated Red Hat support preservation directives (original question + environment; errors + commands + outcomes; decisions + rationale; resolved vs open; user/assistant attribution).Backwards-compatible: existing YAML configs without a
compaction:block continue to parse — the default-factory supplies the disabled-by-defaultCompactionConfiguration.Spec doc this PR implements against:
docs/design/conversation-compaction/conversation-compaction.md(LCORE-1314 spike).Type of change
Tools used to create PR
Related Tickets & Documents
Checklist before requesting a review
Testing
How to test
Check out the branch and sync the environment:
Run the new unit tests on their own:
Expected: 95 passed.
Run the full unit-test suite to confirm no regressions in unrelated tests (the dump-configuration snapshot tests required updating because the dump shape grew two fields):
Expected: 2251 passed, 1 skipped, 0 failed.
Run the full linter / type-checker chain:
Expected: exit 0. pylint 10.00/10.
Smoke-test token estimation:
Expected:
2.Smoke-test backwards-compatible YAML loading (no
compaction:block inlightspeed-stack.yaml):Expected: starts, logs
compaction=CompactionConfiguration(enabled=False, threshold_ratio=0.7, token_floor=4096, buffer_turns=4, buffer_max_ratio=0.3)andinference=InferenceConfiguration(... context_windows={})in the resolved configuration, writesconfiguration.json, exits 0.Evidence captured during PR preparation
Real-shape verification of
_extract_response_textThe summarization functions parse
client.responses.create(...).output[].content[].text. Verified against the real Llama Stack types (no mock):Out of scope for this PR (tracked elsewhere in LCORE-1631)
summarize_chunk/recursively_resummarize— meaningful only once the runtime wiring lands (LCORE-1572).ConversationSummary(LCORE-1571).context_statusresponse field (LCORE-1573).Summary by CodeRabbit
New Features
Documentation
Tests
Chores