Repository: civisanalytics/civis-python

Issue:
Caching confusion in the client
Heard of a use case recently where a user had a notebook instance open for a while and ran into an issue with caching.  The user called `APIClient(resources='all')` but did not see new endpoints they expected to see (because they already called it earlier and the results were cached before the endpoints were created).

My impression is that it's not trivial to refresh `lru_cache` in smart way to avoid this and since this is a rare occurrence, we should address it with documentation.

Discussion:

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 2a4adde..1c18ab7 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -17,6 +17,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - Bumped the minimum required version of `joblib` to v1.3.0,
   which is the version where `joblib.parallel_config` was introduced and
   `joblib.parallel_backend` was deprecated. (#488)
+- Improved the startup time of `import civis` with a 5x speed boost. (#490)
+- The downloaded API spec due to the `civis.APIClient` instantiation is now
+  a time-to-live cache in memory (15 minutes for interactive Python, or 24 hours in scripts). (#491)
 
 ### Deprecated
 ### Removed
diff --git a/docs/source/client.rst b/docs/source/client.rst
index 38180df..5d2dde0 100644
--- a/docs/source/client.rst
+++ b/docs/source/client.rst
@@ -37,15 +37,34 @@ Below are examples of endpoints and how they map to API Client methods:
 | ``GET /workflows/1/executions/2`` | ``client.workflows.get_executions(1, 2)`` |
 +-----------------------------------+-------------------------------------------+
 
-Note that Python's built-in ``help`` function can be used to see lists of
+If your code editor has auto-completion functionality (as many heavy IDEs do),
+typing ``client.`` or ``client.workflows.`` should trigger the display of
+the available resources or methods, respectively.
+If you're running Python interactively
+(e.g., the regular Python interactive shell, IPython, or a Jupyter notebook),
+Python's built-in ``help`` function can be used to see lists of
 available endpoints for a resource (e.g., ``help(client.workflows)``) or to get
 documentation for a specific endpoint function (e.g.,
 ``help(client.workflows.list)``). The ``?`` operator in IPython (e.g., ``?client.workflows``) and the ``shift-tab``
 hotkey in a Jupyter notebook also cause documentation to be displayed.
 
-By default, the Civis API specification specification is downloaded from
-the ``/endpoints`` endpoint the first time :class:`~civis.APIClient` is
-instantiated (and cached in memory for the remainder of the program's run).
+By default, the Civis API specification is downloaded from
+the ``/endpoints`` endpoint the first time an :class:`~civis.APIClient` object is
+instantiated.
+To reduce overhead due to re-downloading the same API spec multiple times when multiple
+``client`` instances are created, this API spec is cached in memory for a set amount of time.
+If you're running Python interactively
+(e.g., the regular Python interactive shell, IPython, or a Jupyter notebook),
+the cached spec expires in 15 minutes,
+and if you're running a script, the spec expires in 24 hours.
+When a cached spec expires and a new ``client`` instance is created,
+a new spec is downloaded from the Civis API
+(so that updates to the Civis API, if any, are available to the new ``client``).
+If you want to force a new spec to be downloaded, you can pass
+``force_refresh_api_spec=True`` to the :class:`~civis.APIClient` constructor.
+Note that for a given :class:`~civis.APIClient` object, the auto-generated resources and methods
+attached to it are never refreshed, even if the Civis API is updated during the lifetime of this object.
+
 In some circumstances, it may be useful to use a local cache of the API
 specification rather than downloading the spec.  This can be done by passing
 the specification to the client through the parameter ``local_api_spec`` as
diff --git a/src/civis/client.py b/src/civis/client.py
index 61370dc..243a7f0 100644
--- a/src/civis/client.py
+++ b/src/civis/client.py
@@ -35,10 +35,18 @@ class APIClient:
         downloaded the first time APIClient is instantiated. Alternatively,
         a local cache of the specification may be passed as either an
         OrderedDict or a filename which points to a json file.
+    force_refresh_api_spec : bool, optional
+        Whether to force re-downloading the API spec,
+        even if the cached version for the given API key hasn't expired.
     """
 
     def __init__(
-        self, api_key=None, return_type="snake", api_version="1.0", local_api_spec=None
+        self,
+        api_key=None,
+        return_type="snake",
+        api_version="1.0",
+        local_api_spec=None,
+        force_refresh_api_spec=False,
     ):
         if return_type not in _RETURN_TYPES:
             raise ValueError(
@@ -50,7 +58,7 @@ class APIClient:
         self.last_response = None
 
         classes = generate_classes_maybe_cached(
-            local_api_spec, session_auth_key, api_version
+            local_api_spec, session_auth_key, api_version, force_refresh_api_spec
         )
         for class_name, cls in classes.items():
             setattr(
diff --git a/src/civis/client.pyi b/src/civis/client.pyi
index cca547f..fc246c8 100644
--- a/src/civis/client.pyi
+++ b/src/civis/client.pyi
@@ -56183,6 +56183,7 @@ class APIClient:
         return_type: str = ...,
         api_version: str = ...,
         local_api_spec: OrderedDict | str | None = ...,
+        force_refresh_api_spec: bool = ...,
     ): ...
     def get_aws_credential_id(
         self,
diff --git a/src/civis/resources/_client_pyi.py b/src/civis/resources/_client_pyi.py
index f1f1b80..0ad2954 100644
--- a/src/civis/resources/_client_pyi.py
+++ b/src/civis/resources/_client_pyi.py
@@ -83,6 +83,7 @@ class APIClient:
         return_type: str = ...,
         api_version: str = ...,
         local_api_spec: OrderedDict | str | None = ...,
+        force_refresh_api_spec: bool = ...,
     ): ...
     def get_aws_credential_id(
         self,
diff --git a/src/civis/resources/_resources.py b/src/civis/resources/_resources.py
index 4bf4e2b..714ec8a 100644
--- a/src/civis/resources/_resources.py
+++ b/src/civis/resources/_resources.py
@@ -3,6 +3,8 @@ from functools import lru_cache
 import json
 import os
 import re
+import sys
+import time
 import textwrap
 from inspect import Signature, Parameter
 
@@ -537,7 +539,6 @@ def parse_api_spec(api_spec, api_version):
     return classes
 
 
-@lru_cache(maxsize=4)
 def get_api_spec(api_key, api_version="1.0", user_agent="civis-python"):
     """Download the Civis API specification.
 
@@ -549,7 +550,7 @@ def get_api_spec(api_key, api_version="1.0", user_agent="civis-python"):
         The version of endpoints to call. May instantiate multiple client
         objects with different versions.  Currently only "1.0" is supported.
     user_agent : string, optional
-        Provide this user agent to the the Civis API, along with an
+        Provide this user agent to the Civis API, along with an
         API client version tag and ``requests`` version tag.
     """
     if api_version == "1.0":
@@ -568,7 +569,35 @@ def get_api_spec(api_key, api_version="1.0", user_agent="civis-python"):
     return spec
 
 
+def _running_interactively():
+    # https://stackoverflow.com/a/64523765
+    return hasattr(sys, "ps1")
+
+
+@lru_cache
+def _spec_expire_time():
+    """Return the duration (seconds) after which a cached API spec expires."""
+    if _running_interactively():
+        return 60 * 15  # 15 minutes
+    else:
+        return 60 * 60 * 24  # 24 hours
+
+
+def _get_ttl_hash():
+    """Return the same value within `seconds` time period."""
+    seconds = _spec_expire_time()
+    return round(time.time() / seconds)
+
+
 @lru_cache(maxsize=4)
+def generate_classes_ttl_cache(api_key, api_version, ttl_hash):
+    """Wraps generate_classes with a time-to-live cache.
+
+    https://stackoverflow.com/a/55900800
+    """
+    return generate_classes(api_key, api_version)
+
+
 def generate_classes(api_key, api_version="1.0"):
     """Dynamically create classes to interface with the Civis API.
 
@@ -617,10 +646,22 @@ def cache_api_spec(cache=CACHED_SPEC_PATH, api_key=None, api_version="1.0"):
         json.dump(spec, _fout)
 
 
-def generate_classes_maybe_cached(cache, api_key, api_version):
+def generate_classes_maybe_cached(
+    cache,
+    api_key,
+    api_version,
+    force_refresh_api_spec=False,
+):
     """Generate class objects either from /endpoints or a local cache."""
+    if cache and force_refresh_api_spec:
+        raise TypeError(
+            "you cannot provide a local spec and "
+            "clear the in-memory cached specs at the same time"
+        )
+    if force_refresh_api_spec:
+        generate_classes_ttl_cache.cache_clear()
     if cache is None:
-        classes = generate_classes(api_key, api_version)
+        classes = generate_classes_ttl_cache(api_key, api_version, _get_ttl_hash())
     else:
         if isinstance(cache, OrderedDict):
             raw_spec = cache


Test patch:
diff --git a/tests/test_resources.py b/tests/test_resources.py
index 235fe45..71be0a2 100644
--- a/tests/test_resources.py
+++ b/tests/test_resources.py
@@ -1,6 +1,7 @@
 import json
 import os
 import tempfile
+import time
 from collections import defaultdict, OrderedDict
 from unittest import mock
 
@@ -655,3 +656,35 @@ def test_client_pyi_matches_api_spec():
             "client.pyi doesn't match the API spec in the codebase. "
             "Run tools/update_civis_api_spec.py."
         )
+
+
+@mock.patch("civis.resources._resources.generate_classes")
+def test_generate_classes_with_ttl_cache__base_case(mock_gen):
+    # Calling generate_classes_maybe_cached multiple times should only call
+    # generate_classes once.
+    for _ in range(5):
+        _resources.generate_classes_maybe_cached(None, "api-key", "1.0")
+    assert mock_gen.call_count == 1
+
+
+@mock.patch("civis.resources._resources.generate_classes")
+def test_generate_classes_with_ttl_cache__force_clear_cache(mock_gen):
+    # Check that we can force clear the cache.
+    for _ in range(5):
+        _resources.generate_classes_maybe_cached(
+            None, "api-key", "1.0", force_refresh_api_spec=True
+        )
+    assert mock_gen.call_count == 5
+
+
+@mock.patch("civis.resources._resources._spec_expire_time")
+@mock.patch("civis.resources._resources.generate_classes")
+def test_generate_classes_with_ttl_cache__expire_cache(mock_gen, mock_expire_time):
+    # Check that we can expire the cache.
+    mock_expire_time.return_value = 0.001
+    for _ in range(5):
+        _resources.generate_classes_maybe_cached(None, "api-key", "1.0")
+        # Sleep the same amount of time as the mock_expire_time,
+        # so that we should trigger the cache clearing at every iteration.
+        time.sleep(0.001)
+    assert mock_gen.call_count == 5