Skip to content

satisfy pyright untyped decorators in mcp.server.fastmcp.server#181

Merged
dsp-ant merged 2 commits intomodelcontextprotocol:mainfrom
zzstoatzz:update-fastmcp-server-typing
Feb 4, 2025
Merged

satisfy pyright untyped decorators in mcp.server.fastmcp.server#181
dsp-ant merged 2 commits intomodelcontextprotocol:mainfrom
zzstoatzz:update-fastmcp-server-typing

Conversation

@zzstoatzz
Copy link
Copy Markdown
Contributor

@zzstoatzz zzstoatzz commented Jan 30, 2025 •
edited
Loading

Motivation and Context

using a .vscode/settings.json like this

{
    "python.analysis.typeCheckingMode": "strict"
}

the following example throws squiggles on main

from mcp.server.fastmcp import FastMCP

mcp = FastMCP("YouHaveBeenGoosed")

@mcp.tool()
def give_me_a_goose() -> str:
    return "goose"

if __name__ == "__main__":
    mcp.run()

image

How Has This Been Tested?

observing a marked lack of squiggles on this branch

Breaking Changes

no

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Type nit

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

i purposefully wasn't so prescriptive with _Function, happy to be more specific if needed

if you're wondering how I got here: PrefectHQ/marvin#1042

zzstoatzz
zzstoatzz commented Jan 30, 2025
View reviewed changes
.gitignore Outdated
Copy link
Copy Markdown
Contributor Author

@zzstoatzz zzstoatzz Jan 30, 2025

Choose a reason for hiding this comment

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

omit artifacts from vscode-based editors

Copy link
Copy Markdown
Member

@dsp-ant dsp-ant Jan 31, 2025

Choose a reason for hiding this comment

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

Terribel. Feel free to keep it. I dont use vscode myself but i am sure plenty people do.

@zzstoatzz zzstoatzz changed the title satisfy pyright complaining about untyped decorators in mcp.server.fastmcp.server.py satisfy pyright untyped decorators in mcp.server.fastmcp.server.py Jan 30, 2025
@zzstoatzz zzstoatzz changed the title satisfy pyright untyped decorators in mcp.server.fastmcp.server.py satisfy pyright untyped decorators in mcp.server.fastmcp.server Jan 30, 2025
@zzstoatzz zzstoatzz marked this pull request as ready for review January 30, 2025 06:07
dsp-ant
dsp-ant requested changes Jan 31, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@dsp-ant dsp-ant left a comment

Choose a reason for hiding this comment

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

Thank you. This is great! Throwing this back into you queue for two pieces:

  • Shall we rename _Function to something that is exported?
  • How can we check in CI/pre-commit that we won't break this?

src/mcp/server/fastmcp/server.py Outdated
Copy link
Copy Markdown
Member

@dsp-ant dsp-ant Jan 31, 2025

Choose a reason for hiding this comment

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

Any reason to use the _. I wonder if a caller wants to somehow wrap add_tool and type it, they might want to reuse the type?

@zzstoatzz zzstoatzz force-pushed the update-fastmcp-server-typing branch from 05bad54 to a7a69e9 Compare February 3, 2025 20:30
@zzstoatzz
Copy link
Copy Markdown
Contributor Author

zzstoatzz commented Feb 3, 2025 •
edited
Loading

@dsp-ant - I've updated the PR to move/rename _Function as AnyFunction in mcp.types - lmk what you think!

as for the question of catching typing regressions in CI, are you suggesting something in addition to the pyright running in shared.yml? in prefect we have some type tests like this (leveraging a mypy pytest plugin) as well as a pyright CI job using a config file.

Not sure what'd be preferable to you, but happy to take a shot at it (here or follow up PR) if one of them sounds attractive!

@zzstoatzz zzstoatzz force-pushed the update-fastmcp-server-typing branch from a7a69e9 to ca06001 Compare February 4, 2025 17:11
@dsp-ant
Copy link
Copy Markdown
Member

dsp-ant commented Feb 4, 2025

@dsp-ant - I've updated the PR to move/rename _Function as AnyFunction in mcp.types - lmk what you think!

as for the question of catching typing regressions in CI, are you suggesting something in addition to the pyright running in shared.yml? in prefect we have some type tests like this (leveraging a mypy pytest plugin) as well as a pyright CI job using a config file.

Not sure what'd be preferable to you, but happy to take a shot at it (here or follow up PR) if one of them sounds attractive!

Is there abenefit over mypy? I guess the current checks should be sufficient right?

@dsp-ant dsp-ant self-requested a review February 4, 2025 18:24
@dsp-ant
Copy link
Copy Markdown
Member

dsp-ant commented Feb 4, 2025

Thank you! Much appreciated! The more type safety the better!

@dsp-ant dsp-ant merged commit 08042c3 into modelcontextprotocol:main Feb 4, 2025
@zzstoatzz
Copy link
Copy Markdown
Contributor Author

zzstoatzz commented Feb 4, 2025

I guess the current checks should be sufficient right?

I think so! perhaps someday a pyright config file in a the root of the repo could more specifically config that existing pyright job but that's firmly in the subjective realm in my book 😄

thanks for the review!

@zzstoatzz zzstoatzz deleted the update-fastmcp-server-typing branch February 4, 2025 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dsp-ant dsp-ant dsp-ant approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@zzstoatzz @dsp-ant