Release/v1.6.0

.github/workflows/pytest.yml

@@ -32,26 +32,38 @@ jobs:
       run: |
         pip install ruff
         ruff format --check pori_python tests
+    - name: short tests
+      run: |
+        pip list
+        pytest --junitxml=junit/test-results-${{ matrix.python-version }}.xml --cov pori_python --cov-report term --cov-report xml
+      env:
+        IPR_USER: ${{ secrets.IPR_TEST_USER }}
+        IPR_PASS: ${{ secrets.IPR_TEST_PASSWORD }}
+        GRAPHKB_USER: ${{ secrets.GKB_TEST_USER }}
+        GRAPHKB_PASS: ${{ secrets.GKB_TEST_PASS }}
+        GRAPHKB_URL: ${{ secrets.GKB_TEST_URL }}
+        EXCLUDE_INTEGRATION_TESTS: 1
+      if: matrix.python-version != 3.11
     - name: Full Tests with pytest
       run: |
         pip list
         pytest --junitxml=junit/test-results-${{ matrix.python-version }}.xml --cov pori_python --cov-report term --cov-report xml
       env:
         IPR_USER: ${{ secrets.IPR_TEST_USER }}
         IPR_PASS: ${{ secrets.IPR_TEST_PASSWORD }}
+        IPR_URL: ${{ secrets.IPR_TEST_URL }}
+        IPR_INTEGRATION_TEST_URL: ${{ secrets.IPR_INTEGRATION_TEST_URL }}
         GRAPHKB_USER: ${{ secrets.GKB_TEST_USER }}
         GRAPHKB_PASS: ${{ secrets.GKB_TEST_PASS }}
         GRAPHKB_URL: ${{ secrets.GKB_TEST_URL }}
-        # SDEV-3381 - Turn off integration tests temporarily, till efficiency is increased
-        # turn on integration tests for one python version only
-        EXCLUDE_INTEGRATION_TESTS: ${{ matrix.python-version != '3.11' }}
+      if: matrix.python-version == 3.11
     - name: Upload pytest test results
       uses: actions/upload-artifact@master
       with:
         name: pytest-results-${{ matrix.python-version }}
         path: junit/test-results-${{ matrix.python-version }}.xml
         # Use always() to always run this step to publish test results when there are test failures
-      if: matrix.python-version == 3.9
+      if: matrix.python-version == 3.11
     - name: Update code coverage report to CodeCov
       uses: codecov/codecov-action@v3
       with:
@@ -61,4 +73,4 @@ jobs:
         env_vars: OS,PYTHON
         name: codecov-umbrella
         fail_ci_if_error: true
-      if: matrix.python-version == 3.9
+      if: matrix.python-version == 3.11

.github/workflows/quick-pytest.yml

@@ -35,6 +35,7 @@ jobs:
         IPR_USER: ${{ secrets.IPR_TEST_USER }}
         IPR_PASS: ${{ secrets.IPR_TEST_PASSWORD }}
         IPR_URL: ${{ secrets.IPR_TEST_URL }}
+        IPR_INTEGRATION_TEST_URL: ${{ secrets.IPR_INTEGRATION_TEST_URL }}
         GRAPHKB_USER: ${{ secrets.GKB_TEST_USER }}
         GRAPHKB_PASS: ${{ secrets.GKB_TEST_PASS }}
         GRAPHKB_URL: ${{ secrets.GKB_TEST_URL }}

.gitignore

@@ -15,6 +15,7 @@ htmlcov
 # common virtual environment names
 venv*
 env
+.venv
 
 # editors
 .idea

pori_python/graphkb/match.py

