Repository navigation
Samplers: Remove ability to specify "full-context windows" from history-based samplers - #26524
Conversation
github-actions
Bot
added
documentation
31f798a to
60abc26
Compare
Because of backend-sampling we initialize samplers before the complete llama_context is there. Therefore, we cannot infer the resolved context length yet at the time we construct the samplers.
60abc26 to
eb75c42
Compare
ggerganov
added
the
merge ready
…gml-org#26524) * Resolve -1 to 1024 instead of ctx-len for samplers Because of backend-sampling we initialize samplers before the complete llama_context is there. Therefore, we cannot infer the resolved context length yet at the time we construct the samplers. * Shared default of 64 for history-based samplers, remove context_size
…gml-org#26524) * Resolve -1 to 1024 instead of ctx-len for samplers Because of backend-sampling we initialize samplers before the complete llama_context is there. Therefore, we cannot infer the resolved context length yet at the time we construct the samplers. * Shared default of 64 for history-based samplers, remove context_size
…gml-org#26524) * Resolve -1 to 1024 instead of ctx-len for samplers Because of backend-sampling we initialize samplers before the complete llama_context is there. Therefore, we cannot infer the resolved context length yet at the time we construct the samplers. * Shared default of 64 for history-based samplers, remove context_size
…gml-org#26524) * Resolve -1 to 1024 instead of ctx-len for samplers Because of backend-sampling we initialize samplers before the complete llama_context is there. Therefore, we cannot infer the resolved context length yet at the time we construct the samplers. * Shared default of 64 for history-based samplers, remove context_size
|
Hi @ORippler, late to the party here, but just wanted to raise a concern around DRY sampling (I authored the original PR that ported DRY to the project): a default as small as 64 means DRY can no longer see repetitions from more than a few sentences back, which is most of what it was built to catch in prose. While users can of course supply a higher |
What are the key differences between DRY and repeat-penalty samplers that would warrant a higher default for the former? I thought they would be different flavors of the same and different thresholds would thus overcomplicate things unnecessarily. |
Rep pen is per-token while DRY is sequence-based. Both samplers always had different defaults with rep pen at 64 since 2023 I believe and DRY at full context (-1) since its original PR in Oct. 2024. That split to my knowledge has worked well for the two years it was in effect and it relates to how the two behave very differently with a bigger context window. For standard rep pen with its per-token handling things can get very hairy quickly with large contexts as it is very sensitive to any token that appears in the entire window like a name appearing multiple times and such, while for DRY it handles multi-token patterns and new tokens only get penalized if they extend a pattern that was seen before past a configured limit ( Due to these differences, the two samplers sharing a default probably would hurt one or the other. |
## Title fix(sampling): make repetition penalties take effect ## Original problem `repeat_penalty`, `frequency_penalty` and `presence_penalty` did nothing unless the request (or config) also set a positive `repeat_last_n`. The same was true of DRY when `dry.penalty_last_n` was set to `-1`. mesh defaulted `repeat_last_n` to `-1` and documented it as "use the whole context". llama.cpp dropped whole-context penalty windows upstream (ggml-org/llama.cpp#26524) because backend sampling builds samplers before the context exists. Since then, a negative window passed to its penalties or DRY sampler is clamped to `0`, which disables the sampler. Our pin includes that change, so every request that left the window at its default silently lost its penalties. ## Diagnostics - At our pinned llama.cpp, `llama_sampler_init_penalties` and `llama_sampler_init_dry` both do `std::max(window, 0)`, and a `0` window disables the sampler. - skippy-server defaulted `repeat_last_n` to `-1`, and the native Skippy sampler passed it straight through. - Current llama.cpp master still clamps the same way, so updating the pin would not fix this. llama-server now defaults both windows to 64 and rejects negative values. ## Fix The fix follows llama.cpp in Rust instead of patching it: - Penalty and DRY windows default to 64 tokens, llama.cpp's default. - `-1` is still accepted, as a legacy alias for that default, so existing configs and clients keep working. Values below `-1` are still rejected, and `0` still disables the penalties. - The alias is applied when skippy-server parses a request, and again just before the native call. The second catches older peers that send `-1` over the stage transport. - A config that sets `repeat_last_n = -1` or `dry.penalty_last_n = -1` loads as before, with a warning: ```text defaults.request_defaults.repeat_last_n = -1 no longer means the whole context; it uses the default window of 64 tokens. Set an explicit token count instead ``` - The config docs no longer promise whole-context windows. **Behaviour change:** requests that set no penalty are unaffected. Requests that set a repeat, frequency or presence penalty without a positive `repeat_last_n` now get it applied over the last 64 tokens, where before they got nothing. Set `repeat_last_n = 0` to keep penalties off. **Compatibility:** no wire, ABI or config schema changes. In mixed-version meshes, an older peer that sends `-1` to a new final stage gets the 64-token default, and a new peer that sends `64` to an older final stage gets penalties over 64 tokens. ## Validation - [x] I ran the relevant local checks, or explained why they do not apply. - `cargo fmt --all --check` - `just no-console-print` - `cargo clippy --all-targets -- -D warnings` for `skippy-protocol`, `skippy-runtime`, `mesh-llm-config`, `skippy-server` and `mesh-llm` - `cargo test --lib` passes for `skippy-protocol` (74), `skippy-runtime` (249), `mesh-llm-config` (224), `skippy-server` (872) and `mesh-llm-host-runtime` (4114). New tests: - `penalty_windows_resolve_legacy_whole_context_to_the_default` (request parsing) - `sampling_raw_resolves_penalty_windows_for_llama_cpp` (the native sampling config) - `whole_context_windows_warn_at_each_configured_path` and `explicit_windows_do_not_warn` (config warnings) - Not run: an end-to-end inference check that compares output with and without a penalty. - [x] UI changes include screenshots or video, or explain why visual aids are not needed. No UI changes. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Configuration** * Repetition and DRY penalty windows now default to the most recent 64 tokens. Set a window to `0` to disable it; explicit nonnegative values remain unchanged. * Existing configurations using `-1` continue to work and use the 64-token default, with a compatibility warning. Values below `-1` are rejected. * **Documentation** * Updated configuration guides to describe the 64-token defaults, the effect of `0`, and the handling of legacy `-1` values. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Overview
Additional information
penalty_last_n < 0in all consumers, but clamp them to 0 when directly constructing samplers vialibllamaRequirements