From 9a2c7496b1dc85732d675251995b16c08989fa82 Mon Sep 17 00:00:00 2001 From: nanglo123 <47036385+nanglo123@users.noreply.github.com> Date: Tue, 8 Oct 2024 11:57:03 -0400 Subject: [PATCH] Fix version pins (#192) This PR fixes a bug where `VERSION_PINS` is always set to an empty dictionary due to a misaligned indent. We also handle invalid prefixes and version datatypes more gracefully and make the logger more informative. --------- Co-authored-by: Charles Tapley Hoyt --- src/pyobo/api/utils.py | 55 +++++++++++++++++++++++++++++++++++--- src/pyobo/constants.py | 31 --------------------- tests/test_version_pins.py | 45 +++++++++++++++++++++++++++++++ 3 files changed, 97 insertions(+), 34 deletions(-) create mode 100644 tests/test_version_pins.py diff --git a/src/pyobo/api/utils.py b/src/pyobo/api/utils.py index 2d683006..180d0399 100644 --- a/src/pyobo/api/utils.py +++ b/src/pyobo/api/utils.py @@ -3,18 +3,23 @@ """Utilities for high-level API.""" import json +import logging +import os +from functools import lru_cache from typing import Optional import bioversions -from ..constants import VERSION_PINS from ..utils.path import prefix_directory_join __all__ = [ "get_version", + "get_version_pins", "VersionError", ] +logger = logging.getLogger(__name__) + class VersionError(ValueError): """A catch-all for version getting failure.""" @@ -26,8 +31,8 @@ def get_version(prefix: str) -> Optional[str]: :param prefix: the resource name :return: The version if available else None """ - # Prioritize loaded environmental variable VERSION_PINS dictionary - version = VERSION_PINS.get(prefix) + # Prioritize loaded environment variable PYOBO_VERSION_PINS dictionary + version = get_version_pins().get(prefix) if version: return version try: @@ -46,3 +51,47 @@ def get_version(prefix: str) -> Optional[str]: return data["version"] return None + + +@lru_cache(1) +def get_version_pins() -> dict[str, str]: + """Retrieve user-defined resource version pins. + + To set your own resource pins, set your machine's environmental variable + "PYOBO_VERSION_PINS" to a JSON string containing string resource prefixes + as keys and string versions of their respective resource as values. + Constraining version pins will make PyOBO rely on cached versions of a resource. + A user might want to pin resource versions that are used by PyOBO due to + the fact that PyOBO will download the latest version of a resource if it is + not pinned. This downloading process can lead to a slow-down in downstream + applications that rely on PyOBO. + """ + version_pins_str = os.getenv("PYOBO_VERSION_PINS") + if not version_pins_str: + return {} + + try: + version_pins = json.loads(version_pins_str) + except ValueError as e: + logger.error( + "The value for the environment variable PYOBO_VERSION_PINS " + "must be a valid JSON string: %s", + e, + ) + return {} + + for prefix, version in list(version_pins.items()): + if not isinstance(prefix, str) or not isinstance(version, str): + logger.error(f"The prefix:{prefix} and version:{version} name must both be strings") + del version_pins[prefix] + + logger.debug( + f"These are the resource versions that are pinned.\n" + f"{version_pins}. " + f"\nPyobo will download the latest version of a resource if it's " + f"not pinned.\nIf you want to use a specific version of a " + f"resource, edit your PYOBO_VERSION_PINS environmental " + f"variable which is a JSON string to include a prefix and version " + f"name." + ) + return version_pins diff --git a/src/pyobo/constants.py b/src/pyobo/constants.py index 0c018c98..16e6d4e1 100644 --- a/src/pyobo/constants.py +++ b/src/pyobo/constants.py @@ -2,9 +2,7 @@ """Constants for PyOBO.""" -import json import logging -import os import re import pystow @@ -13,7 +11,6 @@ "RAW_DIRECTORY", "DATABASE_DIRECTORY", "SPECIES_REMAPPING", - "VERSION_PINS", ] logger = logging.getLogger(__name__) @@ -101,31 +98,3 @@ "isbn", "issn", } - -# Load version pin dictionary from the environmental variable VERSION_PINS -try: - VERSION_PINS_STR = os.getenv("VERSION_PINS") - if not VERSION_PINS_STR: - VERSION_PINS = {} - else: - VERSION_PINS = json.loads(VERSION_PINS_STR) - for k, v in VERSION_PINS.items(): - if not isinstance(k, str) or not isinstance(v, str): - logger.error("The prefix and version name must both be " "strings") - VERSION_PINS = {} - break -except ValueError as e: - logger.error( - "The value for the environment variable VERSION_PINS must be a valid JSON string: %s" % e - ) - VERSION_PINS = {} - -if VERSION_PINS: - logger.debug( - f"These are the resource versions that are pinned.\n{VERSION_PINS}. " - f"\nPyobo will download the latest version of a resource if it's " - f"not pinned.\nIf you want to use a specific version of a " - f"resource, edit your VERSION_PINS environmental " - f"variable which is a JSON string to include a prefix and version " - f"name." - ) diff --git a/tests/test_version_pins.py b/tests/test_version_pins.py new file mode 100644 index 00000000..1fb04a42 --- /dev/null +++ b/tests/test_version_pins.py @@ -0,0 +1,45 @@ +# -*- coding: utf-8 -*- + +"""Tests for PyOBO version pins.""" +import os +import unittest +from unittest import mock + +from pyobo.api.utils import get_version, get_version_pins + +MOCK_PYOBO_VERSION_PINS = '{"ncbitaxon": "2024-07-03", "vo":"2024-04-09", "chebi":"235", "bfo":5}' +FAULTY_MOCK_PYOBO_VERSION_PINS = "{'ncbitaxon': '2024-07-03'}" + + +@mock.patch.dict(os.environ, {"PYOBO_VERSION_PINS": MOCK_PYOBO_VERSION_PINS}) +class TestVersionPins(unittest.TestCase): + """Test using user-defined version pins.""" + + def setUp(self): + """Clear the cache before each test case.""" + get_version_pins.cache_clear() + + def test_correct_version_pin_types(self): + """Test resource and version type.""" + version_pins = get_version_pins() + for resource_prefix, version in version_pins.items(): + self.assertIsInstance(resource_prefix, str) + self.assertIsInstance(version, str) + + def test_use_correct_version_pin(self): + """Tests correct resource version is used.""" + version_pins = get_version_pins() + for resource_prefix, version in version_pins.items(): + self.assertEqual(get_version(resource_prefix), version) + + @mock.patch.dict(os.environ, {"PYOBO_VERSION_PINS": ""}) + def test_empty_version_pins(self): + """Test empty version pins are processed correctly.""" + version_pins = get_version_pins() + self.assertFalse(version_pins) + + @mock.patch.dict(os.environ, {"PYOBO_VERSION_PINS": FAULTY_MOCK_PYOBO_VERSION_PINS}) + def test_incorrectly_set_version_pins(self): + """Test erroneously set version pins are processed correctly.""" + version_pins = get_version_pins() + self.assertFalse(version_pins)