From c19a9426ceb5d4721e98a1c53cc612fd09033d9b Mon Sep 17 00:00:00 2001 From: Guy Nir <37179063+guynir42@users.noreply.github.com> Date: Wed, 30 Aug 2023 15:41:20 -0700 Subject: [PATCH] Add "local" config files to allow default config to be the same for everybody (#71) --- .github/workflows/run-tests.yml | 4 +++ .gitignore | 6 +++++ README.md | 4 +-- alembic/env.py | 8 ++++-- default_config.yaml | 8 ++++-- devshell/docker-compose.yaml | 4 --- models/base.py | 37 +++++++++++++++++++-------- tests/conftest.py | 43 +++++++++++++++++++++----------- tests/docker-compose.yaml | 4 +-- tests/models/test_base.py | 4 +-- tests/models/test_image.py | 4 +-- tests/seechange_config_test.yaml | 10 +++++--- tests/util/test_config.py | 31 ++++++++++------------- util/config.py | 32 ++++++++++++++++++------ 14 files changed, 130 insertions(+), 69 deletions(-) diff --git a/.github/workflows/run-tests.yml b/.github/workflows/run-tests.yml index 8c2d75c7..34a3ad75 100644 --- a/.github/workflows/run-tests.yml +++ b/.github/workflows/run-tests.yml @@ -16,6 +16,10 @@ jobs: COMPOSE_FILE: tests/docker-compose.yaml steps: + - name: Dump docker logs on failure + if: failure() + uses: jwalton/gh-docker-logs@v2 + - name: checkout code uses: actions/checkout@v3 with: diff --git a/.gitignore b/.gitignore index 2c4c5de5..6f67e541 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,10 @@ # some specific files we don't want in the repo +local_config.yaml +local_overrides.yaml +local_augments.yaml +tests/local_config.yaml +tests/local_overrides.yaml +tests/local_augments.yaml data/DECam_examples/c4d_221104_074232_ori.fits.fz # Byte-compiled / optimized / DLL files diff --git a/README.md b/README.md index 8856e8cf..bed602e7 100644 --- a/README.md +++ b/README.md @@ -130,12 +130,12 @@ Database migrations are handled with alembic. If you've just created a database and want to initialize it with all the tables, run ``` - SEECHANGE_CONFIG= alembic upgrade head + alembic upgrade head ``` After editing any schema, you have to create new database migrations to apply them. Do this by running something like: ``` - SEECHANGE_CONFIG= alembic revision --autogenerate -m "" + alembic revision --autogenerate -m "" ``` The comment will go in the filename, so it should really be short. Look out for any warnings, and review the created migration file before applying it (with `alembic upgrade head`). diff --git a/alembic/env.py b/alembic/env.py index acbd72f6..c71cf673 100644 --- a/alembic/env.py +++ b/alembic/env.py @@ -1,3 +1,5 @@ +import os +import pathlib from logging.config import fileConfig from sqlalchemy import engine_from_config @@ -16,9 +18,11 @@ if config.config_file_name is not None: fileConfig(config.config_file_name) -from models import * +default_config_file = str((pathlib.Path(__file__).parent.parent / 'default_config.yaml').resolve()) import util.config -util.config.Config.init() +util.config.Config.init(os.getenv('SEECHANGE_CONFIG', default_config_file)) + +from models import * # add your model's MetaData object here # for 'autogenerate' support diff --git a/default_config.yaml b/default_config.yaml index 35d2f786..d9a76242 100644 --- a/default_config.yaml +++ b/default_config.yaml @@ -1,8 +1,12 @@ +overrides: + - local_overrides.yaml +augments: + - local_augments.yaml + path: data_root: null data_temp: null - server_data: null - # TODO: need to add additional options for server communications + db: engine: postgresql user: postgres diff --git a/devshell/docker-compose.yaml b/devshell/docker-compose.yaml index 47bc49dd..8d4cc3ef 100644 --- a/devshell/docker-compose.yaml +++ b/devshell/docker-compose.yaml @@ -16,8 +16,6 @@ services: seechange_dbmigrate: image: gchr.io/${GITHUB_REPOSITORY_OWNER}/seechange build: ../docker/application - environment: - SEECHANGE_CONFIG: /seechange/devshell/seechange_devshell.yaml depends_on: seechange_postgres: condition: service_healthy @@ -32,8 +30,6 @@ services: seechange: image: gchr.io/${GITHUB_REPOSITORY_OWNER}/seechange build: ../docker/application - environment: - SEECHANGE_CONFIG: /seechange/devshell/seechange_devshell.yaml depends_on: seechange_postgres: condition: service_healthy diff --git a/models/base.py b/models/base.py index a2b8be34..5c5d5095 100644 --- a/models/base.py +++ b/models/base.py @@ -237,8 +237,21 @@ def recursive_merge(self, session, done_list=None): Base = declarative_base(cls=SeeChangeBase) +ARCHIVE = None -class FileOnDiskMixin: + +def get_archive_object(): + """Return a global archive object. If it doesn't exist, create it based on the current config. """ + global ARCHIVE + if ARCHIVE is None: + cfg = config.Config.get() + archive_specs = cfg.value('archive', None) + if archive_specs is not None: + ARCHIVE = Archive(**archive_specs) + return ARCHIVE + + +class FileOnDiskMixin(): """Mixin for objects that refer to files on disk. Files are assumed to live on the local disk (underneath the @@ -315,15 +328,6 @@ class variables "local_path" and "archive" that are initialized from if not os.path.isdir(local_path): os.makedirs(local_path, exist_ok=True) - archive = cfg.value('archive', None) - if archive is not None: - archive = Archive( archive_url=cfg.value('archive.url'), - path_base=cfg.value('archive.path_base'), - token=cfg.value('archive.token'), - verify_cert=cfg.value('archive.verify_cert'), - local_read_dir=cfg.value('archive.read_dir'), - local_write_dir=cfg.value('archive.write_dir') ) - @classmethod def safe_mkdir(cls, path): if path is None or path == '': @@ -426,9 +430,22 @@ def __init__(self, *args, **kwargs): self.filepath = kwargs.pop('filepath', self.filepath) self.nofile = kwargs.pop('nofile', self._do_not_require_file_to_exist()) + self._archive = None + @orm.reconstructor def init_on_load(self): self.nofile = self._do_not_require_file_to_exist() + self._archive = None + + @property + def archive(self): + if getattr(self, '_archive', None) is None: + self._archive = get_archive_object() + return self._archive + + @archive.setter + def archive(self, value): + self._archive = value @staticmethod def _do_not_require_file_to_exist(): diff --git a/tests/conftest.py b/tests/conftest.py index 78d446f9..340096ae 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3,6 +3,7 @@ import uuid import wget import shutil +import pathlib import numpy as np @@ -10,6 +11,7 @@ from astropy.time import Time +from util.config import Config from models.base import SmartSession, CODE_ROOT from models.provenance import CodeVersion, Provenance from models.exposure import Exposure @@ -18,10 +20,33 @@ from util import config from util.archive import Archive + +# idea taken from: https://shay-palachy.medium.com/temp-environment-variables-for-pytest-7253230bd777 +# this fixture should be the first thing loaded by the test suite +@pytest.fixture(scope="session", autouse=True) +def tests_setup_and_teardown(): + # Will be executed before the first test + # print('Initial setup fixture loaded! ') + + # make sure to load the test config + test_config_file = str((pathlib.Path(__file__).parent.parent / 'tests' / 'seechange_config_test.yaml').resolve()) + + Config.get(configfile=test_config_file, setdefault=True) + + yield + # Will be executed after the last test + # print('Final teardown fixture executed! ') + + def rnd_str(n): return ''.join(np.random.choice(list('abcdefghijklmnopqrstuvwxyz'), n)) +@pytest.fixture +def config_test(): + return Config.get() + + @pytest.fixture(scope="session", autouse=True) def code_version(): with SmartSession() as session: @@ -271,29 +296,19 @@ def reference_entry(exposure_factory, provenance_base, provenance_extra): @pytest.fixture def archive(): cfg = config.Config.get() - if cfg.value('archive') is None: + archive_specs = cfg.value('archive') + if archive_specs is None: raise ValueError( "archive in config is None" ) - archive = Archive( archive_url=cfg.value('archive.url'), - verify_cert=cfg.value('archive.verify_cert'), - path_base=cfg.value('archive.path_base'), - local_read_dir=cfg.value('archive.read_dir'), - local_write_dir=cfg.value('archive.write_dir'), - token=cfg.value('archive.token') - ) + archive = Archive( **archive_specs ) yield archive # To tear down, we need to blow away the archive server's directory. # For the test suite, we've also mounted that directory locally, so # we can do that - archivebase = f"{os.getenv('ARCHIVE_DIR')}/{cfg.value('archive.path_base')}" + archivebase = f"{os.getenv('SEECHANGE_TEST_ARCHIVE_DIR')}/{cfg.value('archive.path_base')}" try: shutil.rmtree( archivebase ) except FileNotFoundError: pass -@pytest.fixture -def config_test(): - # Make sure the environment is set as expected for tests - assert os.getenv( "SEECHANGE_CONFIG" ) == "/seechange/tests/seechange_config_test.yaml" - return config.Config.get( os.getenv("SEECHANGE_CONFIG") ) diff --git a/tests/docker-compose.yaml b/tests/docker-compose.yaml index 8181844d..48f1b363 100644 --- a/tests/docker-compose.yaml +++ b/tests/docker-compose.yaml @@ -78,7 +78,7 @@ services: context: ../docker/application environment: SEECHANGE_CONFIG: /seechange/tests/seechange_config_test.yaml - ARCHIVE_DIR: /archive_storage/base + SEECHANGE_TEST_ARCHIVE_DIR: /archive_storage/base depends_on: setuptables: condition: service_completed_successfully @@ -101,7 +101,7 @@ services: context: ../docker/application environment: SEECHANGE_CONFIG: /seechange/tests/seechange_config_test.yaml - ARCHIVE_DIR: /archive_storage/base + SEECHANGE_TEST_ARCHIVE_DIR: /archive_storage/base depends_on: setuptables: condition: service_completed_successfully diff --git a/tests/models/test_base.py b/tests/models/test_base.py index 11f97389..4c7b8280 100644 --- a/tests/models/test_base.py +++ b/tests/models/test_base.py @@ -130,7 +130,7 @@ def test_fileondisk_save_failuremodes( diskfile ): def test_fileondisk_save_singlefile( diskfile, archive ): cfg = config.Config.get() - archivebase = f"{os.getenv('ARCHIVE_DIR')}/{cfg.value('archive.path_base')}" + archivebase = f"{os.getenv('SEECHANGE_TEST_ARCHIVE_DIR')}/{cfg.value('archive.path_base')}" diskfile.filepath = 'test_fileondisk_save.dat' data1 = numpy.random.rand( 32 ).tobytes() @@ -256,7 +256,7 @@ def test_fileondisk_save_singlefile( diskfile, archive ): def test_fileondisk_save_multifile( diskfile, archive ): try: cfg = config.Config.get() - archivebase = f"{os.getenv('ARCHIVE_DIR')}/{cfg.value('archive.path_base')}" + archivebase = f"{os.getenv('SEECHANGE_TEST_ARCHIVE_DIR')}/{cfg.value('archive.path_base')}" diskfile.filepath = 'test_fileondisk_save' data1 = numpy.random.rand( 32 ).tobytes() diff --git a/tests/models/test_image.py b/tests/models/test_image.py index c6f798b7..a205ea65 100644 --- a/tests/models/test_image.py +++ b/tests/models/test_image.py @@ -92,7 +92,7 @@ def test_image_archive_singlefile(demo_image, provenance_base, archive): demo_image.flags = np.random.randint(0, 100, size=demo_image.raw_data.shape, dtype=np.uint16) cfg = config.Config.get() - archivebase = f"{os.getenv('ARCHIVE_DIR')}/{cfg.value('archive.path_base')}" + archivebase = f"{os.getenv('SEECHANGE_TEST_ARCHIVE_DIR')}/{cfg.value('archive.path_base')}" single_fileness = cfg.value( 'storage.images.single_file' ) try: @@ -159,7 +159,7 @@ def test_image_archive_multifile(demo_image, provenance_base, archive): demo_image.flags = np.random.randint(0, 100, size=demo_image.raw_data.shape, dtype=np.uint16) cfg = config.Config.get() - archivebase = f"{os.getenv('ARCHIVE_DIR')}/{cfg.value('archive.path_base')}" + archivebase = f"{os.getenv('SEECHANGE_TEST_ARCHIVE_DIR')}/{cfg.value('archive.path_base')}" single_fileness = cfg.value( 'storage.images.single_file' ) try: diff --git a/tests/seechange_config_test.yaml b/tests/seechange_config_test.yaml index c22b1607..e225c13e 100644 --- a/tests/seechange_config_test.yaml +++ b/tests/seechange_config_test.yaml @@ -1,15 +1,19 @@ preloads: - ../default_config.yaml +overrides: + - local_overrides.yaml +augments: + - local_augments.yaml db: host: seechange_postgres archive: - url: http://archive:8080/ + archive_url: http://archive:8080/ verify_cert: false path_base: test/ - read_dir: null - write_dir: null + local_read_dir: null + local_write_dir: null token: insecure subtraction: diff --git a/tests/util/test_config.py b/tests/util/test_config.py index 4faef273..f032dfe5 100644 --- a/tests/util/test_config.py +++ b/tests/util/test_config.py @@ -15,25 +15,20 @@ class TestConfig: - # Nominally, this tests should pass even if SEECHANGE_CONFIG is not None. However, because there - # are autouse fixtures in ../conftest.py, one of the side effects is that the config default - # gets set. - @pytest.mark.skipif( os.getenv("SEECHANGE_CONFIG") is not None, reason="Clear SEECHANGE_CONFIG to test this" ) - def test_no_default( self ): - assert config.Config._default == None - - @pytest.mark.skipif( os.getenv("SEECHANGE_CONFIG") is None, reason="Set SEECHANGE_CONFIG to test this" ) - def test_default_default( self ): - cfg = config.Config.get() - assert cfg._path == pathlib.Path( os.getenv("SEECHANGE_CONFIG") ) - - def test_set_default( self ): - cfg = config.Config.get( _rundir / 'test.yaml', setdefault=True ) - assert config.Config._default == f'{ (_rundir / "test.yaml").resolve() }' - @pytest.fixture(scope='class') - def cfg( self ): - return config.Config.get( _rundir / 'test.yaml', setdefault=True ) + def cfg(self): + # print('setting up a config object with a spoof yaml file just for testing the config mechanism. ') + return config.Config.get(_rundir / 'test.yaml', setdefault=False) # retain the default for other tests + + def test_default_default( self ): + # make sure that when we load a config without parameters, + # it uses the default config file + default_config_path = (_rundir.parent.parent / 'default_config.yaml').resolve() + default_config_path = os.getenv('SEECHANGE_CONFIG', default_config_path) + assert config.Config._default_default == str(default_config_path) + + def test_config_path( self, cfg ): + assert cfg._path == (_rundir / "test.yaml").resolve() def test_preload(self, cfg): assert cfg.value('preload1dict1.preload1_1val1') == '2_1val1' diff --git a/util/config.py b/util/config.py index 6c8bca96..49a7df51 100644 --- a/util/config.py +++ b/util/config.py @@ -85,14 +85,15 @@ class Config: 4. Change a config value with - confobj.set_or_append_value( fieldspec, value ) + confobj.set_value( fieldspec, value ) This only changes it for the running session, it does *not* affect the YAML files in storage. """ - _default_default = os.getenv("SEECHANGE_CONFIG", None) + top_level_config = str((pathlib.Path(__file__).parent.parent / "default_config.yaml").resolve()) + _default_default = os.getenv('SEECHANGE_CONFIG', top_level_config) _default = None _configs = {} @@ -104,7 +105,9 @@ def init( configfile=None, logger=logging.getLogger("main"), dirmap={} ): Parameters ---------- configfile : str or pathlib.Path, default None - If None, will set the config file to os.getenv("SEECHANGE_CONFIG") + If None, will set the config file to the environmental + variable SEECHANGE_CONFIG, and if that is not defined, + will set to default_config.yaml in the top level of the project. logger: logging object, default: getLogger("main") @@ -112,7 +115,6 @@ def init( configfile=None, logger=logging.getLogger("main"), dirmap={} ): An ugly hack for changing the directories of imported files; see the static function dirmap. """ - Config.get( configfile, logger=logger, dirmap=dirmap ) @staticmethod @@ -316,8 +318,12 @@ def _augment( self, augmentfile, dirmap={} ): augmentpath = pathlib.Path( augmentfile ) if not augmentpath.is_absolute(): augmentpath = ( self._path.parent / augmentfile ).resolve() - augment = Config( augmentpath, logger=self.logger, dirmap=dirmap )._data - self._data = Config._merge_trees( self._data, augment, augment=True ) + if augmentpath.is_file(): + self.logger.info( f'Reading file {augmentfile} as an augment. ' ) + augment = Config( augmentpath, logger=self.logger, dirmap=dirmap )._data + self._data = Config._merge_trees( self._data, augment, augment=True ) + elif augmentfile is not None: + self.logger.info( f'Augment file {augmentfile} not found. ' ) def _override( self, overridefile, dirmap=dirmap ): """Read file (or path) overridefile and override config data. Intended for internal use only. @@ -340,10 +346,15 @@ def _override( self, overridefile, dirmap=dirmap ): recurse into the conflict rules. """ overridepath = pathlib.Path( overridefile ) + if not overridepath.is_absolute(): overridepath = ( self._path.parent / overridefile ).resolve() - override = Config( overridepath, logger=self.logger, dirmap=dirmap )._data - self._data = Config._merge_trees( self._data, override ) + if overridepath.is_file(): + self.logger.info(f'Reading file {overridepath} as an override. ') + override = Config( overridepath, logger=self.logger, dirmap=dirmap )._data + self._data = Config._merge_trees( self._data, override ) + elif overridefile is not None: + self.logger.info( f'Override file {overridefile} not found. ' ) def value( self, field, default=NoValue(), struct=None ): """Get a value from the config structure. @@ -586,3 +597,8 @@ def _merge_trees( left, right, augment=False ): return newdict else: return copy.deepcopy( right ) + + +if __name__ == "__main__": + import os + conf = Config.get()