-
Notifications
You must be signed in to change notification settings - Fork 14
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
Handle oneOf schema validation in _generate_tool_inputs for SmolAgents #14
base: main
Are you sure you want to change the base?
Conversation
Fix: Handle oneOf schema validation in _generate_tool_inputs
Hi there thanks a lot for reporting the issue and opening a PR, I will review ASAP |
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.
also one test with anyOf or oneOf and several type would be nice to add to make sure it works and we don't regress later
# Try anyOf | ||
anyOf = v.get("anyOf") | ||
if anyOf and len(anyOf) > 0: | ||
type_value = anyOf[0].get("type") |
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.
I think it might be good time to fix the anyOf as well if I understand correctly with anyOf we should create a set of type with all the values and then the type_value would become "type_1 | type_2 ..."
|
||
# If still None, try oneOf | ||
if type_value is None: | ||
oneOf = v.get("oneOf") |
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.
actually its same for the oneOf as we can't really express the oneOf condition in python type annotations
type_value = oneOf[0].get("type") | ||
|
||
# Default to "string" if we still couldn't find a type | ||
if type_value is None: |
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.
maybe we should log a warning in that case as I am not sure this is always going to work to default to string?
@tisDDM let me know if you have some time, willingness to make the changes otherwise happy to do that for you. |
@grll Sure. I will have a look this week. |
@grll I gave the implementation with the Union a try. But unfortunately it breaks with the AUTHORIZED_TYPES in Smolagents. I would also appreciate this clean solution, but I think It could get a bit complicated because we'll need to do a PR to Smolagents changing the authorized type so that also Unions of these are authorized. On the other hand - I think - it is no big issue going just with one type. The LLMs/Agents are working sort of text-only at this interface anyway. What do you think? |
While using the firecrawl-mcp-server with Smolagents I found that mcpadapt could not generate all the inputs from the definitions the server sends.
I tracked down the problem to the _generate_tool_inputs function where I think some details in the implementation where missing. I asked my friend Claude 3.7 to update the lines. Checked manually and performed a few tests.