Skip to content

Features vs Crates and modularity (was: Learn or combine efforts from mcp-daemon?) #95

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

Open
Hendler opened this issue Apr 4, 2025 · 24 comments
Labels
enhancement New feature or request

Comments

@Hendler
Copy link
Contributor

Hendler commented Apr 4, 2025

For my own edification I'm curious how the approaches for the official SDK differ from MCP Daemon's. I've asked them as well https://github.com/entrepeneur4lyf/mcp-daemon/issues/4

@Hendler Hendler added the enhancement New feature or request label Apr 4, 2025
@4t145
Copy link
Collaborator

4t145 commented Apr 5, 2025

I will have a look on this. And if there is anything we are not good enough or haven't done, please let us know.

@jokemanfire
Copy link
Collaborator

We can refer to the implementation of WebSocket @4t145

@Hendler
Copy link
Contributor Author

Hendler commented Apr 6, 2025

@jokemanfire thank you for taking a look.
They use Actix, but this repo is using Axum. Axum websocket support seems good but I haven't used it https://docs.rs/axum/latest/axum/extract/ws/struct.WebSocket.html

Maybe the implementation won't be that different.
is there a TODO list or roadmap I can look to to know where to help?

@jokemanfire
Copy link
Collaborator

We can include websocket support in the to-do list, and you can develop it in parallel (and track the issue). We should add pre release transaction items in the future @4t145 @Hendler

@4t145
Copy link
Collaborator

4t145 commented Apr 7, 2025

@Hendler @jokemanfire We already have a websocket example. Websocket transport is not a part of MCP specification. If websocket transport become a de facto standard, we should support it but I haven't seen it so far.

@Hendler
Copy link
Contributor Author

Hendler commented Apr 7, 2025

@4t145 got it. What is high priority on the todo list?

@jokemanfire
Copy link
Collaborator

jokemanfire commented Apr 7, 2025

It should be the first official release. Like #55 , Of course, CI documents and missing test cases are also included.

@jokemanfire
Copy link
Collaborator

@4t145 Can we include the issues or PRs to be done for the release of the first version in the milestone?

@4t145
Copy link
Collaborator

4t145 commented Apr 7, 2025

@jokemanfire @Hendler Let's create a 0.2.0 milestone.

@EItanya
Copy link
Contributor

EItanya commented Apr 7, 2025

This repo definitely looks interesting, but also worth noting that this library uses traits to define all of the MCP operations, whereas this other one uses string paths. Not saying which is better, but the approach of this library does feel more ergonomic. This also brings up one more thing I wanted to mention about this library. I think it's worth logically separating the library into 2 distinct parts. The core, and the extensions. In my mind the core is everything that has to do with message handling, and the spec itself, and then extensions would be the implementation. When I say the implementation I mean the I/O aspects.. Why is this important? I for example am consuming this library only to use the core logic of the spec, and I will not be able to pull in any web libraries, or anything related to TLS. This will allow for users who want a quick way to start with MCP, and users who have use-cases specifically related to the core logic of the spec, and intend to build their own applications around it.

@Hendler
Copy link
Contributor Author

Hendler commented Apr 7, 2025

This repo definitely looks interesting, but also worth noting that this library uses traits to define all of the MCP operations, whereas this other one uses string paths. Not saying which is better, but the approach of this library does feel more ergonomic. This also brings up one more thing I wanted to mention about this library. I think it's worth logically separating the library into 2 distinct parts. The core, and the extensions. In my mind the core is everything that has to do with message handling, and the spec itself, and then extensions would be the implementation. When I say the implementation I mean the I/O aspects.. Why is this important? I for example am consuming this library only to use the core logic of the spec, and I will not be able to pull in any web libraries, or anything related to TLS. This will allow for users who want a quick way to start with MCP, and users who have use-cases specifically related to the core logic of the spec, and intend to build their own applications around it.

@EItanya Looking at the official Typescript and Python implementations, it feels like this repo must be batteries included, but I agree it would be nice (or maybe critical) to have "core logic" be usable by repos like MCP daemon and your work.

@4t145
Copy link
Collaborator

4t145 commented Apr 8, 2025

@Hendler If you disable all the features you can also get a core crate here (I guess)

@ramnivas
Copy link
Contributor

ramnivas commented Apr 8, 2025

As @4t145 mentioned, disabling all features does give access to the “core” crate, but the cognitive load would be lower if there were a clearer separation between the true core (no web framework) and one connector for each web framework. This way, each connector can be developed independently, and using just the core becomes simpler. It also will make contributing another web framework easier (the current approach will need to add even more features).

From a developer experience standpoint, users could opt into rmcp-axum, rmcp-actix, etc., if they prefer a batteries-included setup.

I have a hand-rolled implementation that I’d like to migrate to this crate. My project already uses actix-web, so I only need the core functionality. It’s not a deal breaker if the current structure stays, but I did find it a bit harder to follow due to the mix of multiple concerns.

@Hendler
Copy link
Contributor Author

Hendler commented Apr 8, 2025

As @4t145 mentioned, disabling all features does give access to the “core” crate, but the cognitive load would be lower if there were a clearer separation between the true core (no web framework) and one connector for each web framework. This way, each connector can be developed independently, and using just the core becomes simpler. It also will make contributing another web framework easier (the current approach will need to add even more features).

From a developer experience standpoint, users could opt into rmcp-axum, rmcp-actix, etc., if they prefer a batteries-included setup.

I have a hand-rolled implementation that I’d like to migrate to this crate. My project already uses actix-web, so I only need the core functionality. It’s not a deal breaker if the current structure stays, but I did find it a bit harder to follow due to the mix of multiple concerns.

Thank you @4t145 and @ramnivas
That's a great point of feedback. I will take a stab at a refactor that achieves this, but hopefully small enough so it's easy to give early feedback.

