Skip to content

[Bugfix][Frontend] Eliminate regex based check in reasoning full generator #14821

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Mar 28, 2025
Merged

[Bugfix][Frontend] Eliminate regex based check in reasoning full generator #14821

merged 7 commits into from
Mar 28, 2025

Conversation

gaocegege
Copy link
Contributor

@gaocegege gaocegege commented Mar 14, 2025 •
edited by github-actions bot
Loading

FIX #14761

This PR:

  • Removes buggy regex-based parser in reasoning full generation and introduces a string-compare approach.
  • Adds more test cases for deepseek reasoning

@github-actions GitHub Actions
Copy link

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging.

To run CI, PR reviewers can either: Add ready label to the PR or enable auto-merge.

🚀

@mergify mergify bot added the frontend label Mar 14, 2025
@gaocegege
Copy link
Contributor Author

/assign @aarnphm

@aarnphm
Copy link
Collaborator

aarnphm commented Mar 15, 2025

Is there a specific bug thst u run into?

This seems like a refactor to me

@aarnphm
Copy link
Collaborator

aarnphm commented Mar 15, 2025

oh i see. will take a look

@aarnphm aarnphm requested review from mgoin and aarnphm and removed request for simon-mo, DarkLight1337 and robertgshaw2-redhat March 15, 2025 03:28
@gaocegege
Copy link
Contributor Author

@cjackal Thanks for the clean implementation! I addressed your comments. PTAL

@gaocegege
[Bugfix] Eliminate regex based check in reasoning
391cda7 
Signed-off-by: Ce Gao <cegao@tensorchord.ai>
@gaocegege
fix: Update
a0612d1 
Signed-off-by: Ce Gao <cegao@tensorchord.ai>
@gaocegege
Address comments
5de9109 
Signed-off-by: Ce Gao <cegao@tensorchord.ai>
@gaocegege
Update
8afe808 
Signed-off-by: Ce Gao <cegao@tensorchord.ai>
@gaocegege
add more test cases
99aa883 
Signed-off-by: Ce Gao <cegao@tensorchord.ai>
@gaocegege
Fix pre-commit
ea5967a 
Signed-off-by: Ce Gao <cegao@tensorchord.ai>
@gaocegege
rebase
3cb5320 
Signed-off-by: Ce Gao <cegao@tensorchord.ai>
@gaocegege gaocegege changed the title [Bugfix] Eliminate regex based check in reasoning [Bugfix] Eliminate regex based check in reasoning full generator Mar 28, 2025
@gaocegege gaocegege changed the title [Bugfix] Eliminate regex based check in reasoning full generator [Bugfix][Frontend] Eliminate regex based check in reasoning full generator Mar 28, 2025
@gaocegege
Copy link
Contributor Author

gaocegege commented Mar 28, 2025 •
edited
Loading

#14428 has already been merged, which blocks this PR. Since the last review, the only change made was a rebase, with no additional modifications.

@aarnphm @DarkLight1337 Could you please help merge it?

@DarkLight1337 DarkLight1337 enabled auto-merge (squash) March 28, 2025 08:32
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Mar 28, 2025
@DarkLight1337 DarkLight1337 merged commit 3bbaacb into vllm-project:main Mar 28, 2025
38 of 40 checks passed
@gaocegege gaocegege deleted the regex branch March 28, 2025 11:55
Alex4210987 pushed a commit to LeiWang1999/vllm-bitblas that referenced this pull request Apr 5, 2025
@gaocegege @Alex4210987
[Bugfix][Frontend] Eliminate regex based check in reasoning full gene…
c5fa1e0 
…rator (vllm-project#14821)

Signed-off-by: Ce Gao <cegao@tensorchord.ai>
Signed-off-by: xinyuxiao <xinyuxiao2024@gmail.com>
lulmer pushed a commit to lulmer/vllm that referenced this pull request Apr 7, 2025
@gaocegege @lulmer
[Bugfix][Frontend] Eliminate regex based check in reasoning full gene…
4960fe3 
…rator (vllm-project#14821)

Signed-off-by: Ce Gao <cegao@tensorchord.ai>
Signed-off-by: Louis Ulmer <ulmerlouis@gmail.com>
@ckhordiasma ckhordiasma mentioned this pull request Apr 17, 2025
Closed
lk-chen pushed a commit to lk-chen/vllm that referenced this pull request Apr 29, 2025
@gaocegege @lk-chen
[Bugfix][Frontend] Eliminate regex based check in reasoning full gene…
c185271 
…rator (vllm-project#14821)

Signed-off-by: Ce Gao <cegao@tensorchord.ai>
shreyankg pushed a commit to shreyankg/vllm that referenced this pull request May 3, 2025
@gaocegege @shreyankg
[Bugfix][Frontend] Eliminate regex based check in reasoning full gene…
b28581f 
…rator (vllm-project#14821)

Signed-off-by: Ce Gao <cegao@tensorchord.ai>
RichardoMrMu pushed a commit to RichardoMrMu/vllm that referenced this pull request May 12, 2025
@gaocegege
[Bugfix][Frontend] Eliminate regex based check in reasoning full gene…
5305177 
…rator (vllm-project#14821)

Signed-off-by: Ce Gao <cegao@tensorchord.ai>
Signed-off-by: Mu Huai <tianbowen.tbw@antgroup.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aarnphm aarnphm aarnphm approved these changes

@cjackal cjackal cjackal approved these changes

@DarkLight1337 DarkLight1337 DarkLight1337 approved these changes

@mgoin mgoin Awaiting requested review from mgoin

Assignees
No one assigned
Labels
frontend ready ONLY add when PR is ready to merge/full CI is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: deepseek_r1_reasoning_parser.py except reasoning_content = self.reasoning_regex.findall(model_output)[0] IndexError: list index out of range
4 participants
@gaocegege @aarnphm @cjackal @DarkLight1337