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

Introduce Initialization options that are passed to ServerSession #16

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

dsp-ant
Copy link
Member

@dsp-ant dsp-ant commented Oct 11, 2024

Depends on:

There is currently no way for server implementations to pass capabilities, server name and server version to the protocol. We now introduce an InitializationOption object that can be passed. Users of this are free to use the new get_capabilities to fill capabilities based on the defined handlers. This will allow patterns such as

app = Server()

@app.list_prompts()
async def list_my_prompts(...):
   ...

app.run(..., InitializationOptions(..., capabilities=app.get_capabilities()))

This is mostly an RFC. I am not sold on the specific approach but want to get something going so that we can correctly negotiate capabilities between our Zed implementation of the protocol and Python servers.

Comment on lines 41 to 42
@dataclass
class InitializationOptions:
Copy link
Member

Choose a reason for hiding this comment

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

Can we use a Pydantic model here for consistency?

class InitializationOptions:
server_name: str
server_version: str
capabilities: ServerCapabilities
Copy link
Member

Choose a reason for hiding this comment

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

Can we infer this by default, instead of always requiring it to be passed in?

Copy link
Member Author

Choose a reason for hiding this comment

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

I rather be explicit about it for now. I'll keep it in mind however.

@@ -276,13 +276,27 @@ async def handler(req: CompleteRequest):

return decorator

def get_capabilities(self) -> ServerCapabilities:
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a test for this?

Comment on lines 281 to 286
capabilities: dict[str, dict[str, Any] | None] = {
"prompts": {} if ListPromptsRequest in self.request_handlers else None,
"resources": {} if ListResourcesRequest in self.request_handlers else None,
"tools": {} if ListPromptsRequest in self.request_handlers else None,
"logging": {} if SetLevelRequest in self.request_handlers else None,
}
Copy link
Member

Choose a reason for hiding this comment

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

It seems like this code would be simpler if this is just passed in directly to the ServerCapabilities constructor. I guess the point is to do the None filtering, though—I wish we could figure out how to get Pydantic to do that

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay I talked with Claude about this. There might be ways do this with pydantic, but we both prefer the more explicit approach. I added a helper function so people can pass it via app.create_initialization_option(). If you feel strongly about it, i am happy to change it.

Base automatically changed from davidsp/type-fixes to main October 11, 2024 13:06
@dsp-ant dsp-ant force-pushed the davidsp/init-options branch 2 times, most recently from a1afc21 to 3b5b4a4 Compare October 11, 2024 15:01
We need a way for servers to pass initialization options to the session.
This is the beginning of this.
@dsp-ant dsp-ant force-pushed the davidsp/init-options branch from 3b5b4a4 to cc342a0 Compare October 11, 2024 15:07
@dsp-ant dsp-ant merged commit 9475815 into main Oct 11, 2024
2 checks passed
@dsp-ant dsp-ant deleted the davidsp/init-options branch October 11, 2024 15:10
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