Skip to content
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

Debug radixcache: refactor recursive helper methods #3029

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

luzengxiangcn
Copy link
Contributor

@luzengxiangcn luzengxiangcn commented Jan 21, 2025

Motivation

Currently radixcache is implemented by several recursive functions, which may cause PYTHON Stack overflow problem when tree hits a certain depth. You may reproduce this issue by following code.

from openai import OpenAI
client = OpenAI(base_url="<sglang server url>", api_key="tests")
max_stack_depth = 5000
for i in range(max_stack_depth+10):
    print(i)
    messages = [{"role":"user", "content": "hello!" + "".join([" i"] * i)}]
    print(client.chat.completions.create(model="model", messages=messages, max_tokens=10))

Related Issue: #813
#1499 fixed one of recursive function but NOT ALL: .

Modifications

Replace recursion functions with loop or stack in radix_cache.py

Checklist

@luzengxiangcn luzengxiangcn force-pushed the debug_radixcache_stack_overflow branch 12 times, most recently from cb2dfd7 to 2cc5089 Compare January 23, 2025 09:21
@luzengxiangcn luzengxiangcn marked this pull request as ready for review January 24, 2025 02:19
@luzengxiangcn luzengxiangcn changed the title Refactor recursive helper methods to iterative approach to prevent s… Debug radixcache: Refactor recursive helper methods to iterative approach to prevent s… Jan 24, 2025
@luzengxiangcn luzengxiangcn changed the title Debug radixcache: Refactor recursive helper methods to iterative approach to prevent s… Debug radixcache: refactor recursive helper methods Jan 24, 2025
@merrymercy
Copy link
Contributor

cc @xiezhq-hermann @hnyls2002 Can you help review this?

@xiezhq-hermann
Copy link
Collaborator

Thank you for the contribution, can you make the change minimal and to the point? For example, put the style-wise change in a separated PR if needed.

@luzengxiangcn luzengxiangcn force-pushed the debug_radixcache_stack_overflow branch 2 times, most recently from 2cc5089 to d02f0d0 Compare January 26, 2025 04:17
@luzengxiangcn
Copy link
Contributor Author

Thank you for the contribution, can you make the change minimal and to the point? For example, put the style-wise change in a separated PR if needed.

@xiezhq-hermann Sorry for the inconvenience caused to you due to the extensive code refactoring. To minimize the impact, I made minimal changes and pushed a new commit. I believe this will serve as a better starting point for the review.

Additionally, we can enhance the elegance of the code in the following two aspects, which can be done in another PR:

  1. The logic of insert is essentially match logic + append new node, and the redundant parts can be merged to reduce code duplication.
  2. The original helper functions were designed for iterative logic. After removing the iterative logic, the logic of these functions can be directly integrated into the original function.

@luzengxiangcn luzengxiangcn force-pushed the debug_radixcache_stack_overflow branch 3 times, most recently from 3c8ce57 to 1ecc9ef Compare February 5, 2025 03:36
@luzengxiangcn luzengxiangcn force-pushed the debug_radixcache_stack_overflow branch from 635c8a3 to 4885b90 Compare February 5, 2025 05:37
@luzengxiangcn
Copy link
Contributor Author

clean code and debug logic error

@luzengxiangcn luzengxiangcn reopened this Feb 5, 2025
@luzengxiangcn luzengxiangcn force-pushed the debug_radixcache_stack_overflow branch from 6d95ee7 to 4447c79 Compare February 5, 2025 08:54
Replace recursive implementations with iterative ones using stacks in `_match_prefix_helper`, `_print_helper`, and `_total_size_helper` to prevent potential stack overflow issues. Simplify the return value handling in `_match_prefix_helper` by removing the list parameter and returning a tuple directly. Streamline the `_insert_helper` logic with a while loop for better readability and maintainability. These changes improve performance and reliability while maintaining the same functionality.
@luzengxiangcn luzengxiangcn force-pushed the debug_radixcache_stack_overflow branch from 4447c79 to b39aaae Compare February 6, 2025 04:12
@luzengxiangcn
Copy link
Contributor Author

@xiezhq-hermann Would you please help me review this? I fixed some logic errors in this PR, the code should be good now.

@xiezhq-hermann xiezhq-hermann self-requested a review February 8, 2025 01:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants