[Feature]: add context editing trigger mechanism#209
[Feature]: add context editing trigger mechanism#209riturajFi wants to merge 13 commits intomemodb-io:devfrom
Conversation
slyt3
left a comment
slyt3
left a comment
There was a problem hiding this comment.
lgtm, just minor optimization would be nice, but it doesnt block functionality, but yeah it could improve performence and maintability. You can ignor this issues if @gusye1234
| originalTokens, err = tokenizer.CountMessagePartsTokens(ctx, out.Items) | ||
| // Return error if token counting fails. | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to count tokens for auto-trim: %w", err) | ||
| } |
There was a problem hiding this comment.
token counting run on EVERY get_message call when auto_trim config exsists, wont it will be expensive for a large message sets?
| // Count tokens after auto-trim to estimate removed tokens. | ||
| editedTokens, err := tokenizer.CountMessagePartsTokens(ctx, out.Items) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to count tokens after auto-trim: %w", err) | ||
| } |
There was a problem hiding this comment.
second token counting operation, should be consider caching or optimization?
| // Skip auto-trim when strategy is not supported in v0. | ||
| if in.AutoTrim.Strategy != "remove_tool_result" { | ||
| // Record skip reason for unsupported strategy. | ||
| reason := "unsupported_strategy" | ||
| out.AutoTrimSkipped = true | ||
| out.AutoTrimSkipReason = &reason |
There was a problem hiding this comment.
use registry pattern for extensibility
| // Return 400 when strategy is not the only allowed value. | ||
| if strat != "remove_tool_result" { | ||
| c.JSON(http.StatusBadRequest, serializer.ParamErr("invalid auto_trim_strategy", errors.New("strategy must be remove_tool_result"))) | ||
| return | ||
| } |
There was a problem hiding this comment.
same hardcoded validation in handle layer.
| originalTokens, err = tokenizer.CountMessagePartsTokens(ctx, out.Items) | ||
| // Return error if token counting fails. | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to count tokens for auto-trim: %w", err) |
There was a problem hiding this comment.
add error message it lacks session context for debugging.
same in line 535, not gonna comment same issuse just include in error message session ID for better debugging
There was a problem hiding this comment.
I would discuss about the design, which may be two direction:
- trigger one editing strategy at a time.
- trigger the whole editing strategies.
trigger one editing strategy at a time
like https://platform.claude.com/docs/en/build-with-claude/context-editing#advanced-configuration. The trigger field should be in the strategy itself, like:
edit_strategies=[
{"type": "token_limit", "params": {"limit_tokens": 20000}, "trigger": {"token_gt": 30000}},
{"type": "remove_tool_use")
...
]so that each editing method can be triggered independently, and that gives Acontext combinability. In this design, you don't need to add the new parameter in the same level with edit_strategies. you just need to add a general field to every strategy's parameter, and trigger them accordingly
trigger the whole editing strategies
or we can just use trigger to trigger the whole editing_strategies (which I prefer), like
edit_strategies=[
{"type": "token_limit", "params": {"limit_tokens": 20000},},
{"type": "remove_tool_use")
...
]
editing_trigger = {"token_gt": 30000}I think this is more reasonable, after all, when everyone designs editing, they definitely hope it takes effect together.
Overall, I think the current sdk design a bit stuck in the middle, making it very inconvenient to use: I can only choose one trigger and one editing strategy at a time.
| description="Estimated token reduction from auto-trim", | ||
| ) | ||
| # auto_trim_skipped shows whether auto-trim was skipped. | ||
| auto_trim_skipped: bool | None = Field( |
There was a problem hiding this comment.
what's the difference bettwen auto_trim_applied and auto_trim_skipped?
Aren't these two always opposite booleans?
riturajFi
force-pushed
the
feat/context_editing
branch
3 times, most recently
from
670b772 to
dd285a4
Compare
GenerQAQ
force-pushed
the
dev
branch
2 times, most recently
from
0ca4326 to
64e00b9
Compare
riturajFi
force-pushed
the
feat/context_editing
branch
2 times, most recently
from
185b2cc to
06a47e1
Compare
|
I would love to see to be added tests for editing_trigger validation, gating behavior, and confirm pin+trigger semantics |
riturajFi
force-pushed
the
feat/context_editing
branch
from
06a47e1 to
03b6e85
Compare
3 participants
Why we need this PR?
Describe your solution
Adds optional auto-trim to
GET /session/{session_id}/messagesusing a token threshold and theremove_tool_resultstrategy. The API validates query params, the service trims messages when the threshold is met and returns auto-trim metadata, and the Python SDK exposes both the new request params and response fields.Implementation Tasks
Please ensure your pull request meets the following requirements:
Impact Areas
Which part of Acontext would this feature affect?
Checklist
devbranch.