Skip to content

Add progress notification callback for client #721

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

Merged
merged 10 commits into from
May 15, 2025
Merged

Conversation

ihrpr
Copy link
Contributor

@ihrpr ihrpr commented May 15, 2025

It started as adding Everything server test for StreamableHttp and SSE, but then I noticed I can't test progress notifications.

This PR adds a callback for progress notifications to the client session and sets progress token to request id where there is a callback. This approach is similar to how we are handling it in TypeScript.

Adds Everything server with tools, resources and prompts.

@@ -231,15 +243,25 @@ async def send_request(
](1)
self._response_streams[request_id] = response_stream

# Set up progress token if progress callback is provided
request_data = request.model_dump(by_alias=True, mode="json", exclude_none=True)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[properly non-blocking] wondering if we can set this in the pydantic model before dumping? I'm really not fussed either way (it would just give you validation etc)

# Connect to the server with callbacks
async with sse_client(everything_server_url + "/sse") as streams:
# Set up message handler to capture notifications
async def message_handler(message):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[non-blocking] I was thinking we could literally tap the memory channels that wireup the transport, but this is also good

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just wanted to make sure that the transports also support all the features, especially after catching an issues with sampling in shttp

@ihrpr ihrpr merged commit 5d33861 into main May 15, 2025
13 checks passed
@ihrpr ihrpr deleted the ihrpr/features-tests branch May 15, 2025 16:46
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