@4t145
Copy link
Collaborator

4t145 commented Apr 8, 2025

@ramnivas My motivation is, the current axum web framework intergration is only for people who want a quick start. People who want a more complex transport layer can implement themselves.

So I think the current rmcp crate is "core" enough. If you don't enable feature transport-sse-server, there is no framework and you don't have to enable it. I put it here because some people do want cargo add rmcp and develop a simple mcp server quickly. (4t145/rmcp#27)

@Hendler
Copy link
Contributor Author

Hendler commented Apr 8, 2025

@ramnivas My motivation is, the current axum web framework intergration is only for people who want a quick start. People who want a more complex transport layer can implement themselves.

So I think the current rmcp crate is "core" enough. If you don't enable feature transport-sse-server, there is no framework and you don't have to enable it. I put it here because some people do want cargo add rmcp and develop a simple mcp server quickly. (4t145/rmcp#27)

@4t145 I think the current design is very thoughtful this way. The PR I'm working on won't be complex, but might make code easier to read and extend as the repo grows. It may be ready EOD tomorrow - it's just an idea to help discussions and help me learn the repo. If it isn't an improvement, we can throw it away.

@Hendler
Copy link
Contributor Author

Hendler commented Apr 8, 2025

@4t145 @ramnivas

Here's a start at isolating core functionality and traits from implementations of servers and clients. For now very little has changed naming wise, although some imports might break.

#105

Next steps could include

  • starting to migrate some of the tests or more, new doctests.
  • creating more protocol level models and having them in clearly named files

But, does this make rmcp core more importable and usable in other projects?

@4t145
Copy link
Collaborator

4t145 commented Apr 8, 2025

@Hendler If you just need the model part, you can make transport a feature, make service a feature, and make handler a feature. This could reduce a lot of work for refactor.

@jokemanfire
Copy link
Collaborator

Do we need to start refactoring now? After all, there are still important requirements that have not been developed yet, as running them in parallel can lead to many conflicts. :)

@Hendler
Copy link
Contributor Author

Hendler commented Apr 8, 2025

@Hendler If you just need the model part, you can make transport a feature, make service a feature, and make handler a feature. This could reduce a lot of work for refactor.

@4t145 Are you suggesting this instead of the refactor or in added to the PR?

Do we need to start refactoring now? After all, there are still important requirements that have not been developed yet, as running them in parallel can lead to many conflicts. :)

@jokemanfire Not many files actually changed, and imports are reexported at rmcp level. Maybe you are right?

The argument for it; I think in core devs would want

  1. as few dependencies as possible (can start cleaning up core)
  2. easier usage in WASM or in FFI/.so or .a
  3. can still use feature flags at the rmcp crate level
  4. clearer API for core vs quick start usage

This could be the first PR to align more closely with the Typescript implementation.

  • types.ts for protocol definitions (equivalent to rmcp-core)
  • shared/protocol.ts for core protocol handling (some of this is in rmcp/service.rs)
  • shared/transport.ts for transport interface (similar to rmcp/transport.rs)

In the next PR we might

  1. Move core traits like ServiceRole to rmcp-core
  2. Move RequestIdProvider to rmcp-core
  3. Create rmcp-core/protocol.rs: Core protocol handling (equivalent to TypeScript's protocol.ts)
  4. Create rmcp-core/transport_traits.rs: Transport interface definitions without implementations

Doing this later might be more difficult.

Happy to look at other branches and features that would be affected, and know what is higher priority.

@4t145
Copy link
Collaborator

4t145 commented Apr 8, 2025

@Hendler By now, the necessary dependencies are these below

serde = { version = "1.0", features = ["derive", "rc"] }
serde_json = "1.0"
thiserror = "2"
chrono = { version = "0.4.38", features = ["serde"] }
tokio = { version = "1", features = ["sync", "macros", "rt", "time"] }
futures = "0.3"
tracing = { version = "0.1" }
tokio-util = { version = "0.7" }
pin-project-lite = "0.2"

I think we cannot remove anyone of those in rmcp-core. If you look at a far more complex crate like hyper or tokio, these crate also manage different features including http client, http server, different http version, ffi, by using different features.

Assuming we have already finished those works you are going to do. What would be left in rmcp? It could be handlers, macros, and transport implementation. For macros and transport implementation, we already have featrues to shadow those code. And if you wan't to remove handler part, you can simply add a handler feature to shadow them.

@jokemanfire
Copy link
Collaborator

@4t145 If I understand correctly, it would be better to use features instead of rebuilding multiple Crates for RUST. @Hendler

@Hendler
Copy link
Contributor Author

Hendler commented Apr 9, 2025

@jokemanfire @4t145 I think features approach is fine. This would manifest as more examples and documentation about how to use just the core features and have a feature called core? I could take a stab at that as well. Would that be helpful?

To complete the dialog - as a counter example, the serde project uses separate crates for different implementations. Features sprinkled throughout can make the code less readable and organized. I want traits easy to read and code organized like the Typescript project where possible. Maybe @ramnivas has more to say on this.

@Hendler Hendler changed the title Learn or combine efforts from mcp-daemon? Features vs Crates and modularity (was: Learn or combine efforts from mcp-daemon?) Apr 9, 2025
@ramnivas
Copy link
Contributor

ramnivas commented Apr 9, 2025

@jokemanfire @4t145 As @Hendler mentions, serde vs serde_json is a good counter example, where implementation is separated in a different create. But, as I mentioned earlier, this is not a deal-breaker by any means. Like @Hendler, it felt better to not include anything axum-specific, but by no means that is the only choice.

In the next couple of days, I will try to use the current main branch (the core part, without any features) and see how it goes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants