Skip to content

[data] fix LFM2 tool extractor dropping positional arguments#10594

Open
he-yufeng wants to merge 1 commit into
hiyouga:mainfrom
he-yufeng:fix/lfm2-positional-tool-args
Open

[data] fix LFM2 tool extractor dropping positional arguments#10594
he-yufeng wants to merge 1 commit into
hiyouga:mainfrom
he-yufeng:fix/lfm2-positional-tool-args

Conversation

@he-yufeng

Copy link
Copy Markdown

What does this PR do?

LFM2ToolUtils.tool_extractor parses a generated Pythonic tool call and only reads node.keywords, so any positional argument is silently dropped:

from llamafactory.data.tool_utils import LFM2ToolUtils

LFM2ToolUtils.tool_extractor("<|tool_call_start|>[process(1, 2, 3)]<|tool_call_end|>")
# [FunctionCall(name='process', arguments='{}')]      <- all three args gone

LFM2ToolUtils.tool_extractor("<|tool_call_start|>[process(1, size=10)]<|tool_call_end|>")
# [FunctionCall(name='process', arguments='{"size": 10}')]   <- positional 1 gone

That turns a malformed call into a clean-looking one with missing arguments, which then gets executed with the wrong inputs. Positional values can't be mapped to the tool's named parameters at this point (there's no schema here), so this PR bails to the raw content instead — the same fallback the method already uses for an unparseable call or a bad argument value. This matches the recent SeedToolUtils fix (#10408) where the extractor returns content rather than emitting a broken call.

Before submitting

  • Did you read the contributor guideline?
  • Did you write any new necessary tests? (test_lfm2_tool_extractor_positional_args in tests/data/test_formatter.py, covering positional-only and mixed positional/keyword calls)

Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

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 updates the LFM2 tool extractor to bail to raw content when positional arguments are present, preventing them from being silently dropped, and adds corresponding unit tests. The review feedback suggests extending this safety check to also bail when double-starred keyword arguments (**kwargs) are present, as they result in None keys in the AST and can cause serialization errors.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +873 to +874
if node.args:
return content

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

In Python's AST, double-starred keyword arguments (e.g., **kwargs) are represented as ast.keyword nodes where arg is None. If a tool call contains **kwargs, keyword.arg will be None, which would lead to setting None as a key in args_dict and subsequently causing a TypeError during json.dumps (or producing an invalid JSON key "null"). To prevent this, we should also bail to the raw content if any keyword argument has arg is None.

Suggested change
if node.args:
return content
if node.args or any(keyword.arg is None for keyword in node.keywords):
return content

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant