KAB-871-mcp-ro-component-tools2 #12

mariankrotil · 2025-03-18T14:50:28Z

Partial PR refactoring current component tools code, no new feature added yet
checkout from (KAB-817)

modularize component tools code into separate files
utilizing pydantic.BaseModel for structured output of the tool functions

…ools code

vita-stejskal

Also please:

Add tests for the tools -- mock the SAPI client in the Context and make sure the tools return expected response objects.
Try to retrieve the list of the tools exposed by the MCP server and confirm that their descriptions really contain the specifications (presumably JSON schema) of the parameter and output values. You can use the MCP client from mcp package to list the available tools.
Increment the project version in pyproject.toml file.

vita-stejskal · 2025-03-19T14:05:28Z

src/keboola_mcp_server/component_tools.py

+
+def add_component_tools(mcp: FastMCP) -> None:
+    """Add tools to the MCP server."""
+    mcp.add_tool(list_components, "list_components", "List all available components.")


You shouldn't have to specify the name and description of the tool explicitly. FastMCP infers this information from the function's signature and dosctring.

Done here b153083

vita-stejskal · 2025-03-19T14:11:20Z

src/keboola_mcp_server/component_tools.py

+
+
+async def list_component_configs(
+    component_id: str,


Use component_id: Annotated[str, Field(description='The ID of a component which configurations should be listed.')] to describe the meaning of the parameter. See examples in the FastMCP docs.

Done here b153083

vita-stejskal · 2025-03-19T14:13:34Z

src/keboola_mcp_server/component_tools.py

+    client = ctx.session.state["sapi_client"]
+    assert isinstance(client, KeboolaClient)
+    r_components = client.storage_client.components.list()
+    return [Component(id=r_comp["id"], name=r_comp["name"]) for r_comp in r_components]


Ideally, you should use Component.model_validate(r_comp) instead of translating the Storage API response data to the Component instance manually. The same applies for the ComponentConfig class below.

Done here b153083

mariankrotil · 2025-03-21T09:52:48Z

cc @vita-stejskal
I added a ComponentClient to handle component requests, avoiding the need to use KeboolClient's get method and expose endpoints within the tool function, which I find messy.
is it ok?

mariankrotil · 2025-03-21T10:41:53Z

cc @vita-stejskal I added a ComponentClient to handle component requests, avoiding the need to use KeboolClient's get method and expose endpoints within the tool function, which I find messy. is it ok?

Oops. I ran into an issue where we would be modifying the same file. So, I went with the first option and exposed the endpoints, but we can refactor it in the future if needed.

mariankrotil · 2025-03-21T11:44:09Z

Also please:

Add tests for the tools -- mock the SAPI client in the Context and make sure the tools return expected response objects.

Try to retrieve the list of the tools exposed by the MCP server and confirm that their descriptions really contain the specifications (presumably JSON schema) of the parameter and output values. You can use the MCP client from mcp package to list the available tools.

Increment the project version in pyproject.toml file.

Tests are here: 4806aa3 febc2d9
List of available tools in mcp server:

it contains fc name, fc descriptions and fc params, but I cannot find param description.

vita-stejskal · 2025-03-24T14:42:30Z

coverage.xml

Why are you changing the .coverage and coverage.xml` files?

I removed it from Git tracking in this repo because it was already listed in the .gitignore file, but that was not working. Coverage files update automatically whenever you create or modify tests and run pytest command as they track how much of our code is covered by the tests. But I think they do not have to be shared in the git repo. Or am I wrong?

vita-stejskal · 2025-03-24T14:50:10Z

tests/conftest.py

+
+
+@pytest.fixture
+def mock_context() -> Tuple[Context, KeboolaClient]:


This is not very nice. I'd slit this fixture into two fixtures -- mcp_context() -> Context and keboola_client(mcp_context) -> KeboolaClient.

Done here: 81c8bb7

vita-stejskal · 2025-03-24T14:51:20Z

tests/conftest.py

+
+
+@pytest.fixture
+def mock_context_with_db() -> Tuple[Context, KeboolaClient, ConnectionManager, DatabasePathManager]:


Why do you define this fixture? It seems unused.

Removed here: 81c8bb7

vita-stejskal · 2025-03-24T14:51:28Z

tests/conftest.py

+
+
+@pytest.fixture
+def mock_snowflake_connection():


Why do you define this fixture? It seems unused.

removed here: 81c8bb7

vita-stejskal · 2025-03-24T14:57:33Z

tests/conftest.py

+    # Mock KeboolaClient
+    mock_client = MagicMock(spec=KeboolaClient)
+    mock_client.storage_client = MagicMock(spec=Client)
+    mock_client.storage_client.components = MagicMock()


I don't think that you need to set this mock here. You set it again in the tests anyway.

Done here: 81c8bb7

vita-stejskal · 2025-03-24T15:00:03Z

tests/test_component_tools.py

+async def test_list_component_configs(mock_context):
+    """Test list_component_configs tool."""
+    context, mock_client = mock_context
+    mock_client.storage_client.configurations = MagicMock()


I don't think that you have to set all those mocks explicitly. MagicMock will replicate itself into all attributes that you access on the mock.

Removing it leads to an error FAILED tests/test_component_tools.py::test_list_component_configs- AttributeError: Mock object has no attribute 'configurations'

mariankrotil added 3 commits March 18, 2025 15:35

KAB-871 feat(tools): add structured output and modularize component t…

Verified

This commit was created on github.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

49d6c35

…ools code

KAB-871 refactor: move cmp tools to sep file

Loading
Loading status checks…

0a7397b

KAB-871 style: apply black

Loading
Loading status checks…

1c50530

mariankrotil requested review from radcliffkey, jordanrburger, ncapek and vita-stejskal March 18, 2025 14:50

vita-stejskal changed the base branch from master to KAB-817-statefull-sessions March 19, 2025 13:59

vita-stejskal requested changes Mar 19, 2025

View reviewed changes

vita-stejskal changed the base branch from KAB-817-statefull-sessions to master March 20, 2025 18:24

mariankrotil added 5 commits March 21, 2025 09:15

KAB-871 refactor: udpate wrt reviews

Loading
Loading status checks…

69b8723

KAB-871 feat: add component client

Loading
Loading status checks…

b153083

KAB-871 fix: remove unused imports, isort, black

Loading
Loading status checks…

4978062

KAB-871 refactor: add return type

Loading
Loading status checks…

f515f39

KAB-871 style: black isort apply

Loading
Loading status checks…

d55689b

mariankrotil added 3 commits March 21, 2025 11:33

KAB-871 test: add conftest to mock context

febc2d9

KAB-871 test: add assessing component tools

4806aa3

KAB-871 test: remove component client

Loading
Loading status checks…

c7bc5b6

mariankrotil added 5 commits March 21, 2025 13:40

KAB-871 fix: remove incorrect async calls

2a24cc9

KAB-871 chore: update vers

Loading
Loading status checks…

124c437

KAB-871 fix: async func

Loading
Loading status checks…

105eac5

KAB-871 chore: update coverage

Loading
Loading status checks…

9fc97ec

Merge branch 'master' into KAB-871-mcp-ro-component-tools2

Loading
Loading status checks…

39b9732

vita-stejskal requested changes Mar 24, 2025

View reviewed changes

mariankrotil added 3 commits March 24, 2025 16:29

KAB-871 fix: remove await fc on non-async fctions

Loading
Loading status checks…

7043c51

KAB-871 ci: update gitignore

46cb422

KAB-871 refactor(delete): remove coverage files from git tracking

Loading
Loading status checks…

b00e206

mariankrotil added 4 commits March 27, 2025 08:15

KAB-871 test: update tests and conftest fixtures accordingly

Loading
Loading status checks…

81c8bb7

KAB-871 style: update test comment

3e63a66

KAB-871 test: apply black

Loading
Loading status checks…

3d6d690

KAB-871 test: remove unused imports

Loading
Loading status checks…

56a2132

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAB-871-mcp-ro-component-tools2 #12

KAB-871-mcp-ro-component-tools2 #12

mariankrotil commented Mar 18, 2025 •

edited by jira bot

Loading

vita-stejskal left a comment

vita-stejskal Mar 19, 2025

mariankrotil Mar 21, 2025

vita-stejskal Mar 19, 2025

mariankrotil Mar 21, 2025

vita-stejskal Mar 19, 2025

mariankrotil Mar 21, 2025

mariankrotil commented Mar 21, 2025

mariankrotil commented Mar 21, 2025

mariankrotil commented Mar 21, 2025 •

edited

Loading

vita-stejskal Mar 24, 2025

mariankrotil Mar 26, 2025

vita-stejskal Mar 24, 2025

mariankrotil Mar 27, 2025

vita-stejskal Mar 24, 2025

mariankrotil Mar 27, 2025

vita-stejskal Mar 24, 2025

mariankrotil Mar 27, 2025

vita-stejskal Mar 24, 2025

mariankrotil Mar 27, 2025

vita-stejskal Mar 24, 2025

mariankrotil Mar 27, 2025 •

edited

Loading



		@pytest.fixture
		def mock_context() -> Tuple[Context, KeboolaClient]:



		@pytest.fixture
		def mock_context_with_db() -> Tuple[Context, KeboolaClient, ConnectionManager, DatabasePathManager]:

KAB-871-mcp-ro-component-tools2 #12

Are you sure you want to change the base?

KAB-871-mcp-ro-component-tools2 #12

Conversation

mariankrotil commented Mar 18, 2025 • edited by jira bot Loading

vita-stejskal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariankrotil commented Mar 21, 2025

mariankrotil commented Mar 21, 2025

mariankrotil commented Mar 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariankrotil Mar 27, 2025 • edited Loading

Choose a reason for hiding this comment

mariankrotil commented Mar 18, 2025 •

edited by jira bot

Loading

mariankrotil commented Mar 21, 2025 •

edited

Loading

mariankrotil Mar 27, 2025 •

edited

Loading