@@ -2,7 +2,7 @@
 Functions which return Variants from GraphKB which match some input variant definition
 """
 
-from typing import Dict, List, Optional, Set, Union, cast
+from typing import Dict, List, Optional, Union, cast
 
 from pori_python.types import (
     BasicPosition,
@@ -31,14 +31,7 @@
     looks_like_rid,
     stringifyVariant,
 )
-from .vocab import (
-    get_equivalent_terms,
-    get_term_by_name,
-    get_term_tree,
-    get_terms_set,
-)
-
-FEATURES_CACHE: Set[str] = set()
+from .vocab import get_equivalent_terms, get_term_by_name, get_term_tree, get_terms_set
 
 
 def get_equivalent_features(
@@ -97,8 +90,6 @@ def get_equivalent_features(
             filters.append(
                 {'OR': [{'sourceIdVersion': source_id_version}, {'sourceIdVersion': None}]}
             )
-    elif FEATURES_CACHE and gene_name.lower() not in FEATURES_CACHE and not ignore_cache:
-        return []
     else:
         filters.append({'OR': [{'sourceId': gene_name}, {'name': gene_name}]})
 
@@ -111,22 +102,6 @@ def get_equivalent_features(
     )
 
 
-def cache_missing_features(conn: GraphKBConnection) -> None:
-    """
-    Create a cache of features that exist to avoid repeatedly querying
-    for missing features
-    """
-    genes = cast(
-        List[Ontology],
-        conn.query({'target': 'Feature', 'returnProperties': ['name', 'sourceId'], 'neighbors': 0}),
-    )
-    for gene in genes:
-        if gene['name']:
-            FEATURES_CACHE.add(gene['name'].lower())
-        if gene['sourceId']:
-            FEATURES_CACHE.add(gene['sourceId'].lower())
-
-
 def match_category_variant(
     conn: GraphKBConnection,
     reference_name: str,

pori_python/graphkb/util.py

@@ -1,27 +1,27 @@
-import requests
-from requests.adapters import HTTPAdapter
-
-import hashlib
 import json
 import logging
 import os
 import re
 import time
 from datetime import datetime
-from typing import Any, Dict, Iterable, List, Optional, Union, cast
-from urllib3.util.retry import Retry
+from typing import Dict, Iterable, List, Optional, Union, cast
 from urllib.parse import urlsplit
 
+import requests
+from requests.adapters import HTTPAdapter
+from requests_cache import CachedSession
+from requests_ratelimiter import LimiterAdapter
+from urllib3.util.retry import Retry
+
 from pori_python.types import ParsedVariant, PositionalVariant, Record
 
 from .constants import DEFAULT_LIMIT, TYPES_TO_NOTATION, AA_3to1_MAPPING
 
-QUERY_CACHE: Dict[Any, Any] = {}
-
 # name the logger after the package to make it simple to disable for packages using this one as a dependency
 # https://stackoverflow.com/questions/11029717/how-do-i-disable-log-messages-from-the-requests-library
 
 logger = logging.getLogger('graphkb')
+DEFAULT_LIMITER = LimiterAdapter(per_second=3)
 
 
 def convert_to_rid_list(records: Iterable[Record]) -> List[str]:
@@ -88,11 +88,17 @@ def millis_interval(start: datetime, end: datetime) -> int:
     return millis
 
 
-def cache_key(request_body) -> str:
-    """Create a cache key for a query request to GraphKB."""
-    body = json.dumps(request_body, sort_keys=True)
-    hash_code = hashlib.md5(f'/query{body}'.encode('utf-8')).hexdigest()
-    return hash_code
+def cache_filter(response):
+    """
+    Only cache POST requests when they are for the /query endpoint. Otherwise they are creating content
+    and should not be cached
+    """
+    request = response.request
+
+    if request.method == 'POST':
+        return request.url.rstrip('/').endswith('/query')
+
+    return True
 
 
 class GraphKBConnection:
@@ -102,23 +108,89 @@ def __init__(
         username: str = '',
         password: str = '',
         use_global_cache: bool = True,
+        cache_name: str = '',
+        only_if_cached: bool = False,
+        session: Optional[requests.Session] = None,
+        limiter_kwargs: Optional[dict] = None,
+        **session_kwargs,
     ):
-        self.http = requests.Session()
-        retries = Retry(
+        """
+        Docstring for __init__
+
+        Args:
+        - use_global_cache: cache requests across all requests to GKB
+        - cache_name: Path or connection URL to the database which stors the requests cache. see https://requests-cache.readthedocs.io/en/v0.6.4/user_guide.html#cache-name
+        - only_if_cached: this will set the cache-control header for all requests to only-if-cached which will raise 504 errors if a request does not exist in the cache already rather than making a new network request
+        """
+        session_cls = requests.Session
+        if session is not None:
+            if limiter_kwargs is not None:
+                raise NotImplementedError('cannot add limiter to an existing session')
+            if use_global_cache:
+                raise NotImplementedError(
+                    'the use_global_cache parameter should not be used with a custom session'
+                )
+            if cache_name:
+                raise NotImplementedError(
+                    'cache_name should not be used with a custom input session'
+                )
+        if not use_global_cache and cache_name:
+            raise NotImplementedError('cache_name only applies when use_global_cache is True')
+        if only_if_cached and not use_global_cache:
+            raise NotImplementedError(
+                'only_if_cached=True and use_global_cache=False are conflicting settings'
+            )
+
+        if use_global_cache:
+            session_cls = CachedSession
+            if not cache_name:
+                session_kwargs['backend'] = 'memory'
+            else:
+                session_kwargs['cache_name'] = cache_name
+            session_kwargs['allowable_methods'] = ['GET', 'POST']
+            session_kwargs['filter_fn'] = cache_filter
+            session_kwargs['cache_control'] = True
+
+        if 'PYTEST_CURRENT_TEST' in os.environ or only_if_cached:
+            if limiter_kwargs is not None:
+                logging.warning(
+                    'rate limiting is by default turned off for tests and cache-only queries. Setting limiter to None'
+                )
+                limiter_kwargs = None
+
+        if not session:
+            self.http = session_cls(**session_kwargs)
+        else:
+            self.http = session
+
+        max_retries = Retry(
             total=100,
             connect=5,
             status=5,
             backoff_factor=5,
             status_forcelist=[429, 500, 502, 503, 504],
         )
-        self.http.mount('https://', HTTPAdapter(max_retries=retries))
+
+        if limiter_kwargs:
+            adapter = LimiterAdapter(max_retries=max_retries, **limiter_kwargs)
+        else:
+            adapter = HTTPAdapter(max_retries=max_retries)
+
+        if not only_if_cached:
+            # do not need adapters at all for cache-only hits
+            self.http.mount('http://', adapter)
+            self.http.mount('https://', adapter)
+        self.only_if_cached = only_if_cached
+
         self.token = ''
         self.token_kc = ''
         self.url = url
         self.username = username
         self.password = password
-        self.headers = {'Accept': 'application/json', 'Content-Type': 'application/json'}
-        self.cache: Dict[Any, Any] = {} if not use_global_cache else QUERY_CACHE
+        self.headers = {
+            'Accept': 'application/json',
+            'Content-Type': 'application/json',
+        }
         self.request_count = 0
         self.first_request: Optional[datetime] = None
         self.last_request: Optional[datetime] = None
@@ -137,7 +209,16 @@ def load(self) -> Optional[float]:
                 return self.request_count * 1000 / msec
         return None
 
-    def request(self, endpoint: str, method: str = 'GET', **kwargs) -> Dict:
+    def request(
+        self,
+        endpoint: str,
+        method: str = 'GET',
+        headers: Optional[dict[str, str]] = None,
+        ignore_cache=False,
+        force_refresh=False,
+        only_if_cached=False,
+        **kwargs,
+    ) -> Dict:
         """Request wrapper to handle adding common headers and logging.
 
         Args:
