- Notifications
You must be signed in to change notification settings - Fork5.3k
Feature/bug fix of thinking llm in vllm#3510
Conversation
CLAassistant commentedSep 27, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
|
parshvadaftari left a comment
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.
Hey thanks for raising the PR, can you please incorporate the requested changes?
mem0/memory/main.py Outdated
| ) | ||
| try: | ||
| response = remove_code_blocks(response) | ||
| if '</think>' in response: |
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.
Hey instead of creating a new utils for removing "think" blocks, it would be better if you incorporate it into theremove_code_blocks. This will reduce the code redundancy and latency.
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.
Hi, thanks for the review!
I've remove theremove_thinking_tagsfunction, and adjust theremove_code_blocks.
Is that OK?
tests/memory/test_thinking_tag.py Outdated
| @@ -0,0 +1,135 @@ | |||
| from unittest.mock import MagicMock, patch | |||
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.
A bit skeptical about a entire separate test for removing the "thinking tags". Can we add this to test_vllm?
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.
Move the test totest_vllm.py
bug__fix.md Outdated
| @@ -0,0 +1,64 @@ | |||
| # Ensure when using vllm, the <think></think> tags are handled instead of throw an error | |||
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.
Could you please remove this?
3204212 to0dad8d3Comparedog-last commentedSep 30, 2025
The latest change makes sure:
Thanks again for your attention😊 |
| def create_mocked_memory(): |
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.
This test is not required and also doesn't cover any vllm aspect.
| def create_mocked_async_memory(): | ||
| """Create a fully mocked AsyncMemory instance for testing.""" |
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.
This test is not required and also doesn't cover any vllm aspect.
tests/llms/test_vllm.py Outdated
| @pytest.mark.asyncio | ||
| async def test_async_thinking_tags_in_add_to_vector_store(): |
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.
A bit nit picking over here, but the test function name can be bitter, which doesn't have this long sentence like name.
tests/llms/test_vllm.py Outdated
| return memory, mock_llm, mock_vector_store | ||
| def test_thinking_tags_in_add_to_vector_store(): |
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.
The name of the function seems to be a bit too long.
…hink></think> tags output by the LLM, instead of raise a JSON format error.
…the test to test_vllm.py instead of a entire separate test.
0dad8d3 to4b671a9Comparedog-last commentedOct 2, 2025
Thank you for the valuable feedback! I've made the following changes as suggested:
|
parshvadaftari left a comment
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.
Looks good to me.
51ce6f1 intomem0ai:mainUh oh!
There was an error while loading.Please reload this page.
parshvadaftari commentedOct 3, 2025
Thanks for contributing to mem0. |
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Please delete options that are not relevant.
Checklist:
Maintainer Checklist