Skip to content
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

[Bug] Mirostat samplers don't work properly with parallel generation #3537

Closed
KerfuffleV2 opened this issue Oct 8, 2023 · 2 comments · Fixed by #3543
Closed

[Bug] Mirostat samplers don't work properly with parallel generation #3537

KerfuffleV2 opened this issue Oct 8, 2023 · 2 comments · Fixed by #3543
Labels
bug Something isn't working generation quality Quality of model output

Comments

@KerfuffleV2
Copy link
Collaborator

This is because llama_sample_token in common.cpp uses a static for mirostat1 and 2 mu. Because of this, different sequences will affect each other (including ones that were already deleted).

The fix for this doesn't really seem that simple. I don't think it can be done only inside llama_sample_token. I think llama_sample_token is going to have to get changed to take something like a sequence-specific sampler state structure where stuff like that sequence's mu could get stored. Then it would be up to the app to reset mu when appropriate (like the sequence ends and the slot will be reused).

@KerfuffleV2 KerfuffleV2 added bug Something isn't working generation quality Quality of model output labels Oct 8, 2023
@ggerganov
Copy link
Owner

Yup, these statics should be removed. If the change for adding a sampling state is too big, we should probably disable mirostat sampling to avoid confusion, until the issue is resolved

@KerfuffleV2
Copy link
Collaborator Author

KerfuffleV2 commented Oct 8, 2023

I submitted #3543, but personally I don't really like that approach. (Pretty simple changes though.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working generation quality Quality of model output
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants