fix: review comments from WG Leads by jaiakash · Pull Request #1 · abhijeet-dhumal/kubeflow-mcp-server

jaiakash · 2026-04-13T23:30:44Z

Hi this PR fixes the review comments from this PR : kubeflow#1

Signed-off-by: Akash Jaiswal <akashjaiswal3846@gmail.com>

Copilot

Pull request overview

This PR applies a set of repo hygiene and CI/dev-experience updates in response to WG lead feedback for the Kubeflow MCP Server, including dependency/packaging config adjustments, documentation updates, and CI workflow changes.

Changes:

Adjust Python packaging/testing/tooling configuration (extras, hatch packaging path, pytest/ruff/coverage settings).
Add a Makefile and update CI to run pre-commit + verification + unit tests with coverage.
Update contributor-facing docs and add CLI unit tests.

Reviewed changes

Copilot reviewed 18 out of 19 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
tests/unit/init.py	Removes unit-test package marker docstring.
pyproject.toml	Updates extras, packaging target, pytest/ruff paths, and adds coverage configuration.
kubeflow_mcp/trainer/api/init.py	Normalizes copyright header.
kubeflow_mcp/trainer/init.py	Normalizes copyright header.
kubeflow_mcp/optimizer/init.py	Normalizes copyright header.
kubeflow_mcp/hub/init.py	Normalizes copyright header.
kubeflow_mcp/core/init.py	Normalizes copyright header.
kubeflow_mcp/common/init.py	Normalizes copyright header.
kubeflow_mcp/cli_test.py	Adds CLI-focused unit tests and mocking for lazy imports.
kubeflow_mcp/cli.py	Normalizes copyright header.
kubeflow_mcp/init.py	Normalizes copyright header.
README.md	Updates compatibility table and development instructions to use Make targets.
Makefile	Adds local dev/verify/test targets using `uv`, ruff, mypy, pytest, coverage.
LICENSE	Modifies Apache 2.0 license text and appendix/copyright section.
CONTRIBUTING.md	Updates contributor instructions to align with Makefile/uv workflow and links policies.
COMPATIBILITY.md	Simplifies compatibility matrix header section.
.pre-commit-config.yaml	Removes mypy pre-commit hook, leaving ruff + commit message hook.
.github/workflows/test.yaml	Removes prior test workflow (including extras smoke tests).
.github/workflows/test-python.yaml	Adds new Python unit test workflow with pre-commit + matrix testing + coverage upload.

Comments suppressed due to low confidence (1)

.pre-commit-config.yaml:15

Mypy was removed from pre-commit, but the contributor docs state pre-commit runs mypy. Either re-add the mypy pre-commit hook or update documentation to reflect the current hook set so contributors know what runs locally/CI.

repos:
  - repo: https://github.com/astral-sh/ruff-pre-commit
    rev: v0.9.0
    hooks:
      - id: ruff
        args: [--fix]
      - id: ruff-format

  - repo: https://github.com/compilerla/conventional-pre-commit
    rev: v3.2.0
    hooks:
      - id: conventional-pre-commit
        stages: [commit-msg]
        args: [feat, fix, docs, style, refactor, perf, test, build, ci, chore]

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-13T23:34:45Z

+
+.PHONY: verify
+verify: ## Run linting, formatting and type checking
+	@uv lock --check


make verify runs uv lock --check, but there is no uv.lock file in the repo. In CI this will fail unless the lockfile is committed; either add the lockfile to version control (and keep it updated) or drop/adjust the lock check.

Suggested change

@uv lock --check

@if [[ -f "$(PROJECT_DIR)/uv.lock" ]]; then \

uv lock --check; \

else \

echo "ℹ️ Skipping 'uv lock --check' because uv.lock is not committed to this repository."; \

fi

Copilot · 2026-04-13T23:34:46Z

+verify: ## Run linting, formatting and type checking
+	@uv lock --check
+	@uv run ruff check .
+	@uv run ruff format --check .
+	@uv run mypy kubeflow_mcp


make verify runs uv run mypy kubeflow_mcp, but mypy is not listed in project.optional-dependencies.dev (and the mypy pre-commit hook was removed). This will break local/CI verification; either add mypy back to the dev dependencies (and restore config if desired) or remove mypy from the verify step.

Suggested change

verify: ## Run linting, formatting and type checking

@uv lock --check

@uv run ruff check .

@uv run ruff format --check .

@uv run mypy kubeflow_mcp

verify: ## Run dependency, linting, and formatting checks

@uv lock --check

@uv run ruff check .

@uv run ruff format --check .

Copilot · 2026-04-13T23:34:46Z

    "pytest-asyncio>=0.23",
    "pytest-cov>=4.0",
    "pytest-benchmark>=4.0",
    "ruff>=0.9",


