Skip to content
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

Possible bugs in both tools examples (or at least something to document) #434

Open
jdalegonzalez opened this issue Feb 4, 2025 · 0 comments

Comments

@jdalegonzalez
Copy link

The current code in the tools examples looks like:

if response.message.tool_calls:
  # There may be multiple tool calls in the response
  for tool in response.message.tool_calls:
    # Ensure the function is available, and then call it
    if function_to_call := available_functions.get(tool.function.name):
      print('Calling function:', tool.function.name)
      print('Arguments:', tool.function.arguments)
      output = function_to_call(**tool.function.arguments)
      print('Function output:', output)
    else:
      print('Function', tool.function.name, 'not found')

# Only needed to chat with the model using the tool call results
if response.message.tool_calls:
  # Add the function response to messages for the model to use
  messages.append(response.message)
  messages.append({'role': 'tool', 'content': str(output), 'name': tool.function.name})

  # Get final response from model with function outputs
  final_response = chat('llama3.1', messages=messages)
  print('Final response:', final_response.message.content)

else:
  print('No tool calls returned from model')

This code raises the following questions:

  • Why test "if response.message.tool_calls" twice? Could this be refactored to only need one test? Something like
if response.message.tool_calls:
   messages.append(response.message)
   # There may be multiple calls in the response
   for tool in ...
else
   print('No tool calls returned from model')
  • If that refactor isn't OK, it would be good to document why testing .message.tool_calls twice is necessary
  • If there are multiple tool calls, isn't the output getting thrown away for every tool call except the last one? If that's OK, why is it OK? Should the loop actually be something like:
 for tool in response.message.tool_calls:
    # Ensure the function is available, and then call it
    if function_to_call := available_functions.get(tool.function.name):
      print('Calling function:', tool.function.name)
      print('Arguments:', tool.function.arguments)
      output = function_to_call(**tool.function.arguments)
      print('Function output:', output)
      messages.append({'role': 'tool', 'content': str(output), 'name': tool.function.name})
    else:
      print('Function', tool.function.name, 'not found')
  • If it's OK for output to get tossed, documenting why that's OK would be helpful.

For what it's worth, I'm happy to make the refactors and submit a pull request, I just didn't know if I was missing something. (Since I'm VERY new to ollama, tools, etc...)

@jdalegonzalez jdalegonzalez changed the title Possible but in both tools examples (or at least something to document) Possible bugs in both tools examples (or at least something to document) Feb 4, 2025
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

No branches or pull requests

1 participant