[Misc] FlattenLogprobs -> FlatLogprobs #28335
Conversation
Signed-off-by: Zhuohan Li <zhuohan123@gmail.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the codebase by renaming FlattenLogprobs to FlatLogprobs for better clarity. The changes are mostly correct and applied across the necessary files. However, to make this refactoring more complete and consistent, I've identified a few areas for improvement. Specifically, some test function names in tests/test_logprobs.py still use the old flatten_logprobs naming. Additionally, the environment variable VLLM_FLATTEN_LOGPROBS in vllm/envs.py has not been renamed, creating an inconsistency with the new class name. My review includes suggestions to address these points for better code maintainability.
tests/test_logprobs.py
Outdated
| } | ||
|
|
||
|
|
||
| def test_flatten_logprobs_append() -> None: |
There was a problem hiding this comment.
For consistency with the class rename from FlattenLogprobs to FlatLogprobs, this test function should be renamed to test_flat_logprobs_append.
| def test_flatten_logprobs_append() -> None: | |
| def test_flat_logprobs_append() -> None: |
tests/test_logprobs.py
Outdated
| assert logprobs.decoded_tokens == ["10", "20", "30", "40", "50", "60"] | ||
|
|
||
|
|
||
| def test_flatten_logprobs_extend() -> None: |
There was a problem hiding this comment.
To maintain consistency with the refactoring, this test function should be renamed to test_flat_logprobs_extend.
| def test_flatten_logprobs_extend() -> None: | |
| def test_flat_logprobs_extend() -> None: |
tests/test_logprobs.py
Outdated
| assert logprobs.decoded_tokens == ["40", "50", "60", "10", "20", "30", "10"] | ||
|
|
||
|
|
||
| def test_flatten_logprobs_access() -> None: |
There was a problem hiding this comment.
Consistent with the rename of FlattenLogprobs, this function should be renamed to test_flat_logprobs_access.
| def test_flatten_logprobs_access() -> None: | |
| def test_flat_logprobs_access() -> None: |
vllm/envs.py
Outdated
| # Flag to enable FlatLogprobs whose GC overhead is significantly smaller than | ||
| # the original list[dict[int, Logprob]] approach. | ||
| # After enabled, PromptLogprobs and SampleLogprobs would populated as | ||
| # FlattenLogprobs. | ||
| # FlatLogprobs. | ||
| "VLLM_FLATTEN_LOGPROBS": lambda: bool(int(os.getenv("VLLM_FLATTEN_LOGPROBS", "0"))), |
There was a problem hiding this comment.
While the comment and class name are updated to FlatLogprobs, the environment variable is still named VLLM_FLATTEN_LOGPROBS. This creates an inconsistency. To complete the refactoring, consider renaming the environment variable to VLLM_FLAT_LOGPROBS. This would be a breaking change for users who set this variable. If backward compatibility is a concern, you could support both variables for a transition period or add a note to the comment explaining why the old name is kept.
Signed-off-by: Zhuohan Li <zhuohan123@gmail.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the codebase by renaming FlattenLogprobs to the more succinct FlatLogprobs. The change has been applied consistently across all relevant files, including class definitions, variable names, function names, environment variables, and associated comments and documentation. The refactoring is clean and achieves its goal of improving naming consistency. I have no further comments.
github-actions
bot
added
the
ready
Purpose
Fix #28171 (comment)
Test Plan
All existing tests should pass.
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.