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

when I want load below mcp server milvus tools to smolagents. I got errors: SyntaxError: non-default argument follows default argum #13

Open
bladexxx opened this issue Mar 17, 2025 · 13 comments
Assignees
Labels
bug Something isn't working

Comments

@bladexxx
Copy link

bladexxx commented Mar 17, 2025

hello @grll hope you are doing well!

when I want load below mcp server milvus tools to smolagents. I got below errors
https://github.com/zilliztech/mcp-server-milvus

File "/webapp/anaconda3/lib/python3.11/site-packages/mcpadapt/core.py", line 198, in
self.adapter.adapt(partial(_sync_call_tool, tool.name), tool)
File "/webapp/anaconda3/lib/python3.11/site-packages/mcpadapt/smolagents_adapter.py", line 120, in adapt
exec(class_template, namespace)
File "", line 9
def forward(self, collection_name, vector, vector_field="vector", limit=5, output_fields, metric_type="COSINE", filter_expr) -> str:

SyntaxError: non-default argument follows default argument

@grll
Copy link
Owner

grll commented Mar 17, 2025

Hi @bladexxx thanks for using mcpadapt let me have a look

@grll grll added the bug Something isn't working label Mar 17, 2025
@grll
Copy link
Owner

grll commented Mar 17, 2025

Actually I can already see something is wrong in the forward above:

def forward(self, collection_name, vector, vector_field="vector", limit=5, output_fields, metric_type="COSINE", filter_expr) -> str:

filter_expr is a positional arguments after kwargs which is not allowed in python

@grll
Copy link
Owner

grll commented Mar 17, 2025

hmm it seems to come from the fix we did with optional parameters, when there is = None we don't specify the value in the forward hence if you have = None kwargs after other kwargs you have the bug above. I will write a fix ASAP

@grll grll self-assigned this Mar 17, 2025
@grll
Copy link
Owner

grll commented Mar 17, 2025

to be honest this milvus-mcp-server is doing something a bit wierd here:

https://github.com/zilliztech/mcp-server-milvus/blob/5b38812f069ab8e8dd040668ddf7cd1a838553c9/src/mcp_server_milvus/server.py#L824

            types.Tool(
                name="milvus-vector-search",
                description="Perform vector similarity search on a collection",
                inputSchema={
                    "type": "object",
                    "properties": {
                        "collection_name": {
                            "type": "string",
                            "description": "Name of the collection to search"
                        },
                        "vector": {
                            "type": "array",
                            "items": {"type": "number"},
                            "description": "Query vector"
                        },
                        "vector_field": {
                            "type": "string",
                            "description": "Field containing vectors to search",
                            "default": "vector"
                        },
                        "limit": {
                            "type": "integer",
                            "description": "Maximum number of results",
                            "default": 5
                        },
                        "output_fields": {
                            "type": "array",
                            "items": {"type": "string"},
                            "description": "Fields to include in results"
                        },
                        "metric_type": {
                            "type": "string",
                            "description": "Distance metric (COSINE, L2, IP)",
                            "default": "COSINE"
                        },
                        "filter_expr": {
                            "type": "string",
                            "description": "Optional filter expression"
                        }
                    },
                    "required": ["collection_name", "vector"]
                }
            ),

that's how they define the tool as you can see filter_expr and vector_field is not provided any default but come after other arguments with defaults which is causing the issue. This way of defining tools is really odd I don't understand why aren't they using fastMCP syntax which is much easier to read and would also prevent the issue to arise in the first place

@grll
Copy link
Owner

grll commented Mar 17, 2025

@bladexxx I would ask them to fix based on the above if it's a recurring problem across other mcp servers as well I will consider implementing a fix

@bladexxx
Copy link
Author

Thanks @grll !
I am using smolagents to construct our AI agents, what do you think about smolagents and mcp? can we choose it as our enterprise AI Agent solution, Or is it just not mature yet?

@stephen37
Copy link

