[BUG]: DefragThreshold default is not matching llama.cpp and probably not intended

[dlyz]

Description

DefragThreshold is 0 by default in LLamaSharp with the intent that it will disable defragmentation on the threshold excess.

LLamaSharp/LLama/Abstractions/IContextParams.cs

Line 112 in 6bf010d

/// defragment the KV cache if holes/size > defrag_threshold, Set to < 0 to disable (default)

LLamaSharp/LLama/Common/ModelParams.cs

Line 103 in 6bf010d

public float DefragThreshold { get; set; }

But actually it should be negative to disable this mechanics (-1). 0 means "do defrag when the smallest fragmentation discovered".

This may negatively (maybe sometimes positively) impact performance in some advanced cases, for example with batching.

See llama.cpp default https://github.com/ggerganov/llama.cpp/blob/3ab8b3a92ede46df88bc5a2dfca3777de4a2b2b6/llama.cpp#L11630

Reproduction Steps

-

Environment & Configuration

-

Known Workarounds

No response

[martindevans]

Good catch! Would you be itnerested in putting together a PR to fix this?

The easiest fix would be to just add a default value of -1.

I think a better fix would be to change float DefragThreshold to float? DefragThreshold and then to convert null to -1 in here. That way the default C# value (null) converts to the default llama.cpp value and there are no "magic" numbers.

[Neilblaze]

@AsakusaRinne Is this up for grabs?

[martindevans]

Unless @dlyz is already working on it, I think it's open to whoever moves first :)

[dlyz]

Yeah, sorry, couldn't find time for it yet, maybe closer to the end of the week. @Neilblaze if you can do it earlier, you should definitely take it.

If we go with nullable DefragThreshold, we should probably make Seed nullable too, because 'no seed' constant is not obvious. And in LLamaContext.SetSeed too.

[Neilblaze]

Yeah, sorry, couldn't find time for it yet, maybe closer to the end of the week. @Neilblaze if you can do it earlier, you should definitely take it.

Cool I'll try my best to raise a PR asap, but meanwhile, I'm waiting for this confirmation ↓

If we go with nullable DefragThreshold, we should probably make Seed nullable too, because 'no seed' constant is not obvious. And in LLamaContext.SetSeed too.

Once @martindevans confirms, I can make the updates. Thanks!

[martindevans]

Making seed nullable for the default is a good idea. Go for it @Neilblaze :)