-
Couldn't load subscription status.
- Fork 1.6k
feat: summarize when tokenlimit #3227
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
|
convert to draft as this PR hasn't updated with the approach we discussed |
|
Context creator now preserves the system message and returns the remaining history strictly by Token-limit handling captures the full context, rolls back recent tool-call chains when |
|
Thanks for the thorough PR @fengju0213. I wonder if the new implementation would cause inconsistencies and fragmentation of the summarization logic already implemented in two places.
summary_result = agent.summarize(filename="meeting_notes")
# Returns: {"summary": str, "file_path": str, "status": str}This is mostly used to create workflow.md files of a session
|
thanks for reviewing! @hesamsheikh We can probably unify the naming after rolename later. For now, could you help review and summarize the current logic for handling the token limit? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @fengju0213!! Left some comments. Maybe should also consider adding a test case when SUMMARY_MAX_DEPTH is reached.
b1885f3 to
bc69be7
Compare
|
token counter context 1,000,000 token context/2 -> 500,000 token 10s compress -> summarized memeory ( 100,000 token) (1,000,000 -100,000) /2 +100,000 -> 550,000 compress -> summarized memeory ( 100,000 token) (1,000,000 -100,000-100,000)) /2 +100,000 +100,000 -> 600,000 need to set the minium value for content compression |
97c5376 to
bc69be7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @fengju0213. I have added a few comments. As @MuggleJinx mentioned, i also think a few more test cases are a good idea.
| summary_msg = BaseMessage.make_assistant_message( | ||
| role_name="Assistant", content=content | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here Assistant is used as role_name while in the ContextSummarizerToolkit USER role is used. Are you sure adding the summary as Assistant message is the best approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually,i haven't compared two method in detail,do you have any suggestion?
| # Add new summary | ||
| new_summary_msg = BaseMessage.make_assistant_message( | ||
| role_name="Assistant", | ||
| content=summary_with_prefix + " " + summary_status, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is summary_status added directly to the summary? seems like a big.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will fix,should be file_path
camel/agents/chat_agent.py
Outdated
| for msg in messages: | ||
| content = msg.get('content', '') | ||
| if isinstance(content, str) and content.startswith( | ||
| '[CAMEL_SUMMARY]' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think [CAMEL_SUMMARY] allone is a good semantic prefix for the agent's summary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about CONTEXT_SUMMARY
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to actually send this to the LLM it must be more semantic, explaining what this summary is and how to treat it. For example: "The following is a summary of our conversation from a previous session, ...."
| f"Performing full compression." | ||
| ) | ||
| # Summarize everything (including summaries) | ||
| summary = self.summarize(include_summaries=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's actually best we don't rely on the default prompt which is too generic. maybe mentioning why this summarization is happening would be a good idea. especially when using the ASSISTANT as the role the tone must be adjusted, and the default prompt doesn't do that.
I've ran a simple script to see how the summarization turns out.
CONTEXT BEFORE SUMMARIZATION (13 messages, 525 tokens)
Message 1 [SYSTEM]: You are a helpful assistant.
Message 2 [USER]: Hi! I'm working on a Python project and need help with data analysis.
Message 3 [ASSISTANT]: Hello! I'd be happy to help you with data analysis in Python...
Message 4 [USER]: I have a CSV file with customer sales data...
Message 5 [ASSISTANT]: Great! That's a common dataset structure. To analyze this data, we can use pandas...
Message 6 [USER]: Ok loaded it. Now I need to find the top 5 customers by total revenue.
Message 7 [ASSISTANT]: Perfect! To find the top 5 customers by revenue, we need to: 1. Calculate revenue...
Message 8 [USER]: That worked! But I noticed some customer IDs appear multiple times. Is that normal?
Message 9 [ASSISTANT]: Yes, that's completely normal! Each row represents a single transaction...
Message 10 [USER]: Makes sense. Can you also help me create a bar chart of the top 5 customers?
Message 11 [ASSISTANT]: Absolutely! We can use matplotlib for that...
Message 12 [USER]: Perfect! One more thing - how do I save this chart to a file?
Message 13 [ASSISTANT]: You can save the plot using plt.savefig() before plt.show()...
CONTEXT AFTER SUMMARIZATION (3 messages, 228 tokens)
Message 1 [SYSTEM]:
You are a helpful assistant.
Message 2 [ASSISTANT]:
[CAMEL_SUMMARY] - Dataset: CSV file with columns: customer_id, product_name, quantity, price
- Tools used: pandas for data manipulation, matplotlib for visualization
- Key actions:
- Loaded CSV data using
pd.read_csv() - Calculated revenue per row as
quantity * price - Grouped data by
customer_idand summed revenue to find total revenue per customer - Sorted and selected top 5 customers by total revenue
- Created a bar chart to visualize top 5 customers' revenue using matplotlib
- Saved the bar chart to a PNG file with high resolution and proper layout
- Loaded CSV data using
- Clarification: Multiple entries per customer_id are expected as each row is a separate transaction
- Next steps / Action items:
- Use provided code snippets to perform analysis and visualization
- Save charts as image files for reporting or presentation success
^^^^^^^ BUG!
Message 3 [USER]:
Thanks! Can you also show me how to add a legend to the chart?
|
Hey @fengju0213 just as a reference that might be helpful, I ran Claude Code until it summarized the full context, and here is how it looks: I think there are helpful patterns here we can learn from. moreover, if you notice the tone of the summary (like "Continue with the last task...") you notice the role cannot be Assistant here. I think the best approach would be to append the summary to the system message and wipe the memory. |
Description
Context creator now preserves the system message and returns the remaining history strictly by
timestamp, removing all per-message token bookkeeping.
necessary, swaps in an assistant summary, and records summary state (depth, new-record counts,
last user input). This state blocks back-to-back summaries with negligible progress and caps
retries at three, preventing summarize→retry loops—even when the immediate overflow comes from a
tool call.
should control limits through model backend configuration.
Pending work:
1.clean up the modules related to token_limit and the token counter.
2.Add unit tests that cover input strings capable of triggering the token-limit path.
Checklist
Go over all the following points, and put an
xin all the boxes that apply.Fixes #issue-numberin the PR description (required)pyproject.tomlanduv lockIf you are unsure about any of these, don't hesitate to ask. We are here to help!