Skip to content

Conversation

@daniel-salib
Copy link
Contributor

@daniel-salib daniel-salib commented Dec 1, 2025

Purpose

Refactor parse_output_message() in harmony_utils.py to improve
code organization and maintainability by extracting parsing
logic into focused helper functions.

Changes

Split the monolithic parse_output_message() function into four
specialized helper functions:

  • _parse_browser_tool_call() - Handles browser tool calls
    (search, open, find)
  • _parse_function_call() - Handles custom function calls
  • _parse_reasoning_content() - Handles reasoning/analysis
    content
  • _parse_final_message() - Handles final output messages

Testing

  • Existing harmony utils tests pass
  • No functional changes - pure refactoring

@chatgpt-codex-connector
Copy link

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.

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 refactors the parse_output_message function in harmony_utils.py by extracting logic into several focused helper functions. The changes significantly improve code organization and maintainability. The refactoring is well-done, but I've identified a potential KeyError in the new _parse_browser_tool_call function due to an unsafe dictionary access. I've provided a suggestion to fix this. Apart from that, the changes look good.

@daniel-salib daniel-salib force-pushed the pr1.5-parse-refactor branch 2 times, most recently from f12b817 to 0b175cc Compare December 1, 2025 22:58
@daniel-salib
Copy link
Contributor Author

thanks for the review @qandrew

@robertgshaw2-redhat @chaunceyjiang @yeqcharlotte

please let me know if you have any feedback or if this good to approve/merge

Copy link
Collaborator

@chaunceyjiang chaunceyjiang left a comment

Choose a reason for hiding this comment

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

Thanks~

@github-project-automation github-project-automation bot moved this from To Triage to Ready in gpt-oss Issues & Enhancements Dec 3, 2025
@chaunceyjiang chaunceyjiang added the ready ONLY add when PR is ready to merge/full CI is needed label Dec 3, 2025
@daniel-salib daniel-salib force-pushed the pr1.5-parse-refactor branch 2 times, most recently from cea3600 to d8ebcb0 Compare December 3, 2025 07:22
@DarkLight1337 DarkLight1337 merged commit 404fc4b into vllm-project:main Dec 4, 2025
47 checks passed
PatrykSaffer pushed a commit to PatrykSaffer/vllm that referenced this pull request Dec 4, 2025
charlotte12l pushed a commit to charlotte12l/vllm that referenced this pull request Dec 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

frontend gpt-oss Related to GPT-OSS models ready ONLY add when PR is ready to merge/full CI is needed

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants