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

Optional argument support for smolagents #11

Merged
merged 3 commits into from
Mar 5, 2025

Conversation

murawakimitsuhiro
Copy link
Contributor

@murawakimitsuhiro murawakimitsuhiro commented Mar 5, 2025

Fixes This PR resolves the error in issue #10 (related huggingface/smolagents#626 )

Error output

k: {"type": v["type"], "description": v.get("description", "")}
KeyError: 'type'

The fix addresses cases where arguments are provided using the anyOf schema.

Changed

  • Implemented support for handling functions with optional arguments when defined as Tools (primarily using FastMCP)
  • Added tests for the modified functionality

Implementation approach for optional and default arguments

I've addressed three scenarios related to issue.

  1. Optional arguments with None as default: text: str | None = None
  2. Required arguments with default values: text: str = "hello"
  3. Optional arguments without default values: text: str | None

The current implementation outputs the following forward function for each case

# case1
def forward(self, text=None) -> str:
   ...
   
# case2
def forward(self, text="hello") -> str:
   ...

# case3
def forward(self, text) -> str:
   ...

For optional arguments without default values (case 3), I chose to maintain the existing implementation approach, requiring CodeAgent to explicitly specify these values rather than automatically providing None. But I'm open to discussion on this design choice.

Thank you for reviewing this PR!

@murawakimitsuhiro murawakimitsuhiro marked this pull request as draft March 5, 2025 11:43
@murawakimitsuhiro murawakimitsuhiro changed the title Feature/mcp tool optional args Optional argument support for smolagents Mar 5, 2025
@murawakimitsuhiro murawakimitsuhiro marked this pull request as ready for review March 5, 2025 12:30
@grll
Copy link
Owner

grll commented Mar 5, 2025

@murawakimitsuhiro looks good to me at first glance I will pull it and test locally

@grll
Copy link
Owner

grll commented Mar 5, 2025

@murawakimitsuhiro just the lint complaining could you run ruff ?

@grll
Copy link
Owner

grll commented Mar 5, 2025

with regard to your comment:

For optional arguments without default values (case 3), I chose to maintain the existing implementation approach, requiring CodeAgent to explicitly specify these values rather than automatically providing None. But I'm open to discussion on this design choice.

case 3 in your example is not optional it's a regular positional argument isn't it? So in my view it makes sense to fail rather than defaulting to None.

In my view, case 3 is a positional argument which is not optional even though it might take None as a value so I agree.

Copy link
Owner

@grll grll left a comment

Choose a reason for hiding this comment

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

looks good to me I would just add case 3 to the tests as well and run ruff to pass the lint

if text is None:
return "No input provided"
return f"Echo: {text}"

Copy link
Owner

Choose a reason for hiding this comment

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

could we also add here the case 3? with text: str | None

@grll
Copy link
Owner

grll commented Mar 5, 2025

@murawakimitsuhiro all good, great work thanks a lot!

@grll grll merged commit 88fac67 into grll:main Mar 5, 2025
3 checks passed
@murawakimitsuhiro
Copy link
Contributor Author

@grll Thank you very much for the swift reply!

@kjozsa
Copy link

kjozsa commented Mar 5, 2025

I just tested release 0.0.15 and it does solve my issue as well. Thank you to both!

@murawakimitsuhiro murawakimitsuhiro deleted the feature/mcp-tool-optional-args branch March 6, 2025 07:10
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.

3 participants