Skip to content
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

Add client sampling #132

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

SecretiveShell
Copy link
Contributor

@SecretiveShell SecretiveShell commented Jan 4, 2025

This adds a sampling request handler parameter to the ClientSession Class.

Motivation and Context

This will allow for receiving sampling requests inside of python clients.

How Has This Been Tested?

This still needs extensive testing, but for now I have been testing using this example repo https://github.com/SecretiveShell/MCP-python-client-sampling-demo

Breaking Changes

None

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

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

Currently this PR is failing, the example tests work but the response from the everything server is resp.content[0].text='LLM sampling result: [object Object]' I am opening this as a draft PR for the opportunity to get feedback and iterate on the implementation. This is now resolved

@SecretiveShell
Copy link
Contributor Author

seems like the object Object issue is not related to this implementation modelcontextprotocol/servers#477

@dsp-ant
Copy link
Member

dsp-ant commented Jan 7, 2025

I like the concept here. Do you mind pushing this over the finish line? Cleanup, Documentation and Tests?

@SecretiveShell
Copy link
Contributor Author

Glad you feel that this is the correct way to go about implementing this, if there are no desired changes I am happy to start pushing forwards with docs/testing.

@SecretiveShell
Copy link
Contributor Author

SecretiveShell commented Jan 7, 2025

I've confirmed the functionality is working on my end. However, the testing is blocked by the issue with the sampling test tool in the everything server. Once that's fixed, I can add proper unit tests. Let me know if there are other changes needed in the meantime.

@SecretiveShell SecretiveShell marked this pull request as ready for review January 17, 2025 19:42
@dsp-ant dsp-ant self-requested a review January 22, 2025 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants