Skip to content

feat: update grouped_topk to support softmax and sigmoid #3680

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 4 commits into from
Feb 21, 2025
Merged

feat: update grouped_topk to support softmax and sigmoid #3680

merged 4 commits into from
Feb 21, 2025

Conversation

zixuanzhang226
Copy link
Contributor

@zixuanzhang226 zixuanzhang226 commented Feb 19, 2025 •
edited
Loading

Motivation

This PR is to support both softmax and sigmoid scoring functions in grouped_topk. Also, verified DeepSeek V2/V3/R1 uses biased_grouped_top and updated the corresponding comments.
Ref #2739

Modifications

Checklist

@zixuanzhang226 zixuanzhang226 force-pushed the feat/support-softmax-and-sigmoid-scoring-in-grouped-topk branch from 0f3db83 to af2ee75 Compare February 19, 2025 00:52
@zixuanzhang226 zixuanzhang226 mentioned this pull request Feb 19, 2025
Closed
2 tasks
@zixuanzhang226 zixuanzhang226 force-pushed the feat/support-softmax-and-sigmoid-scoring-in-grouped-topk branch from af2ee75 to 55c2e68 Compare February 19, 2025 01:18
FrankLeeeee
FrankLeeeee approved these changes Feb 19, 2025
View reviewed changes
Copy link
Collaborator

@FrankLeeeee FrankLeeeee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@FrankLeeeee
Copy link
Collaborator

cc @zhyncs @zhaochenyang20

@zhaochenyang20
Copy link
Collaborator

@FrankLeeeee Thanks @zixuanzhang226 could you rebase with the main? I can merge it tommorow.

@zixuanzhang226 zixuanzhang226 force-pushed the feat/support-softmax-and-sigmoid-scoring-in-grouped-topk branch from 55c2e68 to f84b584 Compare February 19, 2025 23:55
@zixuanzhang226
Copy link
Contributor Author

@FrankLeeeee Thanks @zixuanzhang226 could you rebase with the main? I can merge it tommorow.

Hi @zhaochenyang20 , I rebased with main. Thank you!

@zhaochenyang20
Copy link
Collaborator

@zixuanzhang226 Still out of main. And, could you add a description to this PR.

@zixuanzhang226 zixuanzhang226 force-pushed the feat/support-softmax-and-sigmoid-scoring-in-grouped-topk branch from f84b584 to 9ec7746 Compare February 20, 2025 19:34
@zhaochenyang20
Copy link
Collaborator

@zixuanzhang226 could you rebase this with the main? And, do we need to merge it? @FrankLeeeee

@zixuanzhang226
Copy link
Contributor Author

@zixuanzhang226 could you rebase this with the main? And, do we need to merge it? @FrankLeeeee

I rebased it! Thanks!

@zixuanzhang226
Copy link
Contributor Author

@zhaochenyang20 I also updated the description! Thank you!

@FrankLeeeee
Copy link
Collaborator

@zhaochenyang20 this pr can be merged

@zixuanzhang226
Copy link
Contributor Author

@zhaochenyang20 this pr can be merged

Hello @zhaochenyang20, would you please merge it? Thank you!

@zhaochenyang20
Copy link
Collaborator

@zixuanzhang226 I don't know why I can't rebase your PR. Could you rebase it and I can run the CI. I don't have access to merge main into your PR.

@FrankLeeeee
Copy link
Collaborator

@zhaochenyang20 squash and merge?

@zhyncs zhyncs merged commit 0c227ee into sgl-project:main Feb 21, 2025
3 of 18 checks passed
aoshen524 pushed a commit to aoshen524/sglang that referenced this pull request Mar 10, 2025
@zixuanzhang226 @ShenAo1111
feat: update grouped_topk to support softmax and sigmoid (sgl-project…
1de3709 
…#3680)
@ispobock ispobock mentioned this pull request Mar 17, 2025
Closed
5 tasks
ispobock added a commit that referenced this pull request Mar 17, 2025
@ispobock
Revert "feat: update grouped_topk to support softmax and sigmoid (#3680…
7fb5d08 
…)"

This reverts commit 0c227ee.
@ispobock ispobock mentioned this pull request Mar 17, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FrankLeeeee FrankLeeeee FrankLeeeee approved these changes

@merrymercy merrymercy Awaiting requested review from merrymercy merrymercy is a code owner

@Ying1123 Ying1123 Awaiting requested review from Ying1123 Ying1123 is a code owner

@zhyncs zhyncs Awaiting requested review from zhyncs zhyncs is a code owner

@ispobock ispobock Awaiting requested review from ispobock ispobock is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zixuanzhang226 @FrankLeeeee @zhaochenyang20 @zhyncs