-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Unpack settings in FastMCP #1198
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
debug: bool = False, | ||
log_level: Literal["DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"] = "INFO", | ||
host: str = "127.0.0.1", | ||
port: int = 8000, | ||
mount_path: str = "/", | ||
sse_path: str = "/sse", | ||
message_path: str = "/messages/", | ||
streamable_http_path: str = "/mcp", | ||
json_response: bool = False, | ||
stateless_http: bool = False, | ||
warn_on_duplicate_resources: bool = True, | ||
warn_on_duplicate_tools: bool = True, | ||
warn_on_duplicate_prompts: bool = True, | ||
dependencies: Collection[str] = (), | ||
lifespan: Callable[[FastMCP[LifespanResultT]], AbstractAsyncContextManager[LifespanResultT]] | None = None, | ||
auth: AuthSettings | None = None, | ||
transport_security: TransportSecuritySettings | None = None, |
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.
This is the main change. The rest is just to comply with those types.
port: int = 8000, | ||
mount_path: str = "/", | ||
sse_path: str = "/sse", | ||
message_path: str = "/messages/", |
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.
message_path: str = "/messages/", | |
message_path: str = "/messages", |
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.
Hard to trust AI nowadays. 👀
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.
Oh, actually, the AI didn't make a mistake.
The default on Settings.message_path
was added with the /
.
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.
yea i know :(, is that correct tho?
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.
Given that it doesn't follow the pattern, no. But also there's some redirect issues that people may be facing.
But should I really change it in this PR? It may affect people.
log_level: Literal["DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"] = "INFO", | ||
host: str = "127.0.0.1", | ||
port: int = 8000, | ||
mount_path: str = "/", | ||
sse_path: str = "/sse", | ||
message_path: str = "/messages/", | ||
streamable_http_path: str = "/mcp", | ||
json_response: bool = False, | ||
stateless_http: bool = False, | ||
warn_on_duplicate_resources: bool = True, | ||
warn_on_duplicate_tools: bool = True, | ||
warn_on_duplicate_prompts: bool = True, | ||
dependencies: Collection[str] = (), | ||
lifespan: Callable[[FastMCP[LifespanResultT]], AbstractAsyncContextManager[LifespanResultT]] | None = None, | ||
auth: AuthSettings | None = None, | ||
transport_security: TransportSecuritySettings | None = None, |
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.
My main worry is that we now keep the defaults in sync between Settings
and this. I think ideally, but sadly a BC break would be to just accept settings: Settings = Settings()
here. Since I think that would be backwards incompatible. For now, since Settings is only used internally to FastMCP
should we remove the defaults from the Settings
class, such that we only provide it here?
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.
Yep, we should remove the defaults from Settings
.
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.
LGTM
This PR likely broke a lot of
I understand that it's kind of awkward to have a backwards compatibility guarantee for a fast-moving package like |
This is a breaking change and you only updated the patch version of the package. It broke our code: You could fix this by rolling this back, urgently releasing 1.12.4, and then doing it in a backwards compatible way or make a new major version. For reference of what breaks: this code now causes a pydantic validation error
|
Fun coincidence: I had raised the problem with the constructor a while ago and in the issue had proposed a backwards compatible way to handle this |
For Actually, you can also add the environment variable {
"mcpServers": {
"lean-lsp": {
"type": "stdio",
"command": "uvx",
"args": [
"lean-lsp-mcp"
],
"env": {
"UV_CONSTRAINT": "requirements.txt"
}
}
}
} |
please add a bot to your repo that verifies backwards compat. a lot of people depend on your code and you are past alpha. |
It seems there were two different issues here:
I didn't expect people to be instantiating the I guess the way to go here now is to:
|
Settings is a public class and it was the only way to have the kwargs names and types analyzed by the IDE - how was the sense of type safety false? But it seems like indeed few people were using it, otherwise this would have gotten more attention |
No description provided.