From 5402c45d2991a9dea87c4a67dc5a67b90322c67e Mon Sep 17 00:00:00 2001 From: Brett Smith Date: Fri, 12 Jul 2024 10:57:45 -0400 Subject: [PATCH] 21935: Rename safeapi.ThreadSafeApiCache to api.ThreadSafeAPIClient Moving the class lets us delist the safeapi module from reference documentation and streamline it. Changing the class name better describes what it does (it doesn't cache anything!) and follows PEP 8 case rules. Arvados-DCO-1.1-Signed-off-by: Brett Smith --- sdk/python/arvados/__init__.py | 1 + sdk/python/arvados/api.py | 72 +++++++++++++++++++++++++--- sdk/python/arvados/collection.py | 10 ++-- sdk/python/arvados/events.py | 4 +- sdk/python/arvados/safeapi.py | 82 +++----------------------------- sdk/python/tests/test_api.py | 59 +++++++++++++++++++++++ sdk/python/tests/test_safeapi.py | 63 ------------------------ 7 files changed, 139 insertions(+), 152 deletions(-) delete mode 100644 sdk/python/tests/test_safeapi.py diff --git a/sdk/python/arvados/__init__.py b/sdk/python/arvados/__init__.py index 0aea3da640..dc0a7952c2 100644 --- a/sdk/python/arvados/__init__.py +++ b/sdk/python/arvados/__init__.py @@ -38,6 +38,7 @@ from .retry import RetryLoop # `import arvados` with previous versions of the SDK. We must keep the names # accessible even though there's no longer any functional need for them. from . import cache +from . import safeapi # Previous versions of the PySDK used to say `from .api import api`. This # made it convenient to call the API client constructor, but difficult to diff --git a/sdk/python/arvados/api.py b/sdk/python/arvados/api.py index 4756f76c28..48be190d11 100644 --- a/sdk/python/arvados/api.py +++ b/sdk/python/arvados/api.py @@ -40,6 +40,7 @@ from apiclient import discovery as apiclient_discovery from apiclient import errors as apiclient_errors from . import config from . import errors +from . import keep from . import retry from . import util from .logging import GoogleHTTPClientFilter, log_handler @@ -228,6 +229,65 @@ class SafeHTTPCache(object): raise +class ThreadSafeAPIClient(object): + """Thread-safe wrapper for an Arvados API client + + This class takes all the arguments necessary to build a lower-level + Arvados API client `googleapiclient.discovery.Resource`, then + transparently builds and wraps a unique object per thread. This works + around the fact that the client's underlying HTTP client object is not + thread-safe. + + Arguments: + + * apiconfig: Mapping[str, str] | None --- A mapping with entries for + `ARVADOS_API_HOST`, `ARVADOS_API_TOKEN`, and optionally + `ARVADOS_API_HOST_INSECURE`. If not provided, uses + `arvados.config.settings` to get these parameters from user + configuration. You can pass an empty mapping to build the client + solely from `api_params`. + + * keep_params: Mapping[str, Any] --- Keyword arguments used to construct + an associated `arvados.keep.KeepClient`. + + * api_params: Mapping[str, Any] --- Keyword arguments used to construct + each thread's API client. These have the same meaning as in the + `arvados.api.api` function. + + * version: str | None --- A string naming the version of the Arvados API + to use. If not specified, the code will log a warning and fall back to + `'v1'`. + """ + def __init__( + self, + apiconfig: Optional[Mapping[str, str]]=None, + keep_params: Optional[Mapping[str, Any]]={}, + api_params: Optional[Mapping[str, Any]]={}, + version: Optional[str]=None, + ) -> None: + if apiconfig or apiconfig is None: + self._api_kwargs = api_kwargs_from_config(version, apiconfig, **api_params) + else: + self._api_kwargs = normalize_api_kwargs(version, **api_params) + self.api_token = self._api_kwargs['token'] + self.request_id = self._api_kwargs.get('request_id') + self.local = threading.local() + self.keep = keep.KeepClient(api_client=self, **keep_params) + + def localapi(self) -> 'googleapiclient.discovery.Resource': + try: + client = self.local.api + except AttributeError: + client = api_client(**self._api_kwargs) + client._http._request_id = lambda: self.request_id or util.new_request_id() + self.local.api = client + return client + + def __getattr__(self, name: str) -> Any: + # Proxy nonexistent attributes to the thread-local API client. + return getattr(self.localapi(), name) + + def http_cache(data_type: str) -> Optional[SafeHTTPCache]: """Set up an HTTP file cache @@ -480,7 +540,7 @@ def api( *, discoveryServiceUrl: Optional[str]=None, **kwargs: Any, -) -> 'arvados.safeapi.ThreadSafeApiCache': +) -> ThreadSafeAPIClient: """Dynamically build an Arvados API client This function provides a high-level "do what I mean" interface to build an @@ -489,7 +549,7 @@ def api( like you would write in user configuration; or pass additional arguments for lower-level control over the client. - This function returns a `arvados.safeapi.ThreadSafeApiCache`, an + This function returns a `arvados.api.ThreadSafeAPIClient`, an API-compatible wrapper around `googleapiclient.discovery.Resource`. If you're handling concurrency yourself and/or your application is very performance-sensitive, consider calling `api_client` directly. @@ -528,22 +588,20 @@ def api( else: kwargs.update(api_kwargs_from_config(version)) version = kwargs.pop('version') - # We do the import here to avoid a circular import at the top level. - from .safeapi import ThreadSafeApiCache - return ThreadSafeApiCache({}, {}, kwargs, version) + return ThreadSafeAPIClient({}, {}, kwargs, version) def api_from_config( version: Optional[str]=None, apiconfig: Optional[Mapping[str, str]]=None, **kwargs: Any -) -> 'arvados.safeapi.ThreadSafeApiCache': +) -> ThreadSafeAPIClient: """Build an Arvados API client from a configuration mapping This function builds an Arvados API client from a mapping with user configuration. It accepts that mapping as an argument, so you can use a configuration that's different from what the user has set up. - This function returns a `arvados.safeapi.ThreadSafeApiCache`, an + This function returns a `arvados.api.ThreadSafeAPIClient`, an API-compatible wrapper around `googleapiclient.discovery.Resource`. If you're handling concurrency yourself and/or your application is very performance-sensitive, consider calling `api_client` directly. diff --git a/sdk/python/arvados/collection.py b/sdk/python/arvados/collection.py index b04caee4d5..e94f0a05be 100644 --- a/sdk/python/arvados/collection.py +++ b/sdk/python/arvados/collection.py @@ -28,11 +28,11 @@ import time from collections import deque from stat import * +from .api import ThreadSafeAPIClient from .arvfile import split, _FileLikeObjectBase, ArvadosFile, ArvadosFileWriter, ArvadosFileReader, WrappableFile, _BlockManager, synchronized, must_be_writable, NoopLock from .keep import KeepLocator, KeepClient from ._normalize_stream import normalize_stream, escape from ._ranges import Range, LocatorAndRange -from .safeapi import ThreadSafeApiCache import arvados.config as config import arvados.errors as errors import arvados.util @@ -1060,7 +1060,7 @@ class Collection(RichCollectionBase): settings from `apiconfig` (see below). If your client instantiates many Collection objects, you can help limit memory utilization by calling `arvados.api.api` to construct an - `arvados.safeapi.ThreadSafeApiCache`, and use that as the `api_client` + `arvados.api.ThreadSafeAPIClient`, and use that as the `api_client` for every Collection. * keep_client: arvados.keep.KeepClient | None --- The Keep client @@ -1112,8 +1112,8 @@ class Collection(RichCollectionBase): self._api_client = api_client self._keep_client = keep_client - # Use the keep client from ThreadSafeApiCache - if self._keep_client is None and isinstance(self._api_client, ThreadSafeApiCache): + # Use the keep client from ThreadSafeAPIClient + if self._keep_client is None and isinstance(self._api_client, ThreadSafeAPIClient): self._keep_client = self._api_client.keep self._block_manager = block_manager @@ -1263,7 +1263,7 @@ class Collection(RichCollectionBase): @synchronized def _my_api(self): if self._api_client is None: - self._api_client = ThreadSafeApiCache(self._config, version='v1') + self._api_client = ThreadSafeAPIClient(self._config, version='v1') if self._keep_client is None: self._keep_client = self._api_client.keep return self._api_client diff --git a/sdk/python/arvados/events.py b/sdk/python/arvados/events.py index ce049ea0a5..624a1b62f8 100644 --- a/sdk/python/arvados/events.py +++ b/sdk/python/arvados/events.py @@ -300,7 +300,7 @@ class PollClient(threading.Thread): * api: arvados.api_resources.ArvadosAPIClient --- The Arvados API client used to query logs. It will be used in a separate thread, - so if it is not an instance of `arvados.safeapi.ThreadSafeApiCache` + so if it is not an instance of `arvados.api.ThreadSafeAPIClient` it should not be reused after the thread is started. * filters: arvados.events.Filter | None --- One event filter to @@ -525,7 +525,7 @@ def subscribe( * api: arvados.api_resources.ArvadosAPIClient --- The Arvados API client used to query logs. It may be used in a separate thread, - so if it is not an instance of `arvados.safeapi.ThreadSafeApiCache` + so if it is not an instance of `arvados.api.ThreadSafeAPIClient` it should not be reused after this method returns. * filters: arvados.events.Filter | None --- One event filter to diff --git a/sdk/python/arvados/safeapi.py b/sdk/python/arvados/safeapi.py index 56b92e8f08..874fb7d13c 100644 --- a/sdk/python/arvados/safeapi.py +++ b/sdk/python/arvados/safeapi.py @@ -1,81 +1,13 @@ # Copyright (C) The Arvados Authors. All rights reserved. # # SPDX-License-Identifier: Apache-2.0 -"""Thread-safe wrapper for an Arvados API client +"""arvados.safeapi - Shim compatibility module -This module provides `ThreadSafeApiCache`, a thread-safe, API-compatible -Arvados API client. -""" - -import sys -import threading - -from typing import ( - Any, - Mapping, - Optional, -) - -from . import config -from . import keep -from . import util - -api = sys.modules['arvados.api'] - -class ThreadSafeApiCache(object): - """Thread-safe wrapper for an Arvados API client - - This class takes all the arguments necessary to build a lower-level - Arvados API client `googleapiclient.discovery.Resource`, then - transparently builds and wraps a unique object per thread. This works - around the fact that the client's underlying HTTP client object is not - thread-safe. +This module used to define `arvados.safeapi.ThreadSafeApiCache`. Now it only +exists to provide backwards compatible imports. New code should prefer to +import `arvados.api`. - Arguments: - - * apiconfig: Mapping[str, str] | None --- A mapping with entries for - `ARVADOS_API_HOST`, `ARVADOS_API_TOKEN`, and optionally - `ARVADOS_API_HOST_INSECURE`. If not provided, uses - `arvados.config.settings` to get these parameters from user - configuration. You can pass an empty mapping to build the client - solely from `api_params`. - - * keep_params: Mapping[str, Any] --- Keyword arguments used to construct - an associated `arvados.keep.KeepClient`. - - * api_params: Mapping[str, Any] --- Keyword arguments used to construct - each thread's API client. These have the same meaning as in the - `arvados.api.api` function. - - * version: str | None --- A string naming the version of the Arvados API - to use. If not specified, the code will log a warning and fall back to - `'v1'`. - """ - def __init__( - self, - apiconfig: Optional[Mapping[str, str]]=None, - keep_params: Optional[Mapping[str, Any]]={}, - api_params: Optional[Mapping[str, Any]]={}, - version: Optional[str]=None, - ) -> None: - if apiconfig or apiconfig is None: - self._api_kwargs = api.api_kwargs_from_config(version, apiconfig, **api_params) - else: - self._api_kwargs = api.normalize_api_kwargs(version, **api_params) - self.api_token = self._api_kwargs['token'] - self.request_id = self._api_kwargs.get('request_id') - self.local = threading.local() - self.keep = keep.KeepClient(api_client=self, **keep_params) - - def localapi(self) -> 'googleapiclient.discovery.Resource': - try: - client = self.local.api - except AttributeError: - client = api.api_client(**self._api_kwargs) - client._http._request_id = lambda: self.request_id or util.new_request_id() - self.local.api = client - return client +@private +""" - def __getattr__(self, name: str) -> Any: - # Proxy nonexistent attributes to the thread-local API client. - return getattr(self.localapi(), name) +from .api import ThreadSafeAPIClient as ThreadSafeApiCache diff --git a/sdk/python/tests/test_api.py b/sdk/python/tests/test_api.py index 2da05e4d99..760bc7675f 100644 --- a/sdk/python/tests/test_api.py +++ b/sdk/python/tests/test_api.py @@ -23,12 +23,15 @@ from . import run_test_server from apiclient import errors as apiclient_errors from apiclient import http as apiclient_http from arvados.api import ( + ThreadSafeAPIClient, api_client, normalize_api_kwargs, api_kwargs_from_config, _googleapiclient_log_lock, ) from .arvados_testutil import fake_httplib2_response, mock_api_responses, queue_with + +import googleapiclient import httplib2.error if not mimetypes.inited: @@ -523,5 +526,61 @@ class PreCloseSocketTestCase(unittest.TestCase): self.assertEqual(c.close.call_count, expect) +class ThreadSafeAPIClientTestCase(run_test_server.TestCaseWithServers): + MAIN_SERVER = {} + + def test_constructor(self): + env_mapping = { + key: value + for key, value in os.environ.items() + if key.startswith('ARVADOS_API_') + } + extra_params = { + 'timeout': 299, + } + base_params = { + key[12:].lower(): value + for key, value in env_mapping.items() + } + try: + base_params['insecure'] = base_params.pop('host_insecure') + except KeyError: + pass + expected_keep_params = {} + for config, params, subtest in [ + (None, {}, "default arguments"), + (None, extra_params, "extra params"), + (env_mapping, {}, "explicit config"), + (env_mapping, extra_params, "explicit config and params"), + ({}, base_params, "params only"), + ]: + with self.subTest(f"test constructor with {subtest}"): + expected_timeout = params.get('timeout', 300) + expected_params = dict(params) + keep_params = dict(expected_keep_params) + client = ThreadSafeAPIClient(config, keep_params, params, 'v1') + self.assertTrue(hasattr(client, 'localapi'), "client missing localapi method") + self.assertEqual(client.api_token, os.environ['ARVADOS_API_TOKEN']) + self.assertEqual(client._http.timeout, expected_timeout) + self.assertEqual(params, expected_params, + "api_params was modified in-place") + self.assertEqual(keep_params, expected_keep_params, + "keep_params was modified in-place") + + def test_constructor_no_args(self): + client = ThreadSafeAPIClient() + self.assertTrue(hasattr(client, 'localapi'), "client missing localapi method") + self.assertEqual(client.api_token, os.environ['ARVADOS_API_TOKEN']) + self.assertTrue(client.insecure) + + def test_constructor_bad_version(self): + with self.assertRaises(googleapiclient.errors.UnknownApiNameOrVersion): + ThreadSafeAPIClient(version='BadTestVersion') + + def test_pre_v3_0_name(self): + from arvados.safeapi import ThreadSafeApiCache + self.assertIs(ThreadSafeApiCache, ThreadSafeAPIClient) + + if __name__ == '__main__': unittest.main() diff --git a/sdk/python/tests/test_safeapi.py b/sdk/python/tests/test_safeapi.py deleted file mode 100644 index a41219e9c5..0000000000 --- a/sdk/python/tests/test_safeapi.py +++ /dev/null @@ -1,63 +0,0 @@ -# Copyright (C) The Arvados Authors. All rights reserved. -# -# SPDX-License-Identifier: Apache-2.0 - -import os -import unittest - -import googleapiclient - -from arvados import safeapi - -from . import run_test_server - -class SafeApiTest(run_test_server.TestCaseWithServers): - MAIN_SERVER = {} - - def test_constructor(self): - env_mapping = { - key: value - for key, value in os.environ.items() - if key.startswith('ARVADOS_API_') - } - extra_params = { - 'timeout': 299, - } - base_params = { - key[12:].lower(): value - for key, value in env_mapping.items() - } - try: - base_params['insecure'] = base_params.pop('host_insecure') - except KeyError: - pass - expected_keep_params = {} - for config, params, subtest in [ - (None, {}, "default arguments"), - (None, extra_params, "extra params"), - (env_mapping, {}, "explicit config"), - (env_mapping, extra_params, "explicit config and params"), - ({}, base_params, "params only"), - ]: - with self.subTest(f"test constructor with {subtest}"): - expected_timeout = params.get('timeout', 300) - expected_params = dict(params) - keep_params = dict(expected_keep_params) - client = safeapi.ThreadSafeApiCache(config, keep_params, params, 'v1') - self.assertTrue(hasattr(client, 'localapi'), "client missing localapi method") - self.assertEqual(client.api_token, os.environ['ARVADOS_API_TOKEN']) - self.assertEqual(client._http.timeout, expected_timeout) - self.assertEqual(params, expected_params, - "api_params was modified in-place") - self.assertEqual(keep_params, expected_keep_params, - "keep_params was modified in-place") - - def test_constructor_no_args(self): - client = safeapi.ThreadSafeApiCache() - self.assertTrue(hasattr(client, 'localapi'), "client missing localapi method") - self.assertEqual(client.api_token, os.environ['ARVADOS_API_TOKEN']) - self.assertTrue(client.insecure) - - def test_constructor_bad_version(self): - with self.assertRaises(googleapiclient.errors.UnknownApiNameOrVersion): - safeapi.ThreadSafeApiCache(version='BadTestVersion') -- 2.30.2