Skip to content

Fix GLM tool-call argument tag parsing - #59

Open
aarongiuffre wants to merge 2 commits into
NVIDIA:mainfrom
aarongiuffre:fix/glm-tool-argument-tags
Open

aarongiuffre wants to merge 2 commits into
NVIDIA:mainfrom
aarongiuffre:fix/glm-tool-argument-tags

Conversation

@aarongiuffre

Copy link
Copy Markdown

Summary

Fix GLM tool-call argument parsing that could fail with: GLM tool call requires an argument key.

Validation

  • Built the updated Flare binary on the NVIDIA Spark environment.
  • Verified matching binary hashes across both Spark nodes.
  • Ran a controlled GLM-5.3-Flash NVFP4 QAD chat completion with tool_choice=required.
  • Confirmed the OpenAI-compatible API returned a lookup_weather tool call with valid JSON arguments.

Testing notes

The controlled tool-call test passed. Additional regression coverage for other argument encodings and streaming responses would be useful.

@original-el8 original-el8 added area:api Serving API, protocol encoding, tool parsing, and client-visible behavior. triage:design-review Needs a maintainer scope, architecture, or contract decision before implementation or integration. labels Nov 5, 2026
@original-el8

Copy link
Copy Markdown
Collaborator

This PR and #179 need a shared tool-argument semantics decision. This diff adds array coercion (including comma splitting and wrapping strings in arrays) and richer parse errors. #179 deliberately retains generated off-schema values without coercion, with streaming coverage for #39. They are overlapping proposals, not interchangeable fixes.

Marked triage:design-review. Decide the non-strict argument contract first, then retain the necessary parser change and regression coverage. The argument-key failure named in this PR's title also needs a focused reproducer; its diagnostic changes alone do not establish that the failure is fixed.

@original-el8 original-el8 added model:glm-5.3 Affected recipe or documented reproducer: glm-5.3. Impact may be broader. triage:verify-for-closure May already be fixed. Submitter: retest cited fix; close if resolved or report remaining symptoms. and removed triage:design-review Needs a maintainer scope, architecture, or contract decision before implementation or integration. labels Nov 8, 2026
@original-el8

Copy link
Copy Markdown
Collaborator

Marked triage:verify-for-closure after #179 merged as 9d1e0137. Main now retains GLM tool arguments that do not match declared schema types, addressing the stream-abort problem that motivated coercion proposals.

This PR also adds comma-splitting/string-to-array coercion and diagnostics, which are different semantics from retaining the generated value. Re-establish a failing input on main before carrying those changes forward; a successful controlled tool call alone does not show that they remain necessary. Close the proposal if its target failure is resolved, or narrow it to the remaining demonstrated defect.

@aarongiuffre GitHub reports merge conflicts with main, confirmed locally with git merge-tree for PR head 1f38c71d0dde against main 97cfeb0001f1. Conflicting paths:

  • helios/src/output/tagged.rs

Please reconcile the branch with main and rerun the checks affected by the resolution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:api Serving API, protocol encoding, tool parsing, and client-visible behavior. model:glm-5.3 Affected recipe or documented reproducer: glm-5.3. Impact may be broader. triage:verify-for-closure May already be fixed. Submitter: retest cited fix; close if resolved or report remaining symptoms.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants