-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fix: Add checks and better error messages on server object for mcp run #297
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
5f82633
to
e72e167
Compare
e72e167
to
f1eed1a
Compare
(Force pushed to resolve merge conflict.) |
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.
Happy to merge this, but there are two minor nits I'd love to be addressed.
6105346
to
2dfdada
Compare
a3621c9
to
3f2e84d
Compare
Hello @dsp-ant |
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.
All comments addressed. Thank you!
@john0312 please can you resolve merge conflict and we can merge. |
…n/dev mcp run/dev command expects the server object in the python file to be a global variable, and it must be of the FastMCP type and not the low level Server type. The previous error messages may be confusing to users who are not aware of these facts.
3f2e84d
to
e03aecd
Compare
e03aecd
to
9f6df59
Compare
@ihrpr Rebased. Somehow the force push caused the review to go stale. Please help me review again and merge if possible. Thank you. |
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.
thank you for the quick rebase!
mcp run/dev command expects the server object in the python file to be a global variable, and it must be of the FastMCP type and not the low level Server type. The previous error messages may be confusing to users who are not aware of these facts.
Motivation and Context
It is a bit confusing to users who tries to run an MCP server of the low level Server variant with the mcp run command and get unrelated error messages.
How Has This Been Tested?
Run an MCP server of the low level Server variant and check that the correct error message is displayed.
Breaking Changes
None
Types of changes
Checklist
Additional context
N/A