-
Notifications
You must be signed in to change notification settings - Fork 1.5k
chore: add dereference_json_schema #1192
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
907fa6d to
672ba7b
Compare
|
This would also solve the same problem with Claude Code, which I asked about on #1159. This seems like a pragmatic solution. @vl-kp Thank you for this! I'm wondering why leave Although, it seems unfortunate to modify core to accommodate offending clients and lead to lot of configuration options. So I wondered if it was possible using the new middleware feature. It's not at present, because initialization requests are not exposed to middleware. Also, client identification occurs only in the |
There are some corner cases i am not sure it will happen or not:
|
d995567 to
d9c31a8
Compare
Ok, if I understand, this is when
I see, thanks! I guess I would add $def only if max_depth is exceeded. I forgot to ask about |
|
Hi @vl-kp, I have some hesitation about introducing compromises because of client failures; obviously that's not a sustainable way for us to build great features, and folks opening subsequent issues will only deepen the rabbit hole. I do recognize the problem however and I'm intrigued by @richardkmichael's suggestion that MCP Middleware is an excellent way to solve this. I think it is a fantastic motivation for the work in #1215. Then instead of exposing this as a global configuration it could be bundled as e.g. |
I agree, i updated to only add $def when corner case happens
i did a perf test under my Apple M4 Pro(24GB), looks like it doesn't impact too much performance, so i updated to depth to 50, it should meet most of the use cases |
bf98669 to
906c7b6
Compare
sure, we could wait for middleware feature implemented |
|
When I ran into this with Gemini I implemented https://pypi.org/project/jsonref/ with some luck Might be worth considering vs hand rolling the dereferencing |
|
@strawgate Thank you! (FastMCP Agents looks quite handy, glad you mentioned it.) jsonref has a few perhaps notable issues, although none are my use-case. However, their existence speaks to occasional difficulties with specific schema, which I think makes more of a case for externalizing this from FastMCP. As opposed to an internal implementation, jsonref or hand-rolled, needing to be bug fixed in FastMCP. For example, I'm unsure if a wrapped function used by FastMCP could generate a circular schema.
@vl-kp Thanks for the benchmark. They are mostly < 1 ms, so it's not a concern for me. Note also, jsonref has no max-depth. |
|
I'm going to close this PR for now since I don't think this is strategically an approach we want to take, but can open a new PR for an alternative like middleware that would more user control |
|
follow up PR #1641 |
In order to resolve issue #1193