feat: add core MCP server, TrainerClient tools and Resources#2
Conversation
7013e56 to
08213ed
Compare
d069607 to
f418eb1
Compare
9cdc9fd to
db75fd1
Compare
db75fd1 to
470812d
Compare
470812d to
40fce0e
Compare
|
Hey @andreyvelich @astefanutti @kramaranya @szaher @Electronic-Waste - the 2nd onboarding phase is ready for review now 🏁 |
4db9623 to
fa2b656
Compare
|
Thanks @abhijeet-dhumal, is it possible to split the changes into a few PRs (for example MCP server, tools, docs/plugin/benchmark)? |
Thanks @kramaranya .. I completely understand the preference for smaller PRs, and I normally follow that approach. The context here is that after discussing with @andreyvelich and @thesuperzapper, we agreed on a phased onboarding strategy with a time constraint: land the project as a cohesive unit first, then refine via focused follow-up PRs. I tried to keep commits in this PR structured as logically separate components to make review easier. Splitting into parallel PRs now is doable, but these components have tight interdependencies (e.g. tools depend on core server, benchmarks test the tools, security hardening touches multiple layers), so it would add significant overhead to manage the PR dependency chain and likely slow down the merge timeline. @andreyvelich @astefanutti @thesuperzapper What way do you recommend, Happy to adjust either way. |
|
@abhijeet-dhumal Maybe you can separate Claude plugin and SKILLS into separate PR? So we can review MCP tools first. |
8eeb74c to
5a937dc
Compare
…tests Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com>
5a937dc to
5f4a418
Compare
Thanks @jaiakash - all 4 items addressed in the latest push.. Appreciate the thorough review 🙌 |
@andreyvelich @kramaranya @astefanutti Apologies for the size of this PR - I understand it makes review harder. Re: why MCP Resources are included MCP Resources are a protocol-level feature that agents read on-demand when tools reference them:
Without these, tool responses would contain issues that agents can't resolve. They're co-located with the tools because they're registered by the same client module and tested together (architecture tests verify all URIs are resolvable). Happy to clarify anything further — let me know if you'd like me to adjust the scope. |
Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com>
|
Thanks for the thorough review, @kramaranya ! I have resolved the comments raised.. |
…tate to policy Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com>
|
Hey @kramaranya, thanks again for the thorough pass. Addressed several of your threaded points.. |
andreyvelich
left a comment
There was a problem hiding this comment.
Thanks @abhijeet-dhumal!
/lgtm
/approve
/hold @kramaranya in case you have more suggestions
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andreyvelich The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kramaranya
left a comment
There was a problem hiding this comment.
Thank you, @abhijeet-dhumal!
LGTM
/unhold
fixes #7
Description
Implements the core MCP server framework and all TrainerClient-backed tools per KEP-936.
Claude plugin and skills moved to a separate PR per reviewer feedback.
Core Framework (
kubeflow_mcp/core/)readonly,data-scientist,ml-engineer,platform-admin) with allow/deny overridesTrainerClient Tools (23 tools) and 3 MCP resource guides
CLI -
servecommand withstdio/http/ssetransport, persona, auth token, instruction tier, progressive/semantic modesTest Suite (131 tests)
cli_test.py(25) — CLI flags, config fallback, auth/resilience wiring, SSE transportsdk_contracts_test.py(68) — SDK dataclass fields, enum values, API signatures, K8s client methodstest_architecture.py(38) — Tool metadata, persona gating, instruction tiers, resource registrationChecklist
make test-python) — 131 passingmake verify) — 0 errorsRelated Issues
Implements KEP-936