-
Notifications
You must be signed in to change notification settings - Fork 2.8k
10935 Add Harmony Provider #10936
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
base: main
Are you sure you want to change the base?
10935 Add Harmony Provider #10936
Conversation
Review complete. Found 3 issues that should be addressed before merging:
Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues. |
| }, | ||
| ] | ||
|
|
||
| const converted = handler.convertToolsForOpenAI(tools) |
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.
The convertToolsForOpenAI method is protected in the parent class, so calling it directly on the handler instance will cause TypeScript compilation errors. The Cerebras tests demonstrate the correct pattern: create a test wrapper class that exposes the protected method via a public method. For example:
class TestHarmonyHandler extends HarmonyHandler {
public testConvertToolsForOpenAI(tools: any[] | undefined) {
return this.convertToolsForOpenAI(tools)
}
}Then use testHandler.testConvertToolsForOpenAI(tools) in tests.
Fix it with Roo Code or mention @roomote and request a fix.
| @@ -0,0 +1,113 @@ | |||
| import OpenAI from "openai" | |||
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.
This development/debug script and the accompanying test_harmony_toolcall_fix.py are placed in the project root. Consider either removing these files from the PR (they appear to be local development tools) or moving them to an appropriate location such as scripts/ or a documentation directory if they serve as examples.
Fix it with Roo Code or mention @roomote and request a fix.
| "name": "roo-code", | ||
| "packageManager": "pnpm@10.8.1", | ||
| "packageManager": "pnpm@10.28.1", | ||
| "engines": { |
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.
This packageManager version change from pnpm@10.8.1 to pnpm@10.28.1 appears unrelated to the Harmony provider feature. Consider reverting this change or submitting it as a separate PR to keep the feature scope focused.
Fix it with Roo Code or mention @roomote and request a fix.
Related GitHub Issue
Closes: #10935
Description
This PR fixes vLLM compatibility issues with the Harmony provider by addressing two key problems:
Removing unsupported
strictparameter: vLLM doesn't support OpenAI'sstrictmode parameter for function calling. The base class adds this parameter, but Harmony (which uses vLLM) needs it removed to prevent warnings and potential errors.Preserving optional parameters: The base class's
convertToolSchemaForOpenAImethod converts all properties to required fields for OpenAI strict mode. However, vLLM requires that optional parameters (especially those with nullable types like["string", "null"]) remain optional. This is critical for tools likebrowser_actionthat have optional parameters.Implementation details:
convertToolsForOpenAIoverride: Removes thestrictparameter from all function tools after conversion, ensuring vLLM compatibility.convertToolSchemaForOpenAIoverride:requiredarray structure["string", "null"]) to non-nullable types (e.g.,"string")requiredarray, keeping them optionaladditionalProperties: falseis set (required by OpenAI Responses API)This fix ensures that tools with optional parameters work correctly with vLLM backends while maintaining compatibility with the OpenAI-compatible API format.
Test Procedure
Unit Tests Added:
convertToolsForOpenAItests:strictparameter is removed from function toolsstrict: falsefrom parent)Optional parameter preservation test:
browser_actiontool with nullable optional parameters (url,coordinate)action) are in therequiredarrayRunning tests:
All existing tests continue to pass, and the new tests verify the vLLM compatibility fixes.
Pre-Submission Checklist
Screenshots / Videos
N/A - Backend implementation fix with no UI changes.
Documentation Updates
This is an internal implementation fix that improves compatibility with vLLM backends. No user-facing documentation changes are needed.
Additional Notes
Key design decisions:
Why override instead of modifying base class: The base class behavior is correct for OpenAI's strict mode, but Harmony uses vLLM which doesn't support strict mode. Overriding allows Harmony-specific behavior without affecting other providers.
Nullable type handling: The implementation converts
["string", "null"]to"string"while tracking which properties were nullable. This ensures therequiredarray only includes truly required fields, not optional nullable ones.Backward compatibility: The changes maintain full backward compatibility with existing tool schemas while fixing the vLLM-specific issues.
Testing considerations:
browser_actionuse case with nullable optional parametersGet in Touch
Contact me on Discord @ ajgreyling
Important
Adds Harmony provider to handle GPT-OSS models with vLLM compatibility, including new handler, settings, and tests.
HarmonyHandlerinharmony.tsto handle GPT-OSS models with vLLM compatibility.strictparameter inconvertToolsForOpenAI()to prevent vLLM warnings.convertToolSchemaForOpenAI().harmony.tstosrc/api/providers/.index.tsinsrc/api/andsrc/api/providers/to includeHarmonyHandler.provider-settings.tsto add Harmony provider settings.harmony.spec.ts,harmony-edge-cases.spec.ts, andharmony-roo-code-integration.spec.tsfor unit and integration tests.convertToolsForOpenAI()and optional parameter handling.ApiOptions.tsxandconstants.tsto include Harmony provider in settings.Harmony.tsxfor Harmony provider settings UI.README.mdto fix formatting.package.jsonto a newpnpmversion.This description was created by
for 698cd23. You can customize this summary. It will automatically update as commits are pushed.