Skip to content

Conversation

Ithanil
Copy link
Contributor

@Ithanil Ithanil commented Aug 19, 2025

Purpose

This PR adds better handling of the following situations in harmony_utils:

  1. If input content has - for some reason - no text attribute, KeyError is avoided by using using .get() with empty string fallback.
  2. Sometimes GPT-OSS models will emit commentary messages without recipient, leading to .startswith() being called on NoneType. This PR adds checks for None. In that case ultimately still a ValueError will be raised, but with a more accurate error message.

@Ithanil Ithanil requested a review from aarnphm as a code owner August 19, 2025 06:14
@mergify mergify bot added the frontend label Aug 19, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 introduces graceful handling for two edge cases in harmony_utils.py. First, it prevents a KeyError by using .get() with a default value when parsing chat input content that may lack a 'text' attribute. Second, it avoids AttributeError when a message recipient is None by adding explicit None checks before calling string methods. The changes are correct, well-contained, and effectively improve the robustness of the code as described. I have no further suggestions.

Copy link

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging.

To run CI, PR reviewers can either: Add ready label to the PR or enable auto-merge.

🚀

@Ithanil Ithanil force-pushed the handle_edge_cases_harmoy branch from 7278a27 to 7f02c26 Compare August 19, 2025 06:29
Copy link
Collaborator

@heheda12345 heheda12345 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the fix.

@heheda12345 heheda12345 enabled auto-merge (squash) August 21, 2025 19:13
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Aug 21, 2025
@vllm-bot vllm-bot merged commit a11adaf into vllm-project:main Aug 28, 2025
35 of 37 checks passed
zhewenl pushed a commit to zhewenl/vllm that referenced this pull request Aug 28, 2025
Signed-off-by: Jan Kessler <[email protected]>
Co-authored-by: Chen Zhang <[email protected]>
Co-authored-by: Cyrus Leung <[email protected]>
@Ithanil Ithanil deleted the handle_edge_cases_harmoy branch August 31, 2025 04:55
zhewenl pushed a commit to zhewenl/vllm that referenced this pull request Sep 3, 2025
Signed-off-by: Jan Kessler <[email protected]>
Co-authored-by: Chen Zhang <[email protected]>
Co-authored-by: Cyrus Leung <[email protected]>
eicherseiji pushed a commit to eicherseiji/vllm that referenced this pull request Sep 9, 2025
Signed-off-by: Jan Kessler <[email protected]>
Co-authored-by: Chen Zhang <[email protected]>
Co-authored-by: Cyrus Leung <[email protected]>
FeiDaLI pushed a commit to FeiDaLI/vllm that referenced this pull request Sep 25, 2025
Signed-off-by: Jan Kessler <[email protected]>
Co-authored-by: Chen Zhang <[email protected]>
Co-authored-by: Cyrus Leung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend 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.

4 participants