-
Notifications
You must be signed in to change notification settings - Fork 10.9k
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
Min P sampler implementation [alternative to Top P/Top K] #3841
Merged
Merged
Changes from all commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
59d1232
cuda : prints wip
ggerganov 52af782
cuda : new cublas gemm branch for multi-batch quantized src0
ggerganov 16b60dd
cuda : add F32 sgemm branch
ggerganov a3c2843
cuda : fine-tune >= VOLTA params + use MMQ only for small batches
ggerganov 4c6744b
cuda : remove duplicated cuBLAS GEMM code
ggerganov a4e15a3
cuda : add CUDA_USE_TENSOR_CORES and GGML_CUDA_FORCE_MMQ macros
ggerganov 49af767
build : add compile option to force use of MMQ kernels
ggerganov a9e2b74
Super hacky starting implementation of Min P
kalomaze a235a0d
Transform Min P into a proper CLI option
kalomaze 838d58d
Min P disabled if set to 1.0 or 0, otherwise Top P
kalomaze 69ef4ca
Debugging print statements removed
kalomaze 833637b
erring on the side of caution; disable by default
kalomaze 62fc771
Remove accidentally kept prints + min_keep support
kalomaze 49b68e8
Standardize 0.0 disabling min_p upon feedback
kalomaze 6f7cdec
Simplified counter by checking candidates size
kalomaze cb23358
minor whitespace fix
kalomaze fcbbfc1
Even formatting + exclusively 0.0f to disable now
kalomaze 69e638e
cleanup
cebtenzzre 3ddfd67
permit simultaneous use of top_p and min_p
cebtenzzre 18c0aa7
Merge remote-tracking branch 'original/cuda-quantum-batch' into min-p…
kalomaze 87adfad
Merge branch 'min-p-sampling' of https://github.com/kalomaze/koboldcp…
kalomaze 9248325
Update README & set 0.05 default
kalomaze 512cac6
added a bit more context to the README
kalomaze 974640a
Update README for consistency
kalomaze 3b58af2
forgot one small thing!
kalomaze File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Notice that min_p comes after all samplers, meaning that sampling would be among
top_k->tfs->typical->top_p
results, or, as we have enabled almost every sampler, amongtop_k->top_p
results. So, it is among Top K=40, among them Top P 95%, and only then Min PThere 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.
That order seems logical. Min-P is supposed to be a Top-P replacement, but if you were to use it together with other samplers it would fulfill the same role as Top-P and should have the same order. And the default being 0 makes sense since Top-P already has a default value, you don't want to combine both actively at the same time.
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.
I agree, it probably shouldn't just be enabled by default. I don't think it was like that when I looked at it, but maybe I missed it.
Maybe add a
--disable-all-samplers
option or something? I think the default settings are aimed at generally producing decent results, probably not the case with all samplers disabled.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.
Why not? To my understanding, it wouldn't be misleading to apply a Min-P on the top logits that combined make a certain procentage since Min-P takes into consideration the procentage of the first (highest probability) logit.
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.
Yes, it does seem like the default is currently 0.05, which probably should be changed to 0.