@bladexxx I would ask them to fix based on the above if it's a recurring problem across other mcp servers as well I will consider implementing a fix

Hey there, I am the author of the mcp on Milvus, you're right, I actually hadn't seen the FastMCP implementation so I went on a lower level 🙈

I created a PR to change this, do you think this would fix the problem? zilliztech/mcp-server-milvus#3

@dwelve
Copy link

dwelve commented Mar 19, 2025

I encountered the same issue with tavily-mcp, which defines an argument without a default value after arguments with defaults.

https://github.com/tavily-ai/tavily-mcp/blob/14451043edccda987dbb6ff39d6b0876d44328db/src/index.ts#L119

FWIW, Claude Desktop and Cline can interface with tavily-mcp without issue.


Edit: so I was able to resolve the issue with the code generation in order to make tavily-mcp work, by re-ordering the Python function argument list and setting None/nullable to optional parameters with no default value. The actual call to the MCP tool is with a dict so I don't see why we need to assume the tool description has any particular ordering of arguments. Another issue is SmolAgentsTool.name needs to be sanitized to a valid python function name (tavily-search -> tavily_search).

@grll
Copy link
Owner

grll commented Mar 19, 2025

Thanks @grll ! I am using smolagents to construct our AI agents, what do you think about smolagents and mcp? can we choose it as our enterprise AI Agent solution, Or is it just not mature yet?

Hey, to be honest I think it's fairly new I wouldn't call it Enterprise ready yet. However one of the advantage of that is that it's also a fairly small code base that can easily be customized to your needs. From my experience in various enterprise settings I think it reallly depends on the team that you have and that would have to fix issues arising. Is your team ready to spend some time diving into smolagents codebase to fix issues or add some enterprise feature... Also in my use of agentic AI, it is sometimes suprisingly good results but is also very prone to error, as much as possible I would recommend staying the "AI workflow" path unless the flow of actions need to be determined by the AI agent: https://www.anthropic.com/engineering/building-effective-agents

@grll
Copy link
Owner

grll commented Mar 19, 2025

@bladexxx I would ask them to fix based on the above if it's a recurring problem across other mcp servers as well I will consider implementing a fix

Hey there, I am the author of the mcp on Milvus, you're right, I actually hadn't seen the FastMCP implementation so I went on a lower level 🙈

I created a PR to change this, do you think this would fix the problem? zilliztech/mcp-server-milvus#3

always good to go down the low level first before going the abstract way to understand things :)

As far as I see this should fix the issue as milvus tools are now defined as python function which prevents the creation of args without default after args with default

@grll
Copy link
Owner

grll commented Mar 19, 2025

I encountered the same issue with tavily-mcp, which defines an argument without a default value after arguments with defaults.

https://github.com/tavily-ai/tavily-mcp/blob/14451043edccda987dbb6ff39d6b0876d44328db/src/index.ts#L119

FWIW, Claude Desktop and Cline can interface with tavily-mcp without issue.

Edit: so I was able to resolve the issue with the code generation in order to make tavily-mcp work, by re-ordering the Python function argument list and setting None/nullable to optional parameters with no default value. The actual call to the MCP tool is with a dict so I don't see why we need to assume the tool description has any particular ordering of arguments. Another issue is SmolAgentsTool.name needs to be sanitized to a valid python function name (tavily-search -> tavily_search).

thanks for reporting we might indeed need to support that then. Other MCP clients that you mentioned are working because they don't need to redefine the tool as a python functions but tools in smolagents and often other frameworks are python function so to create the tool we need to wrap it inside a python function. We could use a dict as you mention as only argument but then we lose plenty of information that the model might use to make the call like the type etc and would also change the error message the model would receive in case of wrongly formed arguments. Another way would be to reorder the arguments

@stephen37
Copy link

Hey @bladexxx, I merged the PR of Milvus MCP implementation that should make it work with MCPAdapt, let me know if you encounter some problems.

@bladexxx
Copy link
Author

@stephen37 / @grll thank you all. I'll try it again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants