-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add ToolAnnotations support in FastMCP and lowlevel servers #482
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
Conversation
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.
Just to check why we need dictionary and can be have annotations typed for users?
@ihrpr ok requested changes should be in (really good change, thanks a bunch) and up to date w/main. Only change to config is the warning suppression - still a mystery why everybody isn't seeing the test failures with starlette 0.27 and no suppression, but I'm happy to take it out of this PR if you think that's cleaner. |
yes please, let's take it out before merging. Separately let's look why there is that warning. I checked out this PR and run all tests without the warning suppression and all works fine (CI should also give us a signal) |
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.
Let's remove warning suppression before merging
) -> Tool: | ||
"""Create a Tool from a function.""" | ||
from mcp.server.fastmcp import Context | ||
from mcp.server.fastmcp.server import Context |
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.
what do we need to change this? I see the problem, but why do we even need it here?
on top of the file there is
if TYPE_CHECKING:
from mcp.server.fastmcp.server import Context
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.
There's this a little further down in the function:
if issubclass(param.annotation, Context):
(and having it up at the top introduces a circular dependency)
@ihrpr ok killed the warning suppression, can I ask you to reapprove, sorry 😬 |
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.
Looks good!
Summary
Test plan
uv run pytest
to verify changes work correctly🤖 Generated with Claude Code