-
-
Notifications
You must be signed in to change notification settings - Fork 11.8k
[ez] move harmony utils to parser folder #30117
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request refactors the codebase by moving harmony_utils.py into the vllm/entrypoints/openai/parser/ directory. This change helps to centralize parsing-related utilities for MCP, improving the project structure. The changes consist of file renames and corresponding import path updates across several files. I've reviewed the changes and they are correct and consistent with the refactoring goal. There are no functional changes, as expected. This is a good cleanup.
Signed-off-by: Andrew Xia <axia@fb.com>
a9fb82b to
c8c8b4c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| from vllm.entrypoints.openai.parser.harmony_utils import ( | ||
| get_encoding, | ||
| get_streamable_parser_for_assistant, | ||
| render_for_completion, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preserve compatibility for harmony_utils import
Moving harmony_utils under vllm.entrypoints.openai.parser removes the previous vllm.entrypoints.harmony_utils module, but tests/entrypoints/openai/test_response_api_with_harmony.py still imports the old path at line 729, so running the responses test suite now fails immediately with an ImportError. Either update that call site or add a compatibility shim at the old module path to keep the tests runnable.
Useful? React with 👍 / 👎.
yeqcharlotte
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Head branch was pushed to by a user without write access
|
sorry for my late review. But we need to update Lines 164 to 180 in 17eb25e
|
Purpose
Refactor so we centralize all the files related to parsing for MCP
Test Plan
No functional changes, existing unit tests should pass
Test Result