@@ -147,6 +228,13 @@ def request(self, endpoint: str, method: str = 'GET', **kwargs) -> Dict:
         Returns:
             dict: the json response as a python dict
         """
+        if headers is None:
+            headers = {}
+
+        if ignore_cache or force_refresh:
+            headers['Cache-Control'] = 'no-cache'
+        elif only_if_cached or self.only_if_cached:
+            headers['Cache-Control'] = 'only-if-cached'
         url = join_url(self.url, endpoint)
         self.request_count += 1
         connect_timeout = 7
@@ -158,6 +246,11 @@ def request(self, endpoint: str, method: str = 'GET', **kwargs) -> Dict:
         if endpoint in ['query', 'parse']:
             timeout = (connect_timeout, read_timeout)
 
+        request_headers = {}
+        request_headers.update(self.headers)
+        if headers is not None:
+            request_headers.update(headers)
+
         start_time = datetime.now()
 
         if not self.first_request:
@@ -176,11 +269,12 @@ def request(self, endpoint: str, method: str = 'GET', **kwargs) -> Dict:
             try:
                 if need_refresh_login:
                     self.refresh_login()
+                    request_headers.update(self.headers)
                     need_refresh_login = False
 
                 self.request_count += 1
-                resp = requests.request(
-                    method, url, headers=self.headers, timeout=timeout, **kwargs
+                resp = self.http.request(
+                    method, url, headers=request_headers, timeout=timeout, **kwargs
                 )
                 if resp.status_code == 401 or resp.status_code == 403:
                     logger.debug(f'/{endpoint} - {resp.status_code} - retrying')
@@ -293,39 +387,29 @@ def login(self, username: str, password: str, pori_demo: bool = False) -> None:
     def refresh_login(self) -> None:
         self.login(self.username, self.password)
 
-    def set_cache_data(self, request_body: Dict, result: List[Record]) -> None:
-        """Explicitly add a query to the cache."""
-        hash_code = cache_key(request_body)
-        self.cache[hash_code] = result
-
     def query(
         self,
         request_body: Dict = {},
         paginate: bool = True,
-        ignore_cache: bool = False,
-        force_refresh: bool = False,
         limit: int = DEFAULT_LIMIT,
+        **kwargs,
     ) -> List[Record]:
         """
         Query GraphKB
         """
-        result: List[Record] = []
-        hash_code = ''
-
-        if not ignore_cache and paginate:
-            hash_code = cache_key(request_body)
-            if hash_code in self.cache and not force_refresh:
-                return self.cache[hash_code]
 
+        result: List[Record] = []
         while True:
-            content = self.post('query', data={**request_body, 'limit': limit, 'skip': len(result)})
+            content = self.post(
+                'query',
+                data={**request_body, 'limit': limit, 'skip': len(result)},
+                **kwargs,
+            )
             records = content['result']
             result.extend(records)
             if len(records) < limit or not paginate:
                 break
 
-        if not ignore_cache and paginate:
-            self.cache[hash_code] = result
         return result
 
     def parse(self, hgvs_string: str, requireFeatures: bool = False) -> ParsedVariant:
@@ -500,7 +584,9 @@ def stripDisplayName(displayName: str, withRef: bool = True, withRefSeq: bool =
 
 
 def stringifyVariant(
-    variant: Union[PositionalVariant, ParsedVariant], withRef: bool = True, withRefSeq: bool = True
+    variant: Union[PositionalVariant, ParsedVariant],
+    withRef: bool = True,
+    withRefSeq: bool = True,
 ) -> str:
     """
     Convert variant record to a string representation (displayName/hgvs)