-
Notifications
You must be signed in to change notification settings - Fork 0
add multinomial_with_seed for sampler and test_sampler.py #12
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
Conversation
Summary of ChangesHello @aolemila, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to the token sampling mechanism by implementing a Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a valuable feature for deterministic sampling with multinomial_with_seed
and adds corresponding tests. The propagation of sampling_seed
and positions
parameters through the system is handled well. However, there are a couple of critical issues that need addressing. The new multinomial_with_seed
function modifies os.environ
to enable 64-bit support in JAX, which is a dangerous side-effect within a JIT-compiled function and can lead to race conditions and unpredictable behavior. This should be configured at application startup. Additionally, a typo np.concat
instead of np.concatenate
will cause a runtime error. I've also included several medium-severity suggestions to improve code style, readability, and fix minor typos.
024e890
to
bec5ffc
Compare
…, add enable-deterministic-sampling arg, support temperature, top_k, top_p, min_p and sampling_seed without cache_miss
df971ab
to
2e6e64e
Compare
2e6e64e
to
d29ada1
Compare
/LGTM |
Motivation
From sgl-project#202.
Here supports the seed feature, which can be specified by every request. But this feature has to be used with
--enable-deterministic-sampling
.note
multinomial_with_seed
is from SGLang codes.Command
Accurancy
not deterministic
deterministic
Benchmark
not deterministic
deterministic