Repository navigation
LCORE-3910: report the usage of the summarization and fold calls - #2867
Open
max-svistunov wants to merge 1 commit into
Open
max-svistunov wants to merge 1 commit into
max-svistunov wants to merge 1 commit into
Conversation
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (2)
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 |
Compaction summarizes older turns with an LLM call of its own, and folds the summaries with another. The provider bills both, and their usage was discarded: summarize_chunk() and recursively_resummarize() read the text of the response and nothing else. Both functions now take an optional count_call callback and hand it the model and the usage the provider reported for the call. They do so as soon as the response arrived and before they look at it, so a call that returned no text is reported as well. reported_usage() reads the usage from the response; a call without reported usage counts as a call with zero tokens. Without a callback nothing changes. This commit only reports. Nothing passes a callback yet: the metrics, the quota and the token counts told to the client follow in commits of their own. utils.responses.extract_token_usage() is not reused, because it records the LLM metrics under an endpoint path, and the summarizer does not know which endpoint it runs for. Tests: tests/unit/utils/test_compaction.py covers reported_usage() with and without usage, and that each of the two calls is reported, also when it returned no text. The existing tests of the module pass unchanged.
max-svistunov
force-pushed
the
lcore-3910-1-summarizer-usage
branch
from
October 7, 2026 22:50
3078b97 to
8c4445a
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
LCORE-3910, the first of two PRs. Compaction summarizes older turns with an LLM call of its own (
summarize_chunk()), and folds the stored summaries into one with another (recursively_resummarize()). The provider bills both calls. Both functions read the text of the response and nothing else, so the usage the provider reported was lost there, before any metric, quota or token count could include it.Both functions now take an optional keyword-only
count_callcallback and call it with the model and the usage of the call, aTokenCounterwithllm_calls=1. They do so as soon as the response arrived and before they look at it. A call that returned no text makes the function raiseValueError, and it is reported all the same, because the provider billed it.reported_usage()readsusage.input_tokensandusage.output_tokensfrom the response, the fieldsOpenAIResponseObject.usagehas in ogx-client 1.2.5 (the version inuv.lock). A call the provider reported no usage for is reported as one call with zero tokens.Nothing changes for a running service with this PR alone. Nothing passes a callback yet: the two callers in
src/utils/conversation_compaction.pyare not touched. Without a callback the functions do what they did, andreported_usage()is not called. The 35 tests the module had pass unchanged.Why this is a PR of its own. #2797 held LCORE-3910 as one commit of 1.8k lines on top of #2796, and nobody has reviewed it. It is replaced by two PRs. This is the first;
src/utils/compaction.pyis the same file as in #2797. The second holds the rest as three commits:count_call, and the calls are recorded inls_llm_token_sent_total,ls_llm_token_received_totalandls_llm_calls_totalunder the endpoint of the request./v1/query,/v1/streaming_queryand/v1/responsescharge each call to the quota of the user./v1/queryand/v1/streaming_queryinclude the calls in theinput_tokens/output_tokensthey report, and the user guide and the design document say who pays for the calls.This PR does not close LCORE-3910. The second PR does.
Relation to the other open PRs. The branch starts from
mainand does not depend on #2796 (LCORE-3908). The two PRs have no file in common, so they can be reviewed side by side and merged in either order. The second PR needs both merged first: it is built on top of #2796, because it changes the same lines of the streaming and A2A call sites, and on this PR. #2167 (LCORE-1347) movessrc/utils/compaction.pytosrc/lightspeed_stack/utils/and rewrites the imports of both files of this PR, in the lines where it adds its own: expect a conflict there in whichever merges second.Why not
extract_token_usage().utils.responses.extract_token_usage()already turns a usage object into aTokenCounter. It also records the LLM metrics under an endpoint path, and the summarizer does not know which endpoint it runs for.utils/compaction.pyis kept pure on purpose: its docstring says it does not touch conversation state, the cache or locks. Metrics and quota stay out of it in the same way. It reports each call, and the caller decides what to do with the usage.On the ticket text. The ticket lists as its first symptom that
ConversationSummary.token_countis a tiktoken count of the summary text and not the usage the model reported. That field is left as it is, here and in the second PR. It is the size of the summary in the context window, and_maybe_persist_fold()adds these sizes up to decide when the summaries are folded. The usage of the call is another quantity, so it is reported next to the field and does not replace it. The two decisions the ticket asks for (quota, reported token counts) are not part of this PR; they come with parts 3 and 4.Known limits.
summarize_chunk()now has six arguments, one more than pylint allows, and gets atoo-many-argumentsdisable.docs/user_doc/conversation_compaction.md). That stays untrue until the second PR. No documentation changes here.src/utils/compaction.pyCallCounter,reported_usage();summarize_chunk()andrecursively_resummarize()takecount_calland report their call (+45 / -2)tests/unit/utils/test_compaction.pyTestCallUsage, 7 cases (+98 / -1)Type of change
pyproject.toml+uv.lock]requirements.*.txtfor Konflux]Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Manual verification. No live run was made for this PR: nothing passes the callback yet, so a running service makes the same calls and gives the same responses as on
main. The commit of LCORE-3910: count the summarization calls in quota, token counts and metrics #2797, with the samesrc/utils/compaction.py, was verified live on 2026-09-28; the run is in its description.Run the tests of the change:
The new cases are in
TestCallUsage:reported_usage()with usage and without it (no usage object, counts that areNone, no counts),summarize_chunk()reports its call, and each of the two functions reports a call that returned no text. The fold has the no-text case only, because the same line reports in both cases.Check that they fail without the change in
src/. The test module does not import:With the two
count_call(model, reported_usage(response))lines replaced bypass:With the report moved behind the empty-text check, the no-text case of that function fails.