8000 Always parse arguments from API tool calls by aymeric-roucher · Pull Request #1000 · huggingface/smolagents · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Always parse arguments from API tool calls #1000

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 17, 2025
Merged

Always parse arguments from API tool calls #1000

merged 3 commits into from
Mar 17, 2025

Conversation

aymeric-roucher
Copy link
Collaborator
@aymeric-roucher aymeric-roucher commented Mar 17, 2025 •
edited
Loading

Sometimes APIs return proper tool calls, but as a JSON string instead of a dictionary: this was not parsed as json using parse_json_if_needed, thus the usage of said args would fail.
This PR fixes it and adds a regression test.

Here is now how the API models returns are processed after this PR

if return has a built in tool call:
    if this tool call has incompletly parsed arguments like a string '{"location": "Paris"}' instead of the dict '{"location": "Paris"}' , parse them
    # Above line is what this PR #1000 adds
if return has no built-in tool call:
    try parsing the tool call from the return string
    if no tool call is found, fail

albertvillanova
albertvillanova approved these changes Mar 17, 2025
View reviewed changes
Copy link
Member
@albertvillanova albertvillanova left a comment

Choose a reason for hiding this comment

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

Thanks.

src/smolagents/models.py
get_tool_call_from_text(message.content, self.tool_name_key, self.tool_arguments_key)
]
for tool_call in message.tool_calls:
tool_call.function.arguments = parse_json_if_needed(tool_call.function.arguments)
Copy link
Member

Choose a reason for hiding this comment

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

I think I'm missing something: was not already called parse_json_if_needed inside get_tool_call_from_text? Why did not work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because get_tool_call_from_text was only called in case where no tool calls where found! This addition handles the case where a tool call was written by the api in message.tool_call, but its arguments are formatted as a JSON string instead of a proper dictionary

Copy link
Member

Choose a reason for hiding this comment

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

I found out why:

smolagents/src/smolagents/utils.py

Line 148 in 80e387f

json_data = json_blob[first_accolade_index : last_accolade_index + 1].replace('\\"', "'")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Normally the line you're pointing to should not be incriminated, since in the case we are handling here, the message was never parsed in any way on smolagents side!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cf my comment below.

Copy link
Member

Choose a reason for hiding this comment

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

I think the the root cause is the line I pointed out. Let me explain:

  • 2 lines above your new parse_json_if_needed, the code calls get_tool_call_from_text
  • inside get_tool_call_from_text, the function parse_json_if_needed was already called
    • why it did not work, as expected?
    • because previous call to parse_json_blob replaced double-quote with single-quote: the line I pointed out

aymeric-roucher
aymeric-roucher commented Mar 17, 2025
View reviewed changes
tests/test_agents.py
mock_response.choices[0].message.tool_calls = [
ChatMessageToolCall(
function=ChatMessageToolCallDefinition(
name="weather_api", arguments='{"location": "Paris", "date": "today"}'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@albertvillanova the crux of the PR is this edge case presented here: the chat message arguments can be a string instead of a dictionary, thus the need to parse them if so.

Copy link
Member

Choose a reason for hiding this comment

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

I think this test is misleading: the str should have been fixed to dict during the parsing, BEFORE being assigned to ChatMessageToolCallDefinition

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes but here there was no parsing since we're directly using the tool answer from API!

Copy link
Member

Choose a reason for hiding this comment

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

I'm working on that direction... 😉

@albertvillanova albertvillanova mentioned this pull request Mar 17, 2025
Merged
@albertvillanova
Copy link
Member
albertvillanova commented Mar 17, 2025 •
edited
Loading

I opened a PR to address the issue I commented above: #1000 (comment)

Not exactly the same issue you address here, but related.
Anyway, IMO, I think the parsing should always be done.

@aymeric-roucher aymeric-roucher merged commit b8bd0c5 into main Mar 17, 2025
4 checks passed
@albertvillanova albertvillanova deleted the fix-argument-parsing-api branch March 31, 2025 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@albertvillanova albertvillanova albertvillanova approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aymeric-roucher @albertvillanova
0