Skip to content

sampling: add Top-nσ sampler to llama-server #11896

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

Closed
wants to merge 8 commits into from
Closed

sampling: add Top-nσ sampler to llama-server #11896

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1bdb603
sampling: add Top-nσ sampler to `llama-server` and sampler ordering
CasualAutopsy Feb 15, 2025
ff8b612
revert: sampler ordering
CasualAutopsy Feb 15, 2025
1dc5d84
revert: VS' crappy auto-formatting
CasualAutopsy Feb 15, 2025
9068068
revert: VS' crappy auto-formatting pt.2
CasualAutopsy Feb 15, 2025
c05e9e0
revert: my crappy eye sight...
CasualAutopsy Feb 15, 2025
a9e7af0
sampling: add XTC to Top-nσ sampler chain
CasualAutopsy Feb 16, 2025
cc1a170
sampling: add Dyna. Temp. to Top-nσ sampler chain
CasualAutopsy Feb 16, 2025
a558d3a
sampling: actually remove Top-nσ from sampler(oops)
CasualAutopsy Feb 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion common/sampling.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,8 +190,9 @@ struct common_sampler * common_sampler_init(const struct llama_model * model, co
if (params.mirostat == 0) {
if (params.top_n_sigma >= 0) {
llama_sampler_chain_add(result->chain, llama_sampler_init_top_k (params.top_k));
llama_sampler_chain_add(result->chain, llama_sampler_init_temp (params.temp));
llama_sampler_chain_add(result->chain, llama_sampler_init_temp_ext (params.temp, params.dynatemp_range, params.dynatemp_exponent));
llama_sampler_chain_add(result->chain, llama_sampler_init_top_n_sigma (params.top_n_sigma));
llama_sampler_chain_add(result->chain, llama_sampler_init_xtc (params.xtc_probability, params.xtc_threshold, params.min_keep, params.seed));
} else {
Comment on lines 191 to 196
Copy link
Member

Choose a reason for hiding this comment

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

This special-case should probably be removed and configure the sampler chain as usual. @VJHack What was the reason to do it this way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ggerganov When I tested the sampler it didn't work with the penalties or DRY. I'm sure that's the reason why.

Copy link
Contributor

Choose a reason for hiding this comment

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

There was a discussion about top_n_sigma being added into the main chain, but it was probably lost.

This sampler is supposed to run after temp (temp_ext works too), but the current main chain has temperature after all other truncating samplers by default. If top_n_sigma is put in that order, it will suffer from previous samplers that may be used after it, but not before. Ideally, it's top-k->temp->top_nsigma->*everything else*, but that would require changing current default order of samplers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like based on comments from the original author of top_n_sigma in the aphrodite-engine PR thread (aphrodite-engine/aphrodite-engine#825), it doesn't matter whether top_n_sigma is used before or after temperature, but it will result in errors if it is used after other "alphabet" truncation samplers, with the exception of top_k.

Copy link
Contributor

@VJHack VJHack Feb 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This special-case should probably be removed and configure the sampler chain as usual. @VJHack What was the reason to do it this way?

I made this sampler an exception because of two reasons, one of which is stated in the PR:

  1. As the authors of the paper mention, the manipulation for top_n_sigma is done on the logits pre-softmax. Some of the other samplers that would come beforetop_n_sigma (like top_p for example) are taking the softmax before truncating the number of tokens we can sample from. I was concerned that this would degrade the quality of this sampler.
  2. top_n_sigma must be used after temperature is applied. In the paper they repeatedly mention the temperature invariance property being one of the key characteristics of this sampler. @DocShotgun, I understand that this can be a bit confusing but temperature invariance means that the set of candidate tokens remains constant regardless of the temperature value used. According to the algorithm, temperature must be applied before top_n_sigma.

I made top_n_sigma a special-case (stand alone) sampler because it operates on logits pre-softmax and must come after temperature is applied.

Ideally, it's top-k->temp->top_nsigma->*everything else*, but that would require changing current default order of samplers.

I agree with this.

Copy link
Contributor

@MaggotHATE MaggotHATE Feb 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

most backends apply penalties as the very first sampler.

Not sure how significant it is for other backends, but applying penalties to the whole vocabulary affects performance in llama.cpp, so it is subject to change in the future, some day.

As for top_n_sigma and repetitions, I tested it a bit with the same order as in the main chain, but theoretically it should not be needed. This aspect needs more testing, and I'm currently preoccupied by figuring out if Mistral Small 3 is good or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

As for top_n_sigma and repetitions, I tested it a bit with the same order as in the main chain, but theoretically it should not be needed. This aspect needs more testing, and I'm currently preoccupied by figuring out if Mistral Small 3 is good or not.

Hmm so at least it should function properly with penalties? It doesn't look like penalties are allowed to be used with top_n_sigma in this current implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Um... Sorry for the trouble, but my scatter brain ass forgot to remove the lines of code that added nsigma to common samplers. It seems to have caused and error with the workflows since it isn't using the nsigma sampler, making it hang infinitely... sorry...

Copy link
Contributor

@MaggotHATE MaggotHATE Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It doesn't look like penalties are allowed to be used with top_n_sigma in this current implementation

Yes, either repetition penalty and DRY should be added to the new chain (after top_k), or top_n_sigma should be put into the main chain.

I'm not sure if changing the default chain order to logits->top_k->penalties->temp->*everything else* is a good idea now, so maybe it is worth putting this idea aside for a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, either repetition penalty and DRY should be added to the new chain (after top_k), or top_n_sigma should be put into the main chain.

Let's not add a new chain. We should either put the top-sigma sampler in the existing chain or let the user explicitly add the sampler. Remember that this is just a default configuration, the sampler is noop by default and the user can re-configure the order of the samplers. If the sampler has to be applied before softmax, the user is responsible to construct the sampling chain to make sense.

for (const auto & cnstr : params.samplers) {
switch (cnstr) {
Expand Down
2 changes: 2 additions & 0 deletions examples/server/server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ struct slot_params {
{"top_k", sampling.top_k},
{"top_p", sampling.top_p},
{"min_p", sampling.min_p},
{"top_n_sigma", sampling.top_n_sigma},
{"xtc_probability", sampling.xtc_probability},
{"xtc_threshold", sampling.xtc_threshold},
{"typical_p", sampling.typ_p},
Expand Down Expand Up @@ -247,6 +248,7 @@ struct server_task {
params.sampling.top_k = json_value(data, "top_k", defaults.sampling.top_k);
params.sampling.top_p = json_value(data, "top_p", defaults.sampling.top_p);
params.sampling.min_p = json_value(data, "min_p", defaults.sampling.min_p);
params.sampling.top_n_sigma = json_value(data, "top_n_sigma", defaults.sampling.top_n_sigma);
params.sampling.xtc_probability = json_value(data, "xtc_probability", defaults.sampling.xtc_probability);
params.sampling.xtc_threshold = json_value(data, "xtc_threshold", defaults.sampling.xtc_threshold);
params.sampling.typ_p = json_value(data, "typical_p", defaults.sampling.typ_p);
Expand Down