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

[Bugfix] Fix bug in detokenizer.py #8112

Closed
wants to merge 2 commits into from
Closed

Conversation

cafeii
Copy link

@cafeii cafeii commented Sep 3, 2024

Fix the bug where the length of prev_tokens and next_iter_tokens in detokenizer.py would grow exponentially in some cases.

The original code assign the reference of next_iter_tokens to prev_tokens directly, making them point to a same object. When prev_tokens is extended by next_iter_tokens, both of their length will be doubled, which makes the length grows exponentially.

ps: I didn't find any issue that directly point out this bug.

Fix the bug when some cases the length of prev_tokens and next_iter_tokens grow exponentially
Copy link

github-actions bot commented Sep 3, 2024

👋 Hi! Thank you for contributing to the vLLM project.
Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which consists a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of default ones by unblocking the steps in your fast-check build on Buildkite UI.

Once the PR is approved and ready to go, please make sure to run full CI as it is required to merge (or just use auto-merge).

To run full CI, you can do one of these:

  • Comment /ready on the PR
  • Add ready label to the PR
  • Enable auto-merge.

🚀

@cafeii
Copy link
Author

cafeii commented Sep 3, 2024

/ready

@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Sep 3, 2024
@zifeitong
Copy link
Contributor

Do you have a reproducer?

#5919 is almost the same.

#6223 is also related.

@cafeii
Copy link
Author

cafeii commented Sep 4, 2024

reproduce.zip

  • vllm: 0.5.1
  • transformers 4.45.0.dev0
  • tokenizers 0.19.1

If successfully reproduced, it will slow down at the 8th sample, and completely stuck at 9th sample.
The bug's behavior is similar to #5872, but I didn't encounter the same bug as #5872 while reproducing it.
#6223 can also fix the bug.
#5919 is almost the same as this PR.

@njhill
Copy link
Member

njhill commented Oct 22, 2024

Addressed by #5919

@njhill njhill closed this Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready ONLY add when PR is ready to merge/full CI is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants