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

Feat: add alternative choices selection methods #835

Merged
merged 2 commits into from
Aug 5, 2024

Conversation

AidanCooper
Copy link
Contributor

This will likely need refinement and optimisation, so consider this a proposal that I'd like to seek feedback on.

Motivation

SGLang's current choices normalisation method (token length normalised) often performs poorly due to bias towards longer-token options. This arises in cases where the later tokens of an option with many tokens are highly predictable based on its earlier tokens. This is the most succinct example I can come up with that illustrates the flaw, which will trip up even highly capable models:

@sgl.function
def example(s):
    s += sgl.user("What is the capital of France?")
    s += sgl.assistant(sgl.gen("answer", choices=["Paris", "Antidisestablishmentarianism"]))

This PR provides solutions to the above example, and should resolve #523, #608, and possibly other open issues.

Modification

This PR enables the choices normalisation methodology to be configurable, and alongside the existing token length normalised strategy, introduces two new alternatives:

  1. Greedy token selection
  2. Unconditional likelihood normalised, as per this link that @merrymercy shared with me

Both of these implementations probably need to be further refined. One potential issue I've noticed with greedy selection is that it if there are differences in the tokens prepended to the options for token healing purposes, then the selection will be based on this rather than the actual option, which doesn't seem right. It's outside the scope of this PR, but the token healing process in general seems to have an outsized impact on the choices selection.

Checklist

  1. Ensure pre-commit pre-commit run --all-files or other linting tools are used to fix potential lint issues.
  2. Confirm that modifications are covered by complete unit tests. If not, please add more unit tests for correctness.
  3. Modify documentation as needed, such as docstrings or example tutorials.

@max99x
Copy link
Contributor

max99x commented Jul 30, 2024

Really cool to see progress on this issue, as it's a major blocker we're facing. The most effective solution I've found is to use the probability of some kind of end token to distinguish the priority of choices which are prefixes of other choices. It does require me to specify which suffix token(s) to take into account. E.g. if I'm generating a JSON string, I look for a double quote; if I ask the model to answer with only the choice and nothing else, I look for EOT, etc. Would be nice to be able to support that through this same API.

@merrymercy
Copy link
Contributor

Could you resolve the conflicts? I will review it later this week.

@AidanCooper
Copy link
Contributor Author

Could you resolve the conflicts? I will review it later this week.

Done — thanks!

@zhyncs
Copy link
Member

zhyncs commented Aug 2, 2024

Hi @AidanCooper I've fixed the CI issue with the fork. Could you merge the latest main branch? Thanks.

Copy link
Member

@Ying1123 Ying1123 left a comment

Choose a reason for hiding this comment

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

This looks great! Although you call it a proposal, I like the overall design. I left some minor comments. We can merge this after you resolve them.

python/sglang/lang/ir.py Outdated Show resolved Hide resolved
python/sglang/lang/backend/runtime_endpoint.py Outdated Show resolved Hide resolved
python/sglang/test/test_choices.py Outdated Show resolved Hide resolved
@Ying1123 Ying1123 merged commit 94e0115 into sgl-project:main Aug 5, 2024
3 checks passed
@AidanCooper
Copy link
Contributor Author

Thanks @Ying1123! The downside to resolving the default behaviour at the API layer is that we can't specify backend-dependent values, but it's workable in this instance.

Thanks for merging this. I think it's possible that the new selection algorithms could be further optimised for real-world use with some further tweaking, but that will be easier done in follow-on PRs.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The choices normalised logprobs calculation returns poor results due to bias for longer-token options
5 participants