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.

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

Fix bug in main.cpp (penalize_nl=false doesn't work). Supress warning on mingw. #1528

Merged
merged 5 commits into from
Aug 26, 2023
Merged

Fix bug in main.cpp (penalize_nl=false doesn't work). Supress warning on mingw. #1528

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e12f3fb
Fix bug in main.cpp where penalize_nl=false has no effect. It modifie…
tom7 May 19, 2023
311bd6d
Suppress redefinition warning for NOMINMAX on mingw. In my installati…
tom7 May 19, 2023
f5d4b48
main : fix indentation
ggerganov Aug 26, 2023
0d29c8a
Merge branch 'master' into master
ggerganov Aug 26, 2023
1bf050c
main : pass ctx to llama_token_nl()
ggerganov Aug 26, 2023
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
9 changes: 8 additions & 1 deletion examples/main/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@
#include <unistd.h>
#elif defined (_WIN32)
#define WIN32_LEAN_AND_MEAN
#ifndef NOMINMAX
#define NOMINMAX
#endif
#include <windows.h>
#include <signal.h>
#endif
Expand Down Expand Up @@ -428,7 +430,12 @@ int main(int argc, char ** argv) {
last_n_tokens.data() + last_n_tokens.size() - last_n_repeat,
last_n_repeat, alpha_frequency, alpha_presence);
if (!penalize_nl) {
logits[llama_token_nl()] = nl_logit;
for (size_t idx = 0; idx < candidates_p.size; idx++) {
if (candidates_p.data[idx].id == llama_token_nl()) {
candidates_p.data[idx].logit = nl_logit;
break;
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, the order of candidates_p does not seem to be changed.
Why do you think it has?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not that the order of candiates_p has changed, although IMO it's not great to rely on the fact that llama_sample_repetition_penalty and llama_sample_frequency_and_presence_penalties preserve the order (without at least documenting that). The main issue is that on line 418 we've copied the logits into local candidates vector (of llama_token_data_array, not raw floats). So modifying the original logit array from the context does nothing, right?

Copy link
Owner

Choose a reason for hiding this comment

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

@tom7 Apologise for the delay and not paying more attention to this - I think you are right and we've had this bug for quite some time now.

}

if (temp <= 0) {
Expand Down