From be521ffbb8b244612b2762905323893989de8743 Mon Sep 17 00:00:00 2001 From: Evan Mattson Date: Tue, 2 Jul 2024 19:44:32 -0400 Subject: [PATCH 1/3] Update sessions python plugin to work with the correct endpoint. Update unit tests. --- .../sessions_python_plugin.py | 152 ++++++++++-------- .../sessions_python_settings.py | 4 +- .../test_sessions_python_plugin.py | 115 ++++++++----- 3 files changed, 162 insertions(+), 109 deletions(-) diff --git a/python/semantic_kernel/core_plugins/sessions_python_tool/sessions_python_plugin.py b/python/semantic_kernel/core_plugins/sessions_python_tool/sessions_python_plugin.py index 302e4360c52b..27d72d774703 100644 --- a/python/semantic_kernel/core_plugins/sessions_python_tool/sessions_python_plugin.py +++ b/python/semantic_kernel/core_plugins/sessions_python_tool/sessions_python_plugin.py @@ -7,7 +7,7 @@ from io import BytesIO from typing import Annotated, Any -import httpx +from httpx import AsyncClient, HTTPStatusError from pydantic import ValidationError from semantic_kernel.connectors.telemetry import HTTP_USER_AGENT, version_info @@ -35,14 +35,14 @@ class SessionsPythonTool(KernelBaseModel): pool_management_endpoint: HttpsUrl settings: SessionsPythonSettings auth_callback: Callable[..., Awaitable[Any]] - http_client: httpx.AsyncClient + http_client: AsyncClient def __init__( self, auth_callback: Callable[..., Awaitable[Any]], pool_management_endpoint: str | None = None, settings: SessionsPythonSettings | None = None, - http_client: httpx.AsyncClient | None = None, + http_client: AsyncClient | None = None, env_file_path: str | None = None, **kwargs, ): @@ -59,7 +59,7 @@ def __init__( settings = SessionsPythonSettings() if not http_client: - http_client = httpx.AsyncClient() + http_client = AsyncClient() super().__init__( pool_management_endpoint=aca_settings.pool_management_endpoint, @@ -111,6 +111,10 @@ def _build_url_with_version(self, base_url, endpoint, params): """Builds a URL with the provided base URL, endpoint, and query parameters.""" params["api-version"] = SESSIONS_API_VERSION query_string = "&".join([f"{key}={value}" for key, value in params.items()]) + if not base_url.endswith("/"): + base_url += "/" + if endpoint.endswith("/"): + endpoint = endpoint[:-1] return f"{base_url}{endpoint}?{query_string}" @kernel_function( @@ -159,19 +163,24 @@ async def execute_code(self, code: Annotated[str, "The valid Python code to exec } url = self._build_url_with_version( - base_url=self.pool_management_endpoint, - endpoint="python/execute/", + base_url=str(self.pool_management_endpoint), + endpoint="code/execute/", params={"identifier": self.settings.session_id}, ) - response = await self.http_client.post( - url=url, - json=request_body, - ) - response.raise_for_status() - - result = response.json() - return f"Result:\n{result['result']}Stdout:\n{result['stdout']}Stderr:\n{result['stderr']}" + try: + response = await self.http_client.post( + url=url, + json=request_body, + ) + response.raise_for_status() + result = response.json()["properties"] + return f"Result:\n{result['result']}Stdout:\n{result['stdout']}Stderr:\n{result['stderr']}" + except HTTPStatusError as e: + error_message = e.response.text if e.response.text else e.response.reason_phrase + raise FunctionExecutionException( + f"Code execution failed with status code {e.response.status_code} and error: {error_message}" + ) @kernel_function(name="upload_file", description="Uploads a file for the current Session ID") async def upload_file( @@ -199,33 +208,33 @@ async def upload_file( remote_file_path = self._construct_remote_file_path(remote_file_path or os.path.basename(local_file_path)) - with open(local_file_path, "rb") as data: - auth_token = await self._ensure_auth_token() - self.http_client.headers.update( - { - "Authorization": f"Bearer {auth_token}", - USER_AGENT: SESSIONS_USER_AGENT, - } - ) - files = [("file", (remote_file_path, data, "application/octet-stream"))] + auth_token = await self._ensure_auth_token() + self.http_client.headers.update( + { + "Authorization": f"Bearer {auth_token}", + USER_AGENT: "SESSIONS_USER_AGENT", + } + ) - url = self._build_url_with_version( - base_url=self.pool_management_endpoint, - endpoint="python/uploadFile", - params={"identifier": self.settings.session_id}, - ) + url = self._build_url_with_version( + base_url=str(self.pool_management_endpoint), + endpoint="files/upload", + params={"identifier": self.settings.session_id}, + ) - response = await self.http_client.post( - url=url, - json={}, - files=files, # type: ignore + try: + with open(local_file_path, "rb") as data: + files = {"file": (remote_file_path, data, "application/octet-stream")} + response = await self.http_client.post(url=url, files=files) + response.raise_for_status() + response_json = response.json() + return SessionsRemoteFileMetadata.from_dict(response_json["value"][0]["properties"]) + except HTTPStatusError as e: + error_message = e.response.text if e.response.text else e.response.reason_phrase + raise FunctionExecutionException( + f"Upload failed with status code {e.response.status_code} and error: {error_message}" ) - response.raise_for_status() - - response_json = response.json() - return SessionsRemoteFileMetadata.from_dict(response_json["$values"][0]) - @kernel_function(name="list_files", description="Lists all files in the provided Session ID") async def list_files(self) -> list[SessionsRemoteFileMetadata]: """List the files in the session pool. @@ -242,26 +251,36 @@ async def list_files(self) -> list[SessionsRemoteFileMetadata]: ) url = self._build_url_with_version( - base_url=self.pool_management_endpoint, - endpoint="python/files", + base_url=str(self.pool_management_endpoint), + endpoint="files", params={"identifier": self.settings.session_id}, ) - response = await self.http_client.get( - url=url, - ) - response.raise_for_status() - - response_json = response.json() - return [SessionsRemoteFileMetadata.from_dict(entry) for entry in response_json["$values"]] + try: + response = await self.http_client.get( + url=url, + ) + response.raise_for_status() + response_json = response.json() + return [SessionsRemoteFileMetadata.from_dict(entry["properties"]) for entry in response_json["value"]] + except HTTPStatusError as e: + error_message = e.response.text if e.response.text else e.response.reason_phrase + raise FunctionExecutionException( + f"List files failed with status code {e.response.status_code} and error: {error_message}" + ) - async def download_file(self, *, remote_file_path: str, local_file_path: str | None = None) -> BytesIO | None: + async def download_file( + self, + *, + remote_file_name: Annotated[str, "The name of the file to download, relative to /mnt/data"], + local_file_path: Annotated[str | None, "The local file path to save the file to, optional"] = None, + ) -> Annotated[BytesIO | None, "The data of the downloaded file"]: """Download a file from the session pool. Args: - remote_file_path: The path to download the file from, relative to `/mnt/data`. - local_file_path: The path to save the downloaded file to. If not provided, the - file is returned as a BufferedReader. + remote_file_name: The name of the file to download, relative to `/mnt/data`. + local_file_path: The path to save the downloaded file to. Should include the extension. + If not provided, the file is returned as a BufferedReader. Returns: BufferedReader: The data of the downloaded file. @@ -275,19 +294,24 @@ async def download_file(self, *, remote_file_path: str, local_file_path: str | N ) url = self._build_url_with_version( - base_url=self.pool_management_endpoint, - endpoint="python/downloadFile", - params={"identifier": self.settings.session_id, "filename": remote_file_path}, - ) - - response = await self.http_client.get( - url=url, + base_url=str(self.pool_management_endpoint), + endpoint=f"files/content/{remote_file_name}", + params={"identifier": self.settings.session_id}, ) - response.raise_for_status() - - if local_file_path: - with open(local_file_path, "wb") as f: - f.write(response.content) - return None - return BytesIO(response.content) + try: + response = await self.http_client.get( + url=url, + ) + response.raise_for_status() + if local_file_path: + with open(local_file_path, "wb") as f: + f.write(response.content) + return None + + return BytesIO(response.content) + except HTTPStatusError as e: + error_message = e.response.text if e.response.text else e.response.reason_phrase + raise FunctionExecutionException( + f"Download failed with status code {e.response.status_code} and error: {error_message}" + ) diff --git a/python/semantic_kernel/core_plugins/sessions_python_tool/sessions_python_settings.py b/python/semantic_kernel/core_plugins/sessions_python_tool/sessions_python_settings.py index 73453aa770ad..c6bd6ee56aeb 100644 --- a/python/semantic_kernel/core_plugins/sessions_python_tool/sessions_python_settings.py +++ b/python/semantic_kernel/core_plugins/sessions_python_tool/sessions_python_settings.py @@ -27,10 +27,10 @@ class CodeExecutionType(str, Enum): class SessionsPythonSettings(KernelBaseModel): """The Sessions Python code interpreter settings.""" - session_id: str | None = Field(default_factory=lambda: str(uuid.uuid4()), alias="identifier") + session_id: str | None = Field(default_factory=lambda: str(uuid.uuid4()), alias="identifier", exclude=True) code_input_type: CodeInputType | None = Field(default=CodeInputType.Inline, alias="codeInputType") execution_type: CodeExecutionType | None = Field(default=CodeExecutionType.Synchronous, alias="executionType") - python_code: str | None = Field(alias="pythonCode", default=None) + python_code: str | None = Field(alias="code", default=None) timeout_in_sec: int | None = Field(default=100, alias="timeoutInSeconds") sanitize_input: bool | None = Field(default=True, alias="sanitizeInput") diff --git a/python/tests/unit/core_plugins/test_sessions_python_plugin.py b/python/tests/unit/core_plugins/test_sessions_python_plugin.py index 05456ebe00dc..54efad3d1247 100644 --- a/python/tests/unit/core_plugins/test_sessions_python_plugin.py +++ b/python/tests/unit/core_plugins/test_sessions_python_plugin.py @@ -76,10 +76,22 @@ async def async_return(result): "semantic_kernel.core_plugins.sessions_python_tool.sessions_python_plugin.SessionsPythonTool._ensure_auth_token", return_value="test_token", ): - mock_request = httpx.Request(method="POST", url="https://example.com/python/execute/") + mock_request = httpx.Request(method="POST", url="https://example.com/code/execute/") mock_response = httpx.Response( - status_code=200, json={"result": "success", "stdout": "", "stderr": ""}, request=mock_request + status_code=200, + json={ + "$id": "1", + "properties": { + "$id": "2", + "status": "Success", + "stdout": "", + "stderr": "", + "result": "even_numbers = [2 * i for i in range(1, 11)]\\nprint(even_numbers)", + "executionTimeInMilliseconds": 12, + }, + }, + request=mock_request, ) mock_post.return_value = await async_return(mock_response) @@ -101,7 +113,7 @@ async def async_return(result): "semantic_kernel.core_plugins.sessions_python_tool.sessions_python_plugin.SessionsPythonTool._ensure_auth_token", return_value="test_token", ): - mock_request = httpx.Request(method="POST", url="https://example.com/python/execute/") + mock_request = httpx.Request(method="POST", url="https://example.com/code/execute/") mock_response = httpx.Response(status_code=500, request=mock_request) @@ -135,19 +147,22 @@ async def async_return(result): ), patch("builtins.open", mock_open(read_data=b"file data")), ): - mock_request = httpx.Request(method="POST", url="https://example.com/python/uploadFile?identifier=None") + mock_request = httpx.Request(method="POST", url="https://example.com/files/upload?identifier=None") mock_response = httpx.Response( status_code=200, json={ "$id": "1", - "$values": [ + "value": [ { "$id": "2", - "filename": "test.txt", - "size": 123, - "last_modified_time": "2024-06-03T17:48:46.2672398Z", - } + "properties": { + "$id": "3", + "filename": "hello.py", + "size": 123, + "lastModifiedTime": "2024-07-02T19:29:23.4369699Z", + }, + }, ], }, request=mock_request, @@ -159,10 +174,10 @@ async def async_return(result): env_file_path="test.env", ) - result = await plugin.upload_file(local_file_path="test.txt", remote_file_path="uploaded_test.txt") - assert result.filename == "test.txt" + result = await plugin.upload_file(local_file_path="hello.py", remote_file_path="hello.py") + assert result.filename == "hello.py" assert result.size_in_bytes == 123 - assert result.full_path == "/mnt/data/test.txt" + assert result.full_path == "/mnt/data/hello.py" mock_post.assert_awaited_once() @@ -181,19 +196,22 @@ async def async_return(result): ), patch("builtins.open", mock_open(read_data=b"file data")), ): - mock_request = httpx.Request(method="POST", url="https://example.com/python/uploadFile?identifier=None") + mock_request = httpx.Request(method="POST", url="https://example.com/files/upload?identifier=None") mock_response = httpx.Response( status_code=200, json={ "$id": "1", - "$values": [ + "value": [ { "$id": "2", - "filename": "test.txt", - "size": 123, - "last_modified_time": "2024-06-03T17:00:00.0000000Z", - } + "properties": { + "$id": "3", + "filename": "hello.py", + "size": 123, + "lastModifiedTime": "2024-07-02T19:29:23.4369699Z", + }, + }, ], }, request=mock_request, @@ -205,8 +223,8 @@ async def async_return(result): env_file_path="test.env", ) - result = await plugin.upload_file(local_file_path="test.txt") - assert result.filename == "test.txt" + result = await plugin.upload_file(local_file_path="hello.py") + assert result.filename == "hello.py" assert result.size_in_bytes == 123 mock_post.assert_awaited_once() @@ -235,19 +253,22 @@ async def async_return(result): ), patch("builtins.open", mock_open(read_data="print('hello, world~')")), ): - mock_request = httpx.Request(method="POST", url="https://example.com/python/uploadFile?identifier=None") + mock_request = httpx.Request(method="POST", url="https://example.com/files/upload?identifier=None") mock_response = httpx.Response( status_code=200, json={ "$id": "1", - "$values": [ + "value": [ { "$id": "2", - "filename": expected_remote_file_path, - "size": 456, - "last_modified_time": "2024-06-03T17:00:00.0000000Z", - } + "properties": { + "$id": "3", + "filename": expected_remote_file_path, + "size": 456, + "lastModifiedTime": "2024-07-02T19:29:23.4369699Z", + }, + }, ], }, request=mock_request, @@ -286,25 +307,31 @@ async def async_return(result): "semantic_kernel.core_plugins.sessions_python_tool.sessions_python_plugin.SessionsPythonTool._ensure_auth_token", return_value="test_token", ): - mock_request = httpx.Request(method="GET", url="https://example.com/python/files?identifier=None") + mock_request = httpx.Request(method="GET", url="https://example.com/files?identifier=None") mock_response = httpx.Response( status_code=200, json={ "$id": "1", - "$values": [ + "value": [ { "$id": "2", - "filename": "test1.txt", - "size": 123, - "last_modified_time": "2024-06-03T17:00:00.0000000Z", - }, # noqa: E501 + "properties": { + "$id": "3", + "filename": "hello.py", + "size": 123, + "lastModifiedTime": "2024-07-02T19:29:23.4369699Z", + }, + }, { - "$id": "3", - "filename": "test2.txt", - "size": 456, - "last_modified_time": "2024-06-03T18:00:00.0000000Z", - }, # noqa: E501 + "$id": "4", + "properties": { + "$id": "5", + "filename": "world.py", + "size": 456, + "lastModifiedTime": "2024-07-02T19:29:38.1329088Z", + }, + }, ], }, request=mock_request, @@ -315,9 +342,9 @@ async def async_return(result): files = await plugin.list_files() assert len(files) == 2 - assert files[0].filename == "test1.txt" + assert files[0].filename == "hello.py" assert files[0].size_in_bytes == 123 - assert files[1].filename == "test2.txt" + assert files[1].filename == "world.py" assert files[1].size_in_bytes == 456 mock_get.assert_awaited_once() @@ -341,7 +368,8 @@ async def mock_auth_callback(): patch("builtins.open", mock_open()) as mock_file, ): mock_request = httpx.Request( - method="GET", url="https://example.com/python/downloadFile?identifier=None&filename=remote_test.txt" + method="GET", + url="https://example.com/python/files/content/remote_text.txt?identifier=None&filename=remote_test.txt", ) mock_response = httpx.Response(status_code=200, content=b"file data", request=mock_request) @@ -352,7 +380,7 @@ async def mock_auth_callback(): env_file_path="test.env", ) - await plugin.download_file(remote_file_path="remote_test.txt", local_file_path="local_test.txt") + await plugin.download_file(remote_file_name="remote_test.txt", local_file_path="local_test.txt") mock_get.assert_awaited_once() mock_file.assert_called_once_with("local_test.txt", "wb") mock_file().write.assert_called_once_with(b"file data") @@ -374,7 +402,8 @@ async def mock_auth_callback(): return_value="test_token", ): mock_request = httpx.Request( - method="GET", url="https://example.com/python/downloadFile?identifier=None&filename=remote_test.txt" + method="GET", + url="https://example.com/files/content/remote_test.txt?identifier=None&filename=remote_test.txt", ) mock_response = httpx.Response(status_code=200, content=b"file data", request=mock_request) @@ -382,7 +411,7 @@ async def mock_auth_callback(): plugin = SessionsPythonTool(auth_callback=mock_auth_callback) - buffer = await plugin.download_file(remote_file_path="remote_test.txt") + buffer = await plugin.download_file(remote_file_name="remote_test.txt") assert buffer is not None assert buffer.read() == b"file data" mock_get.assert_awaited_once() From 4f980530d6316c9c91366ecebbe0276cf52b772d Mon Sep 17 00:00:00 2001 From: Evan Mattson Date: Wed, 3 Jul 2024 09:24:28 -0400 Subject: [PATCH 2/3] Fix exception handling --- .../sessions_python_tool/sessions_python_plugin.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/python/semantic_kernel/core_plugins/sessions_python_tool/sessions_python_plugin.py b/python/semantic_kernel/core_plugins/sessions_python_tool/sessions_python_plugin.py index 27d72d774703..47d1a28da774 100644 --- a/python/semantic_kernel/core_plugins/sessions_python_tool/sessions_python_plugin.py +++ b/python/semantic_kernel/core_plugins/sessions_python_tool/sessions_python_plugin.py @@ -69,6 +69,7 @@ def __init__( **kwargs, ) + # region Helper Methods async def _ensure_auth_token(self) -> str: """Ensure the auth token is valid.""" try: @@ -117,6 +118,9 @@ def _build_url_with_version(self, base_url, endpoint, params): endpoint = endpoint[:-1] return f"{base_url}{endpoint}?{query_string}" + # endregion + + # region Kernel Functions @kernel_function( description="""Executes the provided Python code. Start and end the code snippet with double quotes to define it as a string. @@ -180,7 +184,7 @@ async def execute_code(self, code: Annotated[str, "The valid Python code to exec error_message = e.response.text if e.response.text else e.response.reason_phrase raise FunctionExecutionException( f"Code execution failed with status code {e.response.status_code} and error: {error_message}" - ) + ) from e @kernel_function(name="upload_file", description="Uploads a file for the current Session ID") async def upload_file( @@ -233,7 +237,7 @@ async def upload_file( error_message = e.response.text if e.response.text else e.response.reason_phrase raise FunctionExecutionException( f"Upload failed with status code {e.response.status_code} and error: {error_message}" - ) + ) from e @kernel_function(name="list_files", description="Lists all files in the provided Session ID") async def list_files(self) -> list[SessionsRemoteFileMetadata]: @@ -267,7 +271,7 @@ async def list_files(self) -> list[SessionsRemoteFileMetadata]: error_message = e.response.text if e.response.text else e.response.reason_phrase raise FunctionExecutionException( f"List files failed with status code {e.response.status_code} and error: {error_message}" - ) + ) from e async def download_file( self, @@ -314,4 +318,5 @@ async def download_file( error_message = e.response.text if e.response.text else e.response.reason_phrase raise FunctionExecutionException( f"Download failed with status code {e.response.status_code} and error: {error_message}" - ) + ) from e + # endregion From 657fe44b35f70227e52e3d8800b7fbc5ed75d529 Mon Sep 17 00:00:00 2001 From: Evan Mattson Date: Wed, 3 Jul 2024 10:37:19 -0400 Subject: [PATCH 3/3] User agent adjustment --- .../core_plugins/sessions_python_tool/sessions_python_plugin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/semantic_kernel/core_plugins/sessions_python_tool/sessions_python_plugin.py b/python/semantic_kernel/core_plugins/sessions_python_tool/sessions_python_plugin.py index 47d1a28da774..fa792d499a20 100644 --- a/python/semantic_kernel/core_plugins/sessions_python_tool/sessions_python_plugin.py +++ b/python/semantic_kernel/core_plugins/sessions_python_tool/sessions_python_plugin.py @@ -216,7 +216,7 @@ async def upload_file( self.http_client.headers.update( { "Authorization": f"Bearer {auth_token}", - USER_AGENT: "SESSIONS_USER_AGENT", + USER_AGENT: SESSIONS_USER_AGENT, } )