sampling: add Top-nσ sampler to llama-server
#11896
Conversation
CasualAutopsy
changed the title
llama-server and sampler orderingllama-server
| 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 { |
There was a problem hiding this comment.
This special-case should probably be removed and configure the sampler chain as usual. @VJHack What was the reason to do it this way?
There was a problem hiding this comment.
@ggerganov When I tested the sampler it didn't work with the penalties or DRY. I'm sure that's the reason why.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- As the authors of the paper mention, the manipulation for
top_n_sigmais done on the logits pre-softmax. Some of the other samplers that would come beforetop_n_sigma(liketop_pfor 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. top_n_sigmamust be used aftertemperatureis 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 beforetop_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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
As for
top_n_sigmaand 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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
It doesn't look like penalties are allowed to be used with
top_n_sigmain 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.
There was a problem hiding this comment.
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.
Sampler ordering* Sorry about that. (I'm a novice, please don't kill me.) |
No description provided.