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

Jump to bottom

Fix partial unicode characters issue #4837

Merged
merged 10 commits into from
Dec 8, 2023
Merged

Fix partial unicode characters issue #4837

merged 10 commits into from
Dec 8, 2023

Conversation

Yiximail
Copy link
Contributor

@Yiximail Yiximail commented Dec 7, 2023

Checklist:

The issue link: #4828 (comment)

Causes of the issue:

Intercepting by token ids causes the tokenizer not to decode properly.

I fixed generate_reply_HF as well as exllama, exllamav2.
The rest loaders looks like it's generated by string interception rather than ids (Or maybe I don't fully understand them.)

About exllama and exllamav2:

image
It seems to decode all tokens entirely each time, so the new reply will always replace the old one.
It looks fine on both Webui and Api, but it does actually generate the wrong characters.
So I fixed them too.
image

@Yiximail
Copy link
Contributor Author

Yiximail commented Dec 7, 2023

May I ask something else?

Why check eos_token_ids before yield the text in generate_reply_HF but after in both exllama and exllamav2 ?

Sometimes it (using exllamav2_hf) doesn't output the last period properly, and I'm not sure if that's the reason.

@oobabooga
Copy link
Owner

The fix looks good. I'll test and merge it later.

Why check eos_token_ids before yield the text in generate_reply_HF but after in both exllama and exllamav2 ?

That's probably a bug. If you could move the eos_token check in those two up that would be helpful.

@oobabooga oobabooga merged commit 1c74b3a into oobabooga:dev Dec 8, 2023
@Yiximail
Copy link
Contributor Author

Yiximail commented Dec 8, 2023

The fix looks good. I'll test and merge it later.

Why check eos_token_ids before yield the text in generate_reply_HF but after in both exllama and exllamav2 ?

That's probably a bug. If you could move the eos_token check in those two up that would be helpful.

Thanks.
I just saw it. Sorry, I'm busy day today.

@oobabooga
Copy link
Owner

Np. I made some tests with Chinese text and it seems to be working well now. If you notice anything else weird, please feel free to submit a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Yiximail @oobabooga