-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Update cohere and MCP, add support for MCP ResourceLink returned from tools #2094
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
PR Change SummaryUpdated tests to ensure compatibility with the latest versions of OpenAI, MistralAI, and MCP.
Modified Files
How can I customize these reviews?Check out the Hyperlint AI Reviewer docs for more information on how to customize the review. If you just want to ignore it on this PR, you can add the Note specifically for link checks, we only check the first 30 links in a file and we cache the results for several hours (for instance, if you just added a page, you might experience this). Our recommendation is to add |
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.
@medaminezghal Thanks Mohamed! The only thing I'm not sure about is how we should handle ResourceLink
.
@medaminezghal Can you please have a look at the failing coverage check? |
dd0f644
to
7b83106
Compare
@DouweM Does it need to add some tests to fix coverage problem? |
@medaminezghal Yep, there are already a number of |
Do I need a real API key to use |
@medaminezghal I wouldn't use |
@DouweM I don't have any API key to use it for tests, so, the Do you think I should use one of the file inside For example:
|
@medaminezghal I think you should only need the actual full file path in the If you have your own OpenAI API key, you can generate the cassette for your new test with that, but if not feel free to push up the test and I can generate the cassette myself locally. |
@DouweM I will just duplicate the tests that use |
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.
@medaminezghal I was out for a few days but just had a chance to review this again. Please have a look at my comments and let me know if I can provide additional guidance (or take over).
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.
@medaminezghal Thanks for your work on this Mohamed! I don't think the MCP resource stuff is quite right yet though. Let me know if you'd like me to finish this up!
@medaminezghal Please have a look at the failing tests |
@DouweM I've fixed the tests. The only failed ones are related with |
@medaminezghal I suggest maybe separating the OTel stuff into a different PR to not block this one. |
@medaminezghal As @alexmojaki suggested, can you please move the Logfire/OTel changes out into a separate PR? Because this was one was almost done and now those changes created some more issues to address :) If you can pull that out, I can make the MCP tests pass and merge this quickly. |
@DouweM Good idea, I will create new PR for Logfire/OTel. |
@DouweM The changes are reverted now. |
@medaminezghal Thanks Mohamed! |
… tools (pydantic#2094) Co-authored-by: Douwe Maan <[email protected]>
I've updated known model list with new OpenAI version, fixed MistralAI test issue in new version, added new ResourceLink MCP type and fixed tests for new MCP version.