Skip to content

llama : greatly reduce output buffer memory usage #6122

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.

Sign up for GitHub

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

Merged
merged 26 commits into from
Mar 26, 2024
Merged

llama : greatly reduce output buffer memory usage #6122

Changes from 1 commit
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
1fd1918
llama : greatly reduce logits memory usage
compilade Mar 15, 2024
98914c0
llama : more compact state saving and reloading
compilade Mar 15, 2024
705d393
llama : fix lctx.n_outputs not being set before building graph
compilade Mar 16, 2024
25981fc
perplexity : adapt to the logits API changes
compilade Mar 17, 2024
17b45c9
perplexity : fix Winogrande, use correct logits for second choice start
compilade Mar 17, 2024
d0129e8
perplexity : normalize spaces and punctuation in Winogrande sentences
compilade Mar 17, 2024
487f89e
llama : fix embedding conditions
compilade Mar 17, 2024
408fcb0
llama : fix llama_get_embeddings_ith when the resulting id is 0
compilade Mar 17, 2024
e19cb3a
llama : fix wrong n_outputs in llama_set_inputs
compilade Mar 17, 2024
a57fa7f
llama : fix not-skipping outputs of non-causal models
compilade Mar 18, 2024
711b0bc
llama : fix running a batch with n_outputs == 0
compilade Mar 18, 2024
d100502
llama : keep same graph topology even when n_outputs == 0
compilade Mar 18, 2024
99c37cc
ggml : saner ggml_can_repeat with empty tensors
compilade Mar 18, 2024
6bf7f3f
ggml : do not multi-thread ops returning empty tensors
compilade Mar 18, 2024
09bb15a
ggml : make ggml_is_empty public and work with views
compilade Mar 19, 2024
4551e7e
llama : use a vector for ctx->output_ids
compilade Mar 19, 2024
8b826c5
ggml : skip empty tensors in all backends
compilade Mar 19, 2024
d04cfaf
llama : fix llama_output_reserve nullptr deref when new_size is 0
compilade Mar 19, 2024
8f70dcb
perplexity : make Winogrande work as it does on master
compilade Mar 19, 2024
615a3a4
llama : clearer error messages for invalid logits or embeddings ids
compilade Mar 19, 2024
7d8d6b5
llama : handle errors from llama_output_reserve at call sites
compilade Mar 21, 2024
5f33a67
perplexity : make hellaswag and multiple-choice outputs identical to …
compilade Mar 21, 2024
ffa9abd
Merge branch 'master' into compilade/smaller-output-buffer
compilade Mar 25, 2024
e9095ac
llama : allow loading state saved with a different ctx size
compilade Mar 26, 2024
5027d81
llama : minor
ggerganov Mar 26, 2024
20248e8
readme : update recent API changes, and warn about Vulkan
compilade Mar 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
perplexity : normalize spaces and punctuation in Winogrande sentences
  • Loading branch information
