From b096bcf52bec20f20b610395aef0b755a4cdf1bc Mon Sep 17 00:00:00 2001 From: skyflow-vivek Date: Tue, 4 Mar 2025 18:21:07 +0530 Subject: [PATCH 1/3] SK-1906 Improve debugging in connections --- skyflow/utils/_utils.py | 5 +++++ skyflow/vault/controller/_connections.py | 3 ++- tests/vault/controller/test__connection.py | 20 ++++++++++++++++++++ 3 files changed, 27 insertions(+), 1 deletion(-) diff --git a/skyflow/utils/_utils.py b/skyflow/utils/_utils.py index 5002956a..d3c21071 100644 --- a/skyflow/utils/_utils.py +++ b/skyflow/utils/_utils.py @@ -341,6 +341,11 @@ def parse_invoke_connection_response(api_response: requests.Response): if 'x-request-id' in api_response.headers: message += ' - request id: ' + api_response.headers['x-request-id'] + + if 'error-from-client' in api_response.headers: + error_from_client = api_response.headers['error-from-client'] + details = [{ "error_from_client": error_from_client }] + raise SkyflowError(message, status_code, details=details) raise SkyflowError(message, status_code) diff --git a/skyflow/vault/controller/_connections.py b/skyflow/vault/controller/_connections.py index 2fc52f11..9f067d92 100644 --- a/skyflow/vault/controller/_connections.py +++ b/skyflow/vault/controller/_connections.py @@ -27,7 +27,7 @@ def invoke(self, request: InvokeConnectionRequest): invoke_connection_request.headers['sky-metadata'] = json.dumps(get_metrics()) - log_info(SkyflowMessages.Info.INVOKE_CONNECTION_TRIGGERED, self.__vault_client.get_logger()) + log_info(SkyflowMessages.Info.INVOKE_CONNECTION_TRIGGERED.value, self.__vault_client.get_logger()) try: response = session.send(invoke_connection_request) @@ -36,5 +36,6 @@ def invoke(self, request: InvokeConnectionRequest): return invoke_connection_response except Exception as e: + if isinstance(e, SkyflowError): raise e raise SkyflowError(SkyflowMessages.Error.INVOKE_CONNECTION_FAILED.value, SkyflowMessages.ErrorCodes.SERVER_ERROR.value) \ No newline at end of file diff --git a/tests/vault/controller/test__connection.py b/tests/vault/controller/test__connection.py index 0bd3d293..5b5a106d 100644 --- a/tests/vault/controller/test__connection.py +++ b/tests/vault/controller/test__connection.py @@ -100,5 +100,25 @@ def test_invoke_request_error(self, mock_send): with self.assertRaises(SkyflowError) as context: self.connection.invoke(request) self.assertEqual(context.exception.message, SkyflowMessages.Error.INVOKE_CONNECTION_FAILED.value) + self.assertTrue(context.exception.details['error_from_client']) + @patch('requests.Session.send') + def test_invoke_request_error_from_client(self, mock_send): + mock_response = Mock() + mock_response.status_code = FAILURE_STATUS_CODE + mock_response.content = ERROR_RESPONSE_CONTENT + mock_response.headers = {'error-from-client': True} + mock_send.return_value = mock_response + + request = InvokeConnectionRequest( + method=RequestMethod.POST, + body=VALID_BODY, + path_params=VALID_PATH_PARAMS, + headers=VALID_HEADERS, + query_params=VALID_QUERY_PARAMS + ) + with self.assertRaises(SkyflowError) as context: + self.connection.invoke(request) + self.assertEqual(context.exception.message, SkyflowMessages.Error.INVOKE_CONNECTION_FAILED.value) + self.assertTrue(context.exception.details['error_from_client']) From 74628e555634735b4f8c3645c309036a8c287b2b Mon Sep 17 00:00:00 2001 From: saileshwar-skyflow Date: Tue, 4 Mar 2025 22:25:09 +0530 Subject: [PATCH 2/3] SK-1906: Fix invoke connection test case --- tests/vault/controller/test__connection.py | 36 ++++++++++------------ 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/tests/vault/controller/test__connection.py b/tests/vault/controller/test__connection.py index 5b5a106d..9723b91a 100644 --- a/tests/vault/controller/test__connection.py +++ b/tests/vault/controller/test__connection.py @@ -1,8 +1,8 @@ import unittest from unittest.mock import Mock, patch - +import requests from skyflow.error import SkyflowError -from skyflow.utils import SkyflowMessages +from skyflow.utils import SkyflowMessages, parse_invoke_connection_response from skyflow.utils.enums import RequestMethod from skyflow.vault.connection import InvokeConnectionRequest from skyflow.vault.controller import Connection @@ -21,7 +21,7 @@ INVALID_HEADERS = "invalid_headers" INVALID_BODY = "invalid_body" FAILURE_STATUS_CODE = 400 -ERROR_RESPONSE_CONTENT = '{"error": {"message": "error occurred"}}' +ERROR_RESPONSE_CONTENT = b'{"error": {"message": "Invalid Request"}}' class TestConnection(unittest.TestCase): def setUp(self): @@ -100,25 +100,21 @@ def test_invoke_request_error(self, mock_send): with self.assertRaises(SkyflowError) as context: self.connection.invoke(request) self.assertEqual(context.exception.message, SkyflowMessages.Error.INVOKE_CONNECTION_FAILED.value) - self.assertTrue(context.exception.details['error_from_client']) - @patch('requests.Session.send') - def test_invoke_request_error_from_client(self, mock_send): - mock_response = Mock() + def test_parse_invoke_connection_response_error_from_client(self): + mock_response = Mock(spec=requests.Response) mock_response.status_code = FAILURE_STATUS_CODE mock_response.content = ERROR_RESPONSE_CONTENT - mock_response.headers = {'error-from-client': True} - mock_send.return_value = mock_response - - request = InvokeConnectionRequest( - method=RequestMethod.POST, - body=VALID_BODY, - path_params=VALID_PATH_PARAMS, - headers=VALID_HEADERS, - query_params=VALID_QUERY_PARAMS - ) + mock_response.headers = { + 'error-from-client': 'true', + 'x-request-id': '12345' + } + mock_response.raise_for_status.side_effect = requests.HTTPError() with self.assertRaises(SkyflowError) as context: - self.connection.invoke(request) - self.assertEqual(context.exception.message, SkyflowMessages.Error.INVOKE_CONNECTION_FAILED.value) - self.assertTrue(context.exception.details['error_from_client']) + parse_invoke_connection_response(mock_response) + + exception = context.exception + + self.assertTrue(any(detail.get('error_from_client') == 'true' for detail in exception.details)) + From 9cd613b979ac0788910521a690fa427a8ae241c6 Mon Sep 17 00:00:00 2001 From: skyflow-vivek Date: Thu, 6 Mar 2025 21:05:55 +0530 Subject: [PATCH 3/3] SK-1934 Fix inconsistent error handling for invoke connections --- skyflow/utils/_skyflow_messages.py | 1 + skyflow/utils/_utils.py | 39 +++++++++++----------- skyflow/vault/controller/_connections.py | 3 +- tests/utils/test__utils.py | 5 +-- tests/vault/controller/test__connection.py | 11 ++++-- 5 files changed, 34 insertions(+), 25 deletions(-) diff --git a/skyflow/utils/_skyflow_messages.py b/skyflow/utils/_skyflow_messages.py index 954c5e14..26ca4a25 100644 --- a/skyflow/utils/_skyflow_messages.py +++ b/skyflow/utils/_skyflow_messages.py @@ -275,6 +275,7 @@ class ErrorLogs(Enum): UPDATE_REQUEST_REJECTED = f"{ERROR}: [{error_prefix}] Update request resulted in failure." QUERY_REQUEST_REJECTED = f"{ERROR}: [{error_prefix}] Query request resulted in failure." GET_REQUEST_REJECTED = f"{ERROR}: [{error_prefix}] Get request resulted in failure." + INVOKE_CONNECTION_REQUEST_REJECTED = f"{ERROR}: [{error_prefix}] Invoke connection request resulted in failure." class Interface(Enum): INSERT = "INSERT" diff --git a/skyflow/utils/_utils.py b/skyflow/utils/_utils.py index d3c21071..2261d3e6 100644 --- a/skyflow/utils/_utils.py +++ b/skyflow/utils/_utils.py @@ -327,29 +327,30 @@ def parse_invoke_connection_response(api_response: requests.Response): invoke_connection_response.response = json_content return invoke_connection_response - except: + except Exception as e: raise SkyflowError(SkyflowMessages.Error.RESPONSE_NOT_JSON.value.format(content), status_code) except HTTPError: - message = SkyflowMessages.Error.API_ERROR.value.format(status_code) - if api_response and api_response.content: - try: - error_response = json.loads(content) - if isinstance(error_response.get('error'), dict) and 'message' in error_response['error']: - message = error_response['error']['message'] - except json.JSONDecodeError: - message = SkyflowMessages.Error.RESPONSE_NOT_JSON.value.format(content) - - if 'x-request-id' in api_response.headers: - message += ' - request id: ' + api_response.headers['x-request-id'] - - if 'error-from-client' in api_response.headers: - error_from_client = api_response.headers['error-from-client'] - details = [{ "error_from_client": error_from_client }] - raise SkyflowError(message, status_code, details=details) - + message = SkyflowMessages.Error.API_ERROR.value.format(status_code) + try: + error_response = json.loads(content) + request_id = api_response.headers['x-request-id'] + error_from_client = api_response.headers.get('error-from-client') + + status_code = error_response.get('error', {}).get('http_code', 500) # Default to 500 if not found + http_status = error_response.get('error', {}).get('http_status') + grpc_code = error_response.get('error', {}).get('grpc_code') + details = error_response.get('error', {}).get('details') + message = error_response.get('error', {}).get('message', "An unknown error occurred.") + + if error_from_client is not None: + if details is None: details = [] + details.append({'error_from_client': error_from_client}) + + raise SkyflowError(message, status_code, request_id, grpc_code, http_status, details) + except json.JSONDecodeError: + message = SkyflowMessages.Error.RESPONSE_NOT_JSON.value.format(content) raise SkyflowError(message, status_code) - def log_and_reject_error(description, status_code, request_id, http_status=None, grpc_code=None, details=None, logger = None): raise SkyflowError(description, status_code, request_id, grpc_code, http_status, details) diff --git a/skyflow/vault/controller/_connections.py b/skyflow/vault/controller/_connections.py index 9f067d92..81c6ea10 100644 --- a/skyflow/vault/controller/_connections.py +++ b/skyflow/vault/controller/_connections.py @@ -3,7 +3,7 @@ from skyflow.error import SkyflowError from skyflow.utils import construct_invoke_connection_request, SkyflowMessages, get_metrics, \ parse_invoke_connection_response -from skyflow.utils.logger import log_info +from skyflow.utils.logger import log_info, log_error_log from skyflow.vault.connection import InvokeConnectionRequest @@ -36,6 +36,7 @@ def invoke(self, request: InvokeConnectionRequest): return invoke_connection_response except Exception as e: + log_error_log(SkyflowMessages.ErrorLogs.INVOKE_CONNECTION_REQUEST_REJECTED.value, self.__vault_client.get_logger()) if isinstance(e, SkyflowError): raise e raise SkyflowError(SkyflowMessages.Error.INVOKE_CONNECTION_FAILED.value, SkyflowMessages.ErrorCodes.SERVER_ERROR.value) \ No newline at end of file diff --git a/tests/utils/test__utils.py b/tests/utils/test__utils.py index c9010c98..e70afc0e 100644 --- a/tests/utils/test__utils.py +++ b/tests/utils/test__utils.py @@ -344,7 +344,8 @@ def test_parse_invoke_connection_response_http_error_with_json_error_message(sel with self.assertRaises(SkyflowError) as context: parse_invoke_connection_response(mock_response) - self.assertEqual(context.exception.message, "Not Found - request id: 1234") + self.assertEqual(context.exception.message, "Not Found") + self.assertEqual(context.exception.request_id, "1234") @patch("requests.Response") def test_parse_invoke_connection_response_http_error_without_json_error_message(self, mock_response): @@ -357,7 +358,7 @@ def test_parse_invoke_connection_response_http_error_without_json_error_message( with self.assertRaises(SkyflowError) as context: parse_invoke_connection_response(mock_response) - self.assertEqual(context.exception.message, SkyflowMessages.Error.RESPONSE_NOT_JSON.value.format("Internal Server Error") + " - request id: 1234") + self.assertEqual(context.exception.message, SkyflowMessages.Error.RESPONSE_NOT_JSON.value.format("Internal Server Error")) @patch("skyflow.utils._utils.log_and_reject_error") def test_handle_exception_json_error(self, mock_log_and_reject_error): diff --git a/tests/vault/controller/test__connection.py b/tests/vault/controller/test__connection.py index 9723b91a..61be3163 100644 --- a/tests/vault/controller/test__connection.py +++ b/tests/vault/controller/test__connection.py @@ -4,6 +4,7 @@ from skyflow.error import SkyflowError from skyflow.utils import SkyflowMessages, parse_invoke_connection_response from skyflow.utils.enums import RequestMethod +from skyflow.utils._version import SDK_VERSION from skyflow.vault.connection import InvokeConnectionRequest from skyflow.vault.controller import Connection @@ -21,7 +22,8 @@ INVALID_HEADERS = "invalid_headers" INVALID_BODY = "invalid_body" FAILURE_STATUS_CODE = 400 -ERROR_RESPONSE_CONTENT = b'{"error": {"message": "Invalid Request"}}' +ERROR_RESPONSE_CONTENT = '"message": "Invalid Request"' +ERROR_FROM_CLIENT_RESPONSE_CONTENT = b'{"error": {"message": "Invalid Request"}}' class TestConnection(unittest.TestCase): def setUp(self): @@ -99,12 +101,14 @@ def test_invoke_request_error(self, mock_send): with self.assertRaises(SkyflowError) as context: self.connection.invoke(request) - self.assertEqual(context.exception.message, SkyflowMessages.Error.INVOKE_CONNECTION_FAILED.value) + self.assertEqual(context.exception.message, f'Skyflow Python SDK {SDK_VERSION} Response {ERROR_RESPONSE_CONTENT} is not valid JSON.') + self.assertEqual(context.exception.message, SkyflowMessages.Error.RESPONSE_NOT_JSON.value.format(ERROR_RESPONSE_CONTENT)) + self.assertEqual(context.exception.http_code, 400) def test_parse_invoke_connection_response_error_from_client(self): mock_response = Mock(spec=requests.Response) mock_response.status_code = FAILURE_STATUS_CODE - mock_response.content = ERROR_RESPONSE_CONTENT + mock_response.content = ERROR_FROM_CLIENT_RESPONSE_CONTENT mock_response.headers = { 'error-from-client': 'true', 'x-request-id': '12345' @@ -117,4 +121,5 @@ def test_parse_invoke_connection_response_error_from_client(self): exception = context.exception self.assertTrue(any(detail.get('error_from_client') == 'true' for detail in exception.details)) + self.assertEqual(exception.request_id, '12345')