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 3 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 from 0eaec72 to 2cc5089 Compare January 26, 2025 03:57
Convert `_match_prefix_helper`, `_insert_helper`, `_print_helper`, and `_total_size_helper` from recursive to iterative implementations using stacks or loop. This prevents potential stack overflow errors with deep trees while maintaining the same functionality. Also update the test cases to use tensor values instead of strings for better testing of the radix cache's core functionality.
@luzengxiangcn luzengxiangcn force-pushed the debug_radixcache_stack_overflow branch 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.

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