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

Fix rid state map leak + Refractor .finished #505

Merged
merged 3 commits into from
Jun 7, 2024

Conversation

Qubitium
Copy link
Contributor

@Qubitium Qubitium commented Jun 5, 2024

  1. HIGH IMPACT: @ZhouXingg Fixed a bug/memory leak of rid state map when stream=False. Rid state map cleanup code after yield is not executed since the receiver of the generator early returns and never calls it again. Qubitium@3407263
  2. MEDIUM IMPACT: The rest of the changes deal with merging all structures/properties related to req.finished from a simple bool into a BaseFinishReason object.
  3. LOW IMPACT: added type hints for some zmq event receivers so ide/coders can actually see real type even if runtime doesn't use them.

Reason for .finished refractor:

  • Consolidate the following into a single finished_reason state/property: output_hit_stop_str, hit_stop_str, finish_reason, finished. They are all related to the same Finished state. Having 4 separate vars separate and passing them around makes the code more complex than necessary.
  • FinishReason.EOS enum is now a more generic FINISH_MATCHED_TOKEN object which now can handle any future changes where any token/tokens are matched, not just for single EOS. For example, EOT tokens in structured chat models in future code. Some existing chat models already have multiple EOS defined in model/config.json. This allows the FinishReason object to hold the actual token hit instead of fixed enum which would requires another eos_token var in the current structure to know which of the N possible EOS token was hit.

TESTS:

Tests are performed with vllm 0.4.2 since 0.4.3 requires another PR #487 :

  • PASS: Matched eos
  • PASS: Matched stop_str
  • PASS: Length reached
  • PASS: dp == 2

@Qubitium Qubitium changed the title Fix rid stat leak + Refractor .finished Fix rid state map leak + Refractor .finished Jun 5, 2024
@merrymercy merrymercy merged commit f70f725 into sgl-project:main Jun 7, 2024
@Qubitium Qubitium deleted the refractor-finished branch June 7, 2024 21:23
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.

2 participants