mypy was removed from the dev extra, but the Makefile/CI still runs mypy. Please re-add mypy to the dev dependencies (or remove mypy from verification) so uv run mypy ... is guaranteed to work in fresh environments.

Suggested change

"ruff>=0.9",

"ruff>=0.9",

"mypy>=1.0",

Copilot · 2026-04-13T23:34:46Z

+    "if self.debug:",
+    "raise AssertionError",
+    "raise NotImplementedError",
+    "if __name__ == .__main__.:",


The coverage exclude_lines pattern if __name__ == .__main__.: looks malformed and will not match the typical Python guard. Update it to match the actual statement (e.g., if __name__ == "__main__":) so coverage reporting behaves as intended.

Suggested change

"if __name__ == .__main__.:",

"if __name__ == \"__main__\":",

Copilot · 2026-04-13T23:34:46Z

 ```

-### Linting
+The pre-commit hooks ensure code quality and consistency (linting with `ruff`, type checking with `mypy`). They are also executed in CI.


This section says pre-commit enforces type checking with mypy, but the current .pre-commit-config.yaml no longer includes a mypy hook. Please update this description (or restore the hook) to keep CONTRIBUTING accurate.

Suggested change

The pre-commit hooks ensure code quality and consistency (linting with `ruff`, type checking with `mypy`). They are also executed in CI.

The pre-commit hooks ensure code quality and consistency (for example, linting with `ruff`). They are also executed in CI.

Copilot · 2026-04-13T23:34:47Z

 [tool.hatch.build.targets.wheel]
-packages = ["src/kubeflow_mcp"]
+packages = ["kubeflow_mcp"]

 [tool.pytest.ini_options]
 asyncio_mode = "auto"
-testpaths = ["tests"]
+testpaths = ["kubeflow_mcp", "tests"]
 addopts = "-v --tb=short"


This repo is no longer using a src/ layout (package is kubeflow_mcp/), but some tooling still references src (e.g., the lint workflow runs mypy src/). Please update CI/tooling to use the new package path (or restore the src/ layout) to avoid CI failures.

Copilot · 2026-04-13T23:34:47Z

 # Contributing to Kubeflow MCP Server

-Thank you for your interest in contributing!
+Thank you for your interest in contributing! Checkout the general Kubeflow contributing guidelines [here](https://www.kubeflow.org/docs/about/contributing/).


Minor grammar: "Checkout the general Kubeflow contributing guidelines" should be "Check out the general Kubeflow contributing guidelines".

Suggested change

Thank you for your interest in contributing! Checkout the general Kubeflow contributing guidelines [here](https://www.kubeflow.org/docs/about/contributing/).

Thank you for your interest in contributing! Check out the general Kubeflow contributing guidelines [here](https://www.kubeflow.org/docs/about/contributing/).

Copilot · 2026-04-13T23:34:47Z

+  finish:
+    needs: test


The previous CI workflow included a smoke-test job to verify optional extras (optimizer, hub) can be installed and imported. That coverage is missing in the new workflow, which increases the risk of breaking optional dependency sets without noticing. Consider reintroducing an extras import/smoke-test matrix (or an equivalent check) in this workflow.

Suggested change

finish:

needs: test

extras-smoke-test:

runs-on: ubuntu-latest

needs: pre-commit

strategy:

fail-fast: false

matrix:

extra: ["optimizer", "hub"]

name: Extras smoke test (${{ matrix.extra }})

steps:

- uses: actions/checkout@v4

- name: Set up Python 3.11

uses: actions/setup-python@v5

with:

python-version: "3.11"

- name: Install uv

uses: astral-sh/setup-uv@v3

- name: Install package with ${{ matrix.extra }} extra

run: uv pip install ".[${{ matrix.extra }}]"

- name: Smoke test ${{ matrix.extra }} extra

run: python -c "import importlib.util, sys; assert importlib.util.find_spec('pip') is not None"

finish:

needs: [test, extras-smoke-test]

Copilot · 2026-04-13T23:34:47Z

      the original version of the Work and any modifications or additions
      to that Work or Derivative Works thereof, that is intentionally
-      submitted to the Licensor for inclusion in the Work by the copyright owner
+      submitted to Licensor for inclusion in the Work by the copyright owner


The repository LICENSE text appears to be modified from the canonical Apache 2.0 wording (e.g., removing "the" in "submitted to the Licensor"). The LICENSE file should match the standard Apache 2.0 text verbatim; please revert this line to the official wording to avoid unintended legal/consistency issues.

Suggested change

submitted to Licensor for inclusion in the Work by the copyright owner

submitted to the Licensor for inclusion in the Work by the copyright owner

Copilot · 2026-04-13T23:34:48Z

+   APPENDIX: How to apply the Apache License to your work.
+
+      To apply the Apache License to your work, attach the following
+      boilerplate notice, with the fields enclosed by brackets "{}"
+      replaced with your own identifying information. (Don't include
+      the brackets!)  The text should be enclosed in the appropriate
+      comment syntax for the file format. We also recommend that a
+      file or class name and description of purpose be included on the
+      same "printed page" as the copyright notice for easier
+      identification within third-party archives.
+
+   Copyright {yyyy} {name of copyright owner}

   Licensed under the Apache License, Version 2.0 (the "License");


The LICENSE file now includes the Apache 2.0 appendix and a placeholder copyright line ("Copyright {yyyy} {name of copyright owner}"). Repositories typically keep LICENSE as the unmodified license text and do not leave placeholders; please revert this section to the standard LICENSE contents (or restore the prior repository-specific copyright line if it was intended).

* chore: initialize kubeflow-mcp repository skeleton Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com> * fix: review comments Signed-off-by: Akash Jaiswal <akashjaiswal3846@gmail.com> * fix: address remaining review comments after akash's fixes Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com> * chore: remove COMPATIBILITY.md, compat matrix already in README Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com> * chore: add spark extra and include it in all optional deps Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com> * fix: remove redundant lint workflow, align commit types, fix copyright year Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com> * fix: update PR template, README install instructions, add missing copyright Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com> * fix: restore original LICENSE content per review feedback Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com> --------- Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com> Signed-off-by: Akash Jaiswal <akashjaiswal3846@gmail.com> Co-authored-by: Akash Jaiswal <akashjaiswal3846@gmail.com>

fix: review comments

5f04d3d

Signed-off-by: Akash Jaiswal <akashjaiswal3846@gmail.com>

Copilot AI review requested due to automatic review settings April 13, 2026 23:30

Copilot started reviewing on behalf of jaiakash April 13, 2026 23:31 View session

Copilot AI reviewed Apr 13, 2026

View reviewed changes

abhijeet-dhumal merged commit 5b67792 into abhijeet-dhumal:add-trainer-tools-baseline Apr 15, 2026
3 of 4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: review comments from WG Leads#1

fix: review comments from WG Leads#1
abhijeet-dhumal merged 1 commit into
abhijeet-dhumal:add-trainer-tools-baselinefrom
jaiakash:akash-review-fix

jaiakash commented Apr 13, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 13, 2026

Uh oh!

Copilot AI Apr 13, 2026

Uh oh!

Copilot AI Apr 13, 2026

Uh oh!

Copilot AI Apr 13, 2026

Uh oh!

Copilot AI Apr 13, 2026

Uh oh!

Copilot AI Apr 13, 2026

Uh oh!

Copilot AI Apr 13, 2026

Uh oh!

Copilot AI Apr 13, 2026

Uh oh!

Copilot AI Apr 13, 2026

Uh oh!

Copilot AI Apr 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-	@uv lock --check
+	@if [[ -f "$(PROJECT_DIR)/uv.lock" ]]; then \
+	  uv lock --check; \
+	else \
+	  echo "ℹ️  Skipping 'uv lock --check' because uv.lock is not committed to this repository."; \
+	fi

	"if __name__ == .__main__.:",
	"if __name__ == \"__main__\":",

	The pre-commit hooks ensure code quality and consistency (linting with `ruff`, type checking with `mypy`). They are also executed in CI.
	The pre-commit hooks ensure code quality and consistency (for example, linting with `ruff`). They are also executed in CI.

	Thank you for your interest in contributing! Checkout the general Kubeflow contributing guidelines [here](https://www.kubeflow.org/docs/about/contributing/).
	Thank you for your interest in contributing! Check out the general Kubeflow contributing guidelines [here](https://www.kubeflow.org/docs/about/contributing/).

-  finish:
-    needs: test
+  extras-smoke-test:
+    runs-on: ubuntu-latest
+    needs: pre-commit
+    strategy:
+      fail-fast: false
+      matrix:
+        extra: ["optimizer", "hub"]
+    name: Extras smoke test (${{ matrix.extra }})
+    steps:
+      - uses: actions/checkout@v4
+      - name: Set up Python 3.11
+        uses: actions/setup-python@v5
+        with:
+          python-version: "3.11"
+      - name: Install uv
+        uses: astral-sh/setup-uv@v3
+      - name: Install package with ${{ matrix.extra }} extra
+        run: uv pip install ".[${{ matrix.extra }}]"
+      - name: Smoke test ${{ matrix.extra }} extra
+        run: python -c "import importlib.util, sys; assert importlib.util.find_spec('pip') is not None"
+  finish:
+    needs: [test, extras-smoke-test]

	submitted to Licensor for inclusion in the Work by the copyright owner
	submitted to the Licensor for inclusion in the Work by the copyright owner

Conversation

jaiakash commented Apr 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants