From 89ed0688726ce79e4dea9122560beb9ddb1816d1 Mon Sep 17 00:00:00 2001 From: Li Wan <49334982+wanliAlex@users.noreply.github.com> Date: Tue, 18 Jul 2023 11:43:29 +1000 Subject: [PATCH] Skip version check for cloud v2 rightnow (#124) skip the version check for cloud v2 --- src/marqo/client.py | 44 ++++++++++++++---- tests/v0_tests/test_client.py | 84 +++++++++++++++++++++++++++++++---- 2 files changed, 111 insertions(+), 17 deletions(-) diff --git a/src/marqo/client.py b/src/marqo/client.py index 255f4a33..0a615894 100644 --- a/src/marqo/client.py +++ b/src/marqo/client.py @@ -2,6 +2,7 @@ from typing import Any, Dict, List, Optional, Union from pydantic import error_wrappers +from requests.exceptions import RequestException from marqo.index import Index from marqo.config import Config @@ -11,8 +12,10 @@ from marqo import errors from marqo.version import minimum_supported_marqo_version from marqo.marqo_logging import mq_logger +from marqo.errors import MarqoWebError # we want to avoid name conflicts with marqo.version from packaging import version as versioning_helpers +from json import JSONDecodeError # A dictionary to cache the marqo url and version for compatibility check marqo_url_and_version_cache = {} @@ -199,12 +202,37 @@ def get_cpu_info(self): return self.http.get(path="device/cpu") def _marqo_minimum_supported_version_check(self): - if self.url not in marqo_url_and_version_cache: - marqo_url_and_version_cache[self.url] = self.get_marqo()["version"] - marqo_version = marqo_url_and_version_cache[self.url] - if versioning_helpers.parse(marqo_version) < versioning_helpers.parse(minimum_supported_marqo_version()): - mq_logger.warning(f"Your Marqo Python client requires a minimum Marqo version of " - f"{minimum_supported_marqo_version()} to function properly, while your Marqo version is {marqo_version}. " - f"Please upgrade your Marqo instance to avoid potential errors. " - f"If you have already upgraded your Marqo instance but still having this warning, please import you Marqo Python client again and retry.") + min_ver = minimum_supported_marqo_version() + skip_warning_message = ( + f"Marqo encountered a problem trying to check the Marqo version found at `{self.url}`. " + f"The minimum supported Marqo version for this client is {min_ver}. " + f"If you are sure your Marqo version is compatible with this client, you can ignore this message. ") + + # Skip the check if the url is previously labelled as "_skipped" + if self.url in marqo_url_and_version_cache and marqo_url_and_version_cache[self.url] == "_skipped": + mq_logger.warning(skip_warning_message) + return + + # Skip the check for Marqo CloudV2 APIs right now + skip_version_check_url = ["https://api.marqo.ai", "https://cloud.marqo.ai"] + if self.url in skip_version_check_url: + marqo_url_and_version_cache[self.url] = "_skipped" + mq_logger.warning(skip_warning_message) + return + + # Do version check + try: + if self.url not in marqo_url_and_version_cache: + marqo_url_and_version_cache[self.url] = self.get_marqo()["version"] + marqo_version = marqo_url_and_version_cache[self.url] + if versioning_helpers.parse(marqo_version) < versioning_helpers.parse(min_ver): + mq_logger.warning(f"Your Marqo Python client requires a minimum Marqo version of " + f"{minimum_supported_marqo_version()} to function properly, but your Marqo version is {marqo_version}. " + f"Please upgrade your Marqo instance to avoid potential errors. " + f"If you have already changed your Marqo instance but still get this warning, please restart your Marqo client Python interpreter.") + except (MarqoWebError, RequestException, TypeError, KeyError) as e: + mq_logger.warning(skip_warning_message) + marqo_url_and_version_cache[self.url] = "_skipped" + return + diff --git a/tests/v0_tests/test_client.py b/tests/v0_tests/test_client.py index f17a709a..d5746a39 100644 --- a/tests/v0_tests/test_client.py +++ b/tests/v0_tests/test_client.py @@ -1,8 +1,12 @@ +import requests +from json import JSONDecodeError from unittest import mock + from marqo.client import Client from tests.marqo_test import MarqoTestCase from marqo.errors import MarqoApiError from marqo.client import marqo_url_and_version_cache +from marqo.errors import BackendTimeoutError, BackendCommunicationError class TestClient(MarqoTestCase): @@ -33,9 +37,8 @@ def test_health(self): assert 'status' in res['backend'] def test_version_check_instantiation(self): - with mock.patch("marqo.client.mq_logger.warning") as mock_warning,\ - mock.patch("marqo.client.Client.get_marqo") as mock_get_marqo: - + with mock.patch("marqo.client.mq_logger.warning") as mock_warning, \ + mock.patch("marqo.client.Client.get_marqo") as mock_get_marqo: mock_get_marqo.return_value = {'version': '0.0.0'} client = Client(**self.client_settings) @@ -48,26 +51,89 @@ def test_version_check_instantiation(self): warning_message = mock_warning.call_args[0][0] # Assert the message is what you expect - self.assertIn("Please upgrade your Marqo instance to avoid potential errors.", warning_message) + self.assertIn("Please upgrade your Marqo instance to avoid potential errors.", warning_message) # Assert the url is in the cache self.assertIn(self.client_settings['url'], marqo_url_and_version_cache) assert marqo_url_and_version_cache[self.client_settings['url']] == '0.0.0' + def test_skip_version_check_for_cloud_v2(self): + for url in ["https://api.marqo.ai", "https://cloud.marqo.ai"]: + with mock.patch("marqo.client.mq_logger.warning") as mock_warning, \ + mock.patch("marqo.client.Client.get_marqo") as mock_get_marqo: + mock_get_marqo.return_value = {'version': '0.0.0'} + client = Client(url=url) + + mock_get_marqo.assert_not_called() + + assert url in marqo_url_and_version_cache + assert marqo_url_and_version_cache[url] == '_skipped' + + def test_skip_version_check_for_previously_labelled_url(self): + with mock.patch.dict("marqo.client.marqo_url_and_version_cache", + {self.client_settings["url"]: "_skipped"}) as mock_cache, \ + mock.patch("marqo.client.Client.get_marqo") as mock_get_marqo: + client = Client(**self.client_settings) + + mock_get_marqo.assert_not_called() + + def test_error_handling_in_version_check(self): + side_effect_list = [requests.exceptions.JSONDecodeError("test", "test", 1), BackendCommunicationError("test"), + BackendTimeoutError("test"), requests.exceptions.RequestException("test"), + KeyError("test"), KeyError("test"), requests.exceptions.Timeout("test")] + for i, side_effect in enumerate(side_effect_list): + with mock.patch("marqo.client.mq_logger.warning") as mock_warning, \ + mock.patch("marqo.client.Client.get_marqo") as mock_get_marqo: + mock_get_marqo.side_effect = side_effect + client = Client(**self.client_settings) + + mock_get_marqo.assert_called_once() + + # Check the warning was logged + mock_warning.assert_called_once() + + # Get the warning message + warning_message = mock_warning.call_args[0][0] + + # Assert the message is what you expect + self.assertIn("Marqo encountered a problem trying to check the Marqo version found", warning_message) + self.assertEqual(marqo_url_and_version_cache, dict({self.client_settings["url"]: "_skipped"})) + + marqo_url_and_version_cache.clear() + def test_version_check_multiple_instantiation(self): """Ensure that duplicated instantiation of the client does not result in multiple APIs calls of get_marqo()""" with mock.patch("marqo.client.Client.get_marqo") as mock_get_marqo: mock_get_marqo.return_value = {'version': '0.0.0'} client = Client(**self.client_settings) - mock_get_marqo.assert_called_once() - mock_get_marqo.reset_mock() + mock_get_marqo.assert_called_once() + mock_get_marqo.reset_mock() for _ in range(10): with mock.patch("marqo.client.mq_logger.warning") as mock_warning, \ - mock.patch("marqo.client.Client.get_marqo") as mock_get_marqo: - + mock.patch("marqo.client.Client.get_marqo") as mock_get_marqo: client = Client(**self.client_settings) mock_get_marqo.assert_not_called() - mock_warning.assert_called_once() \ No newline at end of file + mock_warning.assert_called_once() + + def test_skipped_version_check_multiple_instantiation(self): + """Ensure that the url labelled as `_skipped` only call get_marqo() once""" + url = "https://dummy/url" + with mock.patch("marqo.client.Client.get_marqo") as mock_get_marqo: + mock_get_marqo.side_effect = requests.exceptions.RequestException("test") + client = Client(url=url) + + mock_get_marqo.assert_called_once() + mock_get_marqo.reset_mock() + assert marqo_url_and_version_cache[url] == '_skipped' + + for _ in range(10): + with mock.patch("marqo.client.mq_logger.warning") as mock_warning, \ + mock.patch("marqo.client.Client.get_marqo") as mock_get_marqo: + client = Client(url=url) + + mock_get_marqo.assert_not_called() + # Check the warning was logged every instantiation + mock_warning.assert_called_once()