@compilade
compilade committed Mar 17, 2024
commit d0129e8e2963c17dd9e17a7bec8f54b981fa949b
32 changes: 32 additions & 0 deletions examples/perplexity/perplexity.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1038,6 +1038,38 @@ static std::vector<winogrande_entry> load_winogrande_from_csv(const std::string&
auto choice2 = line.substr(comma_pos[2]+1, comma_pos[3] - comma_pos[2] - 1);
auto answer = line.substr(comma_pos[3]+1, line.size() - comma_pos[3] - 1);
auto index = line.substr(0, comma_pos[0]);
if ('a' <= sentence[0] && sentence[0] <= 'z') {
// make the first letter a capital letter
sentence[0] -= 'a' - 'A';
}
for (int i = 0; i < (int) sentence.size() - 1; ++i) {
// trim repeated spaces and spaces before punctuation
if (sentence[i] == ' ') {
char next = sentence[i+1];
if (next == ' ' || next == ',' || next == '.' || next == '\'') {
char r[2] = { next, 0 };
sentence.replace(i, 2, r);
--i; // stay at the same index for repeated spaces
}
} else if (sentence[i] == ',' || sentence[i] == '.') {
if (sentence[i] == sentence[i+1]) {
// trim repeated punctuation (forward to work at the end of sentences)
char r[2] = { sentence[i], 0 };
sentence.replace(i, 2, r);
--i; // same index to then run the other checks on that punctuation
} else if (0 < i && sentence[i-1] == sentence[i]) {
// trim repeated punctuation (looks back to work with the space trim)
char r[2] = { sentence[i], 0 };
sentence.replace(i-1, 2, r);
i -= 2; // go back because content was shifted
} else if (sentence[i+1] != ' ') {
// add missing space after punctuation
// (since the loop stops before the end, this adds no trailing space)
char r[3] = { sentence[i], ' ', 0 };
sentence.replace(i, 1, r);
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change related to the PR? If it isn't, it should probably be in a different PR so that it is easier to review.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's somewhat related, because I've changed how the Winogrande score is calculated (it didn't use the correct logits for the choice word of the second choice before, and it used a choice-word-skipping logic to mitigate the effects), and normalizing the sentences helped to make Phi-2_Q4_K_M's score a bit better (72.6125 +/- 1.2533 when normalizing vs 72.0600 +/- 1.2611 when not normalizing). Some of the sentences in the Winogrande dataset I used has weird punctuation and inconsistent spacing, which sometimes slightly skews the log-likelyhood, but this only affects 111/1267 of the tasks in https://huggingface.co/datasets/ikawrakow/winogrande-eval-for-llama.cpp.

$ grep -E "(^[a-z]|\.\w|\.\.| '| \.| ,|  +)" winogrande-debiased-eval.csv | wc -l
111

I could try extracting this into another PR or removing it altogether if it's too out-of-place, like if it's better to keep the dataset as-is even if some of the formatting confuses some models.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can review the changes in llama.cpp and ggml, but I cannot review these changes. Since perplexity is used to test the correctness of many other things, I think it is important that the changes are carefully reviewed. So as it is, either someone else will have to review this PR, or it will have to be split.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case I could make Winogrande work as it does on master, even though it's wrong and needlessly complicated, and then fix it in a separate PR. All of the benchmarks in perplexity (except Winogrande, but I could fix this) should give the exact same results in this PR as on master, which should make it relatively easy to test that it works as expected or know if I made a mistake somewhere.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Wrong and needlessly complicated": this basically necessitates a separate PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have been testing hellaswag since there are some changes, and over 400 tasks with I see a difference of 0.25 compared to master (75.00 vs 74.75) with llama 7B F16. Over mistral F16 there is also a small difference (81.25 vs 81.00). Over the full 10042 tasks the result is basically the same so it is probably nothing, but I wonder if something changed that could result in a different computation? I thought it could be due to different matrix sizes, but there is also a difference with MMQ.

Copy link
Collaborator Author

@compilade compilade Mar 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if something changed that could result in a different computation?

I've tested with stories260K.gguf while showing the logprobs of the 4 choices of each task, and I see that the numbers (compared to master) are (mostly) exactly the same for the first choice, while for the other three, the 4 most significant digits match while the rest seem to vary. So if this changes the final result, the choices were already very close to each other. This is either due to some numerical instability in floats or there's something I did wrong differently. (apparently, it's both)

Okay, while typing this I ran some tests, and I don't understand why, but evaluating the last tokens of the sequences (even without extracting the logits) is apparently needed to get the exact same numbers as master. Previously, I simply skipped the evaluation of the last tokens because their output isn't necessary at all. It's weird because the choices are in separate sequences, so the KV cache shouldn't be affected... (tested with Mamba too, and at least my intuition holds there, so the way the KV cache is updated for Transformer-based models makes this different. Why would the KV cells of unrelated sequences be changed slightly like this?) I'll need to run tests for the other benchmarks and will push a fix tomorrow.

I've also noticed that when running the same benchmark with different batch sizes (even on master), I see similar differences on batch boundaries. So maybe it's not a big deal? (the difference in logprobs have the same magnitude in both cases (the max difference I saw was around 0.0003)). Should I try to keep compatibility with the exact same "rounding errors" as master? (EDIT: Answering to myself: Yes, keep compatibility, at least in this PR. Fix implemented in 5f33a67)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See this comment (and the one referenced in it) for explanation of why the results are different: ggml-org/whisper.cpp#1941 (comment)

int where = 0;
for ( ; where < int(sentence.size()); ++where) {
if (sentence[where] == '_') break;
Expand Down