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

convert : ignore tokens if their IDs are within [0, vocab_size) #3831

Merged
merged 1 commit into from
Oct 28, 2023

Conversation

ggerganov
Copy link
Owner

continuation #3585

Haven't tested

Copy link
Collaborator

@KerfuffleV2 KerfuffleV2 left a comment

Choose a reason for hiding this comment

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

Super wishy-washy approval here. I really only updated it to apply to master and cleaned some stuff up, I didn't try to understand the code much. I tested it and it seemed to work on a model that needed manually removing the excess low-id tokens before.

Can we add TheBloke as contributor so I can pass the buck and make him review it? :) (He's a good person to review these kinds of changes since he converts so many models, also this should make his life easier if merged.) edit: I guess I'm dumb and he already is a contributor, but he doesn't appear in the list.

edit2: I'm dumb, but not in the way I initially thought I was dumb. So I guess that makes me extra dumb. He'd need to be a collaborator, not a contributor.

@KerfuffleV2
Copy link
Collaborator

@TheBloke If you have the time, could you please take a look at this one? It's #3585 updated to apply to master. Hopefully we can get this merged and you won't have to mess around manually removing those extra tokens from the metadata.

@TheBloke
Copy link
Contributor

Looks good to me! Tested it with a few models, all looks to be fine.

Thanks

@KerfuffleV2 KerfuffleV2 merged commit 8a2f2fe into master Oct 28, 2023
7 checks passed
Nexesenex pushed a commit to Nexesenex/croco.cpp that referenced this pull request Oct 28, 2023
Nexesenex pushed a commit to Nexesenex/croco.cpp that referenced this pull request Oct 28, 2023
olexiyb pushed a commit to Sanctum-AI/llama.cpp that referenced this pull request Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants