From 898c5c942d5b7fb8473bd854bc6ad827f928e1d8 Mon Sep 17 00:00:00 2001 From: "Yakubov, Sergey" Date: Tue, 14 Nov 2023 11:57:40 -0500 Subject: [PATCH 1/8] optimize object store pull added sync_cache parameter to the get_filename function that allows skip pulling to cache when not needed --- .../sample/object_store_conf.xml.sample | 16 +++++----- lib/galaxy/datatypes/protocols.py | 2 +- lib/galaxy/job_execution/setup.py | 5 +++- lib/galaxy/managers/datasets.py | 2 +- lib/galaxy/model/__init__.py | 16 +++++----- lib/galaxy/model/none_like.py | 2 +- lib/galaxy/objectstore/azure_blob.py | 11 +++++-- lib/galaxy/objectstore/caching.py | 7 ++++- lib/galaxy/objectstore/cloud.py | 9 ++++-- lib/galaxy/objectstore/irods.py | 14 +++++++-- lib/galaxy/objectstore/s3.py | 14 ++++++--- lib/galaxy/tool_util/data/__init__.py | 2 +- test/integration/objectstore/_base.py | 3 +- ...est_remote_objectstore_cache_operations.py | 29 +++++++++++++++++++ .../dynamic_tool_destination/mockGalaxy.py | 2 +- test/unit/app/tools/test_wrappers.py | 2 +- .../data/datatypes/test_connectivity_table.py | 2 +- test/unit/data/datatypes/test_validation.py | 2 +- test/unit/data/datatypes/util.py | 6 ++-- .../tool_util/data/test_tool_data_bundles.py | 2 +- 20 files changed, 105 insertions(+), 43 deletions(-) diff --git a/lib/galaxy/config/sample/object_store_conf.xml.sample b/lib/galaxy/config/sample/object_store_conf.xml.sample index b99028bc4f7..e6eeeaeabcb 100644 --- a/lib/galaxy/config/sample/object_store_conf.xml.sample +++ b/lib/galaxy/config/sample/object_store_conf.xml.sample @@ -135,7 +135,7 @@ - + @@ -150,7 +150,7 @@ - + @@ -166,7 +166,7 @@ - + @@ -181,7 +181,7 @@ - + @@ -196,7 +196,7 @@ - + @@ -211,7 +211,7 @@ - + @@ -226,7 +226,7 @@ - + @@ -319,7 +319,7 @@ - + This data is backed up using iRODs native hierarchal storage management mechanisms. The rules describing how data is stored and backed up in iRODS can be found in our institutional [iRODS documentation](https://irods.org/uploads/2018/Saum-SURFsara-Data_Archiving_in_iRODS-slides.pdf) diff --git a/lib/galaxy/datatypes/protocols.py b/lib/galaxy/datatypes/protocols.py index 93b2875a979..150866a8ada 100644 --- a/lib/galaxy/datatypes/protocols.py +++ b/lib/galaxy/datatypes/protocols.py @@ -30,7 +30,7 @@ class HasExtraFilesPath(Protocol): class HasFileName(Protocol): - def get_file_name(self) -> str: + def get_file_name(self, sync_cache=True) -> str: ... diff --git a/lib/galaxy/job_execution/setup.py b/lib/galaxy/job_execution/setup.py index de321944418..1cff9c37c53 100644 --- a/lib/galaxy/job_execution/setup.py +++ b/lib/galaxy/job_execution/setup.py @@ -276,7 +276,10 @@ class JobIO(Dictifiable): da_false_path = dataset_path_rewriter.rewrite_dataset_path(da.dataset, "output") mutable = da.dataset.dataset.external_filename is None dataset_path = DatasetPath( - da.dataset.dataset.id, da.dataset.get_file_name(), false_path=da_false_path, mutable=mutable + da.dataset.dataset.id, + da.dataset.get_file_name(sync_cache=False), + false_path=da_false_path, + mutable=mutable, ) job_outputs.append(JobOutput(da.name, da.dataset, dataset_path)) diff --git a/lib/galaxy/managers/datasets.py b/lib/galaxy/managers/datasets.py index 6734b0d2fd1..c639316e9f5 100644 --- a/lib/galaxy/managers/datasets.py +++ b/lib/galaxy/managers/datasets.py @@ -273,7 +273,7 @@ class DatasetSerializer(base.ModelSerializer[DatasetManager], deletable.Purgable # expensive: allow config option due to cost of operation if is_admin or self.app.config.expose_dataset_path: if not dataset.purged: - return dataset.get_file_name() + return dataset.get_file_name(sync_cache=False) self.skip() def serialize_extra_files_path(self, item, key, user=None, **context): diff --git a/lib/galaxy/model/__init__.py b/lib/galaxy/model/__init__.py index a0199e157af..a3595acc569 100644 --- a/lib/galaxy/model/__init__.py +++ b/lib/galaxy/model/__init__.py @@ -2516,8 +2516,8 @@ class FakeDatasetAssociation: self.dataset = dataset self.metadata = dict() - def get_file_name(self): - return self.dataset.get_file_name() + def get_file_name(self, sync_cache=True): + return self.dataset.get_file_name(sync_cache) def __eq__(self, other): return isinstance(other, FakeDatasetAssociation) and self.dataset == other.dataset @@ -3926,14 +3926,14 @@ class Dataset(Base, StorableObject, Serializable): if not self.shareable: raise Exception(CANNOT_SHARE_PRIVATE_DATASET_MESSAGE) - def get_file_name(self): + def get_file_name(self, sync_cache=True): if self.purged: log.warning(f"Attempt to get file name of purged dataset {self.id}") return "" if not self.external_filename: object_store = self._assert_object_store_set() if object_store.exists(self): - file_name = object_store.get_filename(self) + file_name = object_store.get_filename(self, sync_cache=sync_cache) else: file_name = "" if not file_name and self.state not in (self.states.NEW, self.states.QUEUED): @@ -4387,10 +4387,10 @@ class DatasetInstance(RepresentById, UsesCreateAndUpdateTime, _HasTable): state = property(get_dataset_state, set_dataset_state) - def get_file_name(self) -> str: + def get_file_name(self, sync_cache=True) -> str: if self.dataset.purged: return "" - return self.dataset.get_file_name() + return self.dataset.get_file_name(sync_cache=sync_cache) def set_file_name(self, filename: str): return self.dataset.set_file_name(filename) @@ -9168,7 +9168,7 @@ class MetadataFile(Base, StorableObject, Serializable): alt_name=os.path.basename(self.get_file_name()), ) - def get_file_name(self): + def get_file_name(self, sync_cache=True): # Ensure the directory structure and the metadata file object exist try: da = self.history_dataset or self.library_dataset @@ -9183,7 +9183,7 @@ class MetadataFile(Base, StorableObject, Serializable): if not object_store.exists(self, extra_dir="_metadata_files", extra_dir_at_root=True, alt_name=alt_name): object_store.create(self, extra_dir="_metadata_files", extra_dir_at_root=True, alt_name=alt_name) path = object_store.get_filename( - self, extra_dir="_metadata_files", extra_dir_at_root=True, alt_name=alt_name + self, extra_dir="_metadata_files", extra_dir_at_root=True, alt_name=alt_name, sync_cache=sync_cache ) return path except AttributeError: diff --git a/lib/galaxy/model/none_like.py b/lib/galaxy/model/none_like.py index d24a70329cd..7fa7175d7f3 100644 --- a/lib/galaxy/model/none_like.py +++ b/lib/galaxy/model/none_like.py @@ -38,5 +38,5 @@ class NoneDataset(RecursiveNone): def missing_meta(self): return False - def get_file_name(self): + def get_file_name(self, _sync_cache=True): return "None" diff --git a/lib/galaxy/objectstore/azure_blob.py b/lib/galaxy/objectstore/azure_blob.py index b72e6b9dbfd..83c1d700e19 100644 --- a/lib/galaxy/objectstore/azure_blob.py +++ b/lib/galaxy/objectstore/azure_blob.py @@ -109,6 +109,7 @@ class AzureBlobObjectStore(ConcreteObjectStore): self.cache_size = cache_dict.get("size") or self.config.object_store_cache_size self.staging_path = cache_dict.get("path") or self.config.object_store_cache_path + self.cache_updated_data = cache_dict.get("cache_updated_data", True) self._initialize() @@ -136,6 +137,7 @@ class AzureBlobObjectStore(ConcreteObjectStore): "cache": { "size": self.cache_size, "path": self.staging_path, + "cache_updated_data": self.cache_updated_data, }, } ) @@ -501,12 +503,15 @@ class AzureBlobObjectStore(ConcreteObjectStore): base_dir = kwargs.get("base_dir", None) dir_only = kwargs.get("dir_only", False) obj_dir = kwargs.get("obj_dir", False) + sync_cache = kwargs.get("sync_cache", True) # for JOB_WORK directory if base_dir and dir_only and obj_dir: return os.path.abspath(rel_path) cache_path = self._get_cache_path(rel_path) + if not sync_cache: + return cache_path # S3 does not recognize directories as files so cannot check if those exist. # So, if checking dir only, ensure given dir exists in cache and return # the expected cache path. @@ -515,8 +520,8 @@ class AzureBlobObjectStore(ConcreteObjectStore): # if not os.path.exists(cache_path): # os.makedirs(cache_path) # return cache_path - # Check if the file exists in the cache first - if self._in_cache(rel_path): + # Check if the file exists in the cache first, always pull if file size in cache is zero + if self._in_cache(rel_path) and (dir_only or os.path.getsize(self._get_cache_path(rel_path)) > 0): return cache_path # Check if the file exists in persistent storage and, if it does, pull it into cache elif self._exists(obj, **kwargs): @@ -542,7 +547,7 @@ class AzureBlobObjectStore(ConcreteObjectStore): # Copy into cache cache_file = self._get_cache_path(rel_path) try: - if source_file != cache_file: + if source_file != cache_file and self.cache_updated_data: # FIXME? Should this be a `move`? shutil.copy2(source_file, cache_file) self._fix_permissions(cache_file) diff --git a/lib/galaxy/objectstore/caching.py b/lib/galaxy/objectstore/caching.py index 2097da2c680..2df46868149 100644 --- a/lib/galaxy/objectstore/caching.py +++ b/lib/galaxy/objectstore/caching.py @@ -12,7 +12,10 @@ from typing import ( from typing_extensions import NamedTuple -from galaxy.util import nice_size +from galaxy.util import ( + nice_size, + string_as_bool, +) from galaxy.util.sleeper import Sleeper log = logging.getLogger(__name__) @@ -124,11 +127,13 @@ def parse_caching_config_dict_from_xml(config_xml): cache_size = float(c_xml.get("size", -1)) staging_path = c_xml.get("path", None) monitor = c_xml.get("monitor", "auto") + cache_updated_data = string_as_bool(c_xml.get("cache_updated_data", "True")) cache_dict = { "size": cache_size, "path": staging_path, "monitor": monitor, + "cache_updated_data": cache_updated_data, } else: cache_dict = {} diff --git a/lib/galaxy/objectstore/cloud.py b/lib/galaxy/objectstore/cloud.py index 4a995d833f6..888df3549f4 100644 --- a/lib/galaxy/objectstore/cloud.py +++ b/lib/galaxy/objectstore/cloud.py @@ -66,6 +66,7 @@ class CloudConfigMixin: "cache": { "size": self.cache_size, "path": self.staging_path, + "cache_updated_data": self.cache_updated_data, }, } @@ -103,6 +104,7 @@ class Cloud(ConcreteObjectStore, CloudConfigMixin): self.cache_size = cache_dict.get("size") or self.config.object_store_cache_size self.staging_path = cache_dict.get("path") or self.config.object_store_cache_path + self.cache_updated_data = cache_dict.get("cache_updated_data", True) self._initialize() @@ -621,12 +623,15 @@ class Cloud(ConcreteObjectStore, CloudConfigMixin): dir_only = kwargs.get("dir_only", False) obj_dir = kwargs.get("obj_dir", False) rel_path = self._construct_path(obj, **kwargs) + sync_cache = kwargs.get("sync_cache", True) # for JOB_WORK directory if base_dir and dir_only and obj_dir: return os.path.abspath(rel_path) cache_path = self._get_cache_path(rel_path) + if not sync_cache: + return cache_path # S3 does not recognize directories as files so cannot check if those exist. # So, if checking dir only, ensure given dir exists in cache and return # the expected cache path. @@ -636,7 +641,7 @@ class Cloud(ConcreteObjectStore, CloudConfigMixin): # os.makedirs(cache_path) # return cache_path # Check if the file exists in the cache first - if self._in_cache(rel_path): + if self._in_cache(rel_path) and (dir_only or os.path.getsize(self._get_cache_path(rel_path)) > 0): return cache_path # Check if the file exists in persistent storage and, if it does, pull it into cache elif self._exists(obj, **kwargs): @@ -663,7 +668,7 @@ class Cloud(ConcreteObjectStore, CloudConfigMixin): # Copy into cache cache_file = self._get_cache_path(rel_path) try: - if source_file != cache_file: + if source_file != cache_file and self.cache_updated_data: # FIXME? Should this be a `move`? shutil.copy2(source_file, cache_file) self._fix_permissions(cache_file) diff --git a/lib/galaxy/objectstore/irods.py b/lib/galaxy/objectstore/irods.py index 63b81e7a1a9..aaf672ca0c4 100644 --- a/lib/galaxy/objectstore/irods.py +++ b/lib/galaxy/objectstore/irods.py @@ -26,6 +26,7 @@ from galaxy.exceptions import ( from galaxy.util import ( directory_hash_id, ExecutionTimer, + string_as_bool, umask_fix_perms, unlink, ) @@ -81,6 +82,7 @@ def parse_config_xml(config_xml): _config_xml_error("cache") cache_size = float(c_xml[0].get("size", -1)) staging_path = c_xml[0].get("path", None) + cache_updated_data = string_as_bool(c_xml.get("cache_updated_data", "True")) attrs = ("type", "path") e_xml = config_xml.findall("extra_dir") @@ -109,6 +111,7 @@ def parse_config_xml(config_xml): "cache": { "size": cache_size, "path": staging_path, + "cache_updated_data": cache_updated_data, }, "extra_dirs": extra_dirs, "private": DiskObjectStore.parse_private_from_config_xml(config_xml), @@ -142,6 +145,7 @@ class CloudConfigMixin: "cache": { "size": self.cache_size, "path": self.staging_path, + "cache_updated_data": self.cache_updated_data, }, } @@ -209,6 +213,7 @@ class IRODSObjectStore(DiskObjectStore, CloudConfigMixin): self.staging_path = cache_dict.get("path") or self.config.object_store_cache_path if self.staging_path is None: _config_dict_error("cache->path") + self.cache_updated_data = cache_dict.get("cache_updated_data", True) extra_dirs = {e["type"]: e["path"] for e in config_dict.get("extra_dirs", [])} if not extra_dirs: @@ -716,6 +721,7 @@ class IRODSObjectStore(DiskObjectStore, CloudConfigMixin): dir_only = kwargs.get("dir_only", False) obj_dir = kwargs.get("obj_dir", False) rel_path = self._construct_path(obj, **kwargs) + sync_cache = kwargs.get("sync_cache", True) # for JOB_WORK directory if base_dir and dir_only and obj_dir: @@ -723,6 +729,8 @@ class IRODSObjectStore(DiskObjectStore, CloudConfigMixin): return os.path.abspath(rel_path) cache_path = self._get_cache_path(rel_path) + if not sync_cache: + return cache_path # iRODS does not recognize directories as files so cannot check if those exist. # So, if checking dir only, ensure given dir exists in cache and return # the expected cache path. @@ -731,8 +739,8 @@ class IRODSObjectStore(DiskObjectStore, CloudConfigMixin): # if not os.path.exists(cache_path): # os.makedirs(cache_path) # return cache_path - # Check if the file exists in the cache first - if self._in_cache(rel_path): + # Check if the file exists in the cache first, always pull if file size in cache is zero + if self._in_cache(rel_path) and (dir_only or os.path.getsize(self._get_cache_path(rel_path)) > 0): log.debug("irods_pt _get_filename: %s", ipt_timer) return cache_path # Check if the file exists in persistent storage and, if it does, pull it into cache @@ -764,7 +772,7 @@ class IRODSObjectStore(DiskObjectStore, CloudConfigMixin): # Copy into cache cache_file = self._get_cache_path(rel_path) try: - if source_file != cache_file: + if source_file != cache_file and self.cache_updated_data: # FIXME? Should this be a `move`? shutil.copy2(source_file, cache_file) self._fix_permissions(cache_file) diff --git a/lib/galaxy/objectstore/s3.py b/lib/galaxy/objectstore/s3.py index e0656864a93..41b4d58e96a 100644 --- a/lib/galaxy/objectstore/s3.py +++ b/lib/galaxy/objectstore/s3.py @@ -145,6 +145,7 @@ class CloudConfigMixin: "cache": { "size": self.cache_size, "path": self.staging_path, + "cache_updated_data": self.cache_updated_data, }, } @@ -186,6 +187,7 @@ class S3ObjectStore(ConcreteObjectStore, CloudConfigMixin): self.cache_size = cache_dict.get("size") or self.config.object_store_cache_size self.staging_path = cache_dict.get("path") or self.config.object_store_cache_path + self.cache_updated_data = cache_dict.get("cache_updated_data", True) extra_dirs = {e["type"]: e["path"] for e in config_dict.get("extra_dirs", [])} self.extra_dirs.update(extra_dirs) @@ -634,7 +636,7 @@ class S3ObjectStore(ConcreteObjectStore, CloudConfigMixin): def _get_data(self, obj, start=0, count=-1, **kwargs): rel_path = self._construct_path(obj, **kwargs) # Check cache first and get file if not there - if not self._in_cache(rel_path): + if not self._in_cache(rel_path) or os.path.getsize(self._get_cache_path(rel_path)) == 0: self._pull_into_cache(rel_path) # Read the file content from cache data_file = open(self._get_cache_path(rel_path)) @@ -647,6 +649,8 @@ class S3ObjectStore(ConcreteObjectStore, CloudConfigMixin): base_dir = kwargs.get("base_dir", None) dir_only = kwargs.get("dir_only", False) obj_dir = kwargs.get("obj_dir", False) + sync_cache = kwargs.get("sync_cache", True) + rel_path = self._construct_path(obj, **kwargs) # for JOB_WORK directory @@ -654,6 +658,8 @@ class S3ObjectStore(ConcreteObjectStore, CloudConfigMixin): return os.path.abspath(rel_path) cache_path = self._get_cache_path(rel_path) + if not sync_cache: + return cache_path # S3 does not recognize directories as files so cannot check if those exist. # So, if checking dir only, ensure given dir exists in cache and return # the expected cache path. @@ -662,8 +668,8 @@ class S3ObjectStore(ConcreteObjectStore, CloudConfigMixin): # if not os.path.exists(cache_path): # os.makedirs(cache_path) # return cache_path - # Check if the file exists in the cache first - if self._in_cache(rel_path): + # Check if the file exists in the cache first, always pull if file size in cache is zero + if self._in_cache(rel_path) and (dir_only or os.path.getsize(self._get_cache_path(rel_path)) > 0): return cache_path # Check if the file exists in persistent storage and, if it does, pull it into cache elif self._exists(obj, **kwargs): @@ -691,7 +697,7 @@ class S3ObjectStore(ConcreteObjectStore, CloudConfigMixin): # Copy into cache cache_file = self._get_cache_path(rel_path) try: - if source_file != cache_file: + if source_file != cache_file and self.cache_updated_data: # FIXME? Should this be a `move`? shutil.copy2(source_file, cache_file) self._fix_permissions(cache_file) diff --git a/lib/galaxy/tool_util/data/__init__.py b/lib/galaxy/tool_util/data/__init__.py index 7846188320d..94e27437939 100644 --- a/lib/galaxy/tool_util/data/__init__.py +++ b/lib/galaxy/tool_util/data/__init__.py @@ -891,7 +891,7 @@ class DirectoryAsExtraFiles(HasExtraFiles): class OutputDataset(HasExtraFiles, Protocol): ext: str - def get_file_name(self) -> str: + def get_file_name(self, _sync_cache=True) -> str: ... diff --git a/test/integration/objectstore/_base.py b/test/integration/objectstore/_base.py index 272aa0fa134..f85b77cb0be 100644 --- a/test/integration/objectstore/_base.py +++ b/test/integration/objectstore/_base.py @@ -17,7 +17,7 @@ OBJECT_STORE_CONFIG = string.Template( - + @@ -102,6 +102,7 @@ class BaseSwiftObjectStoreIntegrationTestCase(BaseObjectStoreIntegrationTestCase "port": OBJECT_STORE_PORT, "access_key": OBJECT_STORE_ACCESS_KEY, "secret_key": OBJECT_STORE_SECRET_KEY, + "cache_updated_data": False, } ) ) diff --git a/test/integration/objectstore/test_remote_objectstore_cache_operations.py b/test/integration/objectstore/test_remote_objectstore_cache_operations.py index 5ac29c6d9ff..212ac517f88 100644 --- a/test/integration/objectstore/test_remote_objectstore_cache_operations.py +++ b/test/integration/objectstore/test_remote_objectstore_cache_operations.py @@ -3,6 +3,7 @@ import shutil import pytest +from galaxy_test.base.populators import skip_without_tool from ._base import ( BaseSwiftObjectStoreIntegrationTestCase, files_count, @@ -32,6 +33,34 @@ class TestCacheOperation(BaseSwiftObjectStoreIntegrationTestCase): self.upload_dataset() assert files_count(self.object_store_cache_path) == 1 + @skip_without_tool("create_2") + def test_cache_populated_after_tool_run(self): + history_id = self.dataset_populator.new_history() + running_response = self.dataset_populator.run_tool_raw( + "create_2", + {"sleep_time": 0}, + history_id, + ) + result = self.dataset_populator.wait_for_tool_run( + run_response=running_response, history_id=history_id, assert_ok=False + ).json() + details = self.dataset_populator.get_job_details(result["jobs"][0]["id"], full=True).json() + assert details["state"] == "ok" + # check files in the cache are empty after job finishes (they are created by Galaxy before the tool executes) + assert files_count(self.object_store_cache_path) == 2 + hda1_details = self.dataset_populator.get_history_dataset_details(history_id, assert_ok=True, hid=1) + hda2_details = self.dataset_populator.get_history_dataset_details(history_id, assert_ok=True, hid=2) + assert os.path.getsize(hda1_details["file_name"]) == 0 + assert os.path.getsize(hda2_details["file_name"]) == 0 + # get dataset content - this should trigger pulling to the cache + content1 = self.dataset_populator.get_history_dataset_content(history_id, dataset_id=hda1_details["dataset_id"]) + content2 = self.dataset_populator.get_history_dataset_content(history_id, dataset_id=hda2_details["dataset_id"]) + assert content1 == "1\n" + assert content2 == "2\n" + # check files in the cache are not empty now + assert os.path.getsize(hda1_details["file_name"]) != 0 + assert os.path.getsize(hda2_details["file_name"]) != 0 + def test_cache_repopulated(self): hda = self.upload_dataset() assert files_count(self.object_store_cache_path) == 1 diff --git a/test/unit/app/jobs/dynamic_tool_destination/mockGalaxy.py b/test/unit/app/jobs/dynamic_tool_destination/mockGalaxy.py index 4a618a34b03..919eecfc642 100644 --- a/test/unit/app/jobs/dynamic_tool_destination/mockGalaxy.py +++ b/test/unit/app/jobs/dynamic_tool_destination/mockGalaxy.py @@ -43,7 +43,7 @@ class Dataset: def get_metadata(self): return self.metadata - def get_file_name(self, **_kwargs): + def get_file_name(self, _sync_cache=True): return self.file_name_ diff --git a/test/unit/app/tools/test_wrappers.py b/test/unit/app/tools/test_wrappers.py index e9185de69ef..96c18253e87 100644 --- a/test/unit/app/tools/test_wrappers.py +++ b/test/unit/app/tools/test_wrappers.py @@ -312,7 +312,7 @@ class MockDataset: self.ext = MOCK_DATASET_EXT self.tags = [] - def get_file_name(self): + def get_file_name(self, _sync_cache=True): return MOCK_DATASET_PATH diff --git a/test/unit/data/datatypes/test_connectivity_table.py b/test/unit/data/datatypes/test_connectivity_table.py index 3a3037c27c7..521ecb59c13 100644 --- a/test/unit/data/datatypes/test_connectivity_table.py +++ b/test/unit/data/datatypes/test_connectivity_table.py @@ -9,7 +9,7 @@ from galaxy.util import galaxy_directory @pytest.fixture def dataset(): class MockDataset: - def get_file_name(self): + def get_file_name(self, _sync_cache=True): return os.path.join(galaxy_directory(), "test-data/1.ct") return MockDataset() diff --git a/test/unit/data/datatypes/test_validation.py b/test/unit/data/datatypes/test_validation.py index dc6ed5ac662..0b4164dd5e1 100644 --- a/test/unit/data/datatypes/test_validation.py +++ b/test/unit/data/datatypes/test_validation.py @@ -60,5 +60,5 @@ class MockDataset: self.file_name_ = file_name self.datatype = datatype - def get_file_name(self): + def get_file_name(self, _sync_cache=True): return self.file_name_ diff --git a/test/unit/data/datatypes/util.py b/test/unit/data/datatypes/util.py index 881f847ab7a..df12959a6b8 100644 --- a/test/unit/data/datatypes/util.py +++ b/test/unit/data/datatypes/util.py @@ -13,7 +13,7 @@ class MockDatasetDataset: self.purged = False self.file_name_ = file_name - def get_file_name(self): + def get_file_name(self, _sync_cache=True): return self.file_name_ def set_file_name(self, file_name): @@ -23,7 +23,7 @@ class MockDatasetDataset: class MockMetadata: file_name_: Optional[str] = None - def get_file_name(self): + def get_file_name(self, _sync_cache=True): return self.file_name_ def set_file_name(self, file_name): @@ -37,7 +37,7 @@ class MockDataset: self.dataset = None self.file_name_: Optional[str] = None - def get_file_name(self): + def get_file_name(self, _sync_cache=True): return self.file_name_ def set_file_name(self, file_name): diff --git a/test/unit/tool_util/data/test_tool_data_bundles.py b/test/unit/tool_util/data/test_tool_data_bundles.py index a884bf2486d..f4926156a32 100644 --- a/test/unit/tool_util/data/test_tool_data_bundles.py +++ b/test/unit/tool_util/data/test_tool_data_bundles.py @@ -76,7 +76,7 @@ class OutputDataset: extra_files_path: str ext: str = "data_manager_json" - def get_file_name(self) -> str: + def get_file_name(self, _sync_cache=True) -> str: return self.file_name_ def extra_files_path_exists(self) -> bool: From f9182d9bd38f1464cad447615ddbaee45353ea47 Mon Sep 17 00:00:00 2001 From: "Yakubov, Sergey" Date: Tue, 14 Nov 2023 13:48:45 -0500 Subject: [PATCH 2/8] fix xml parsing for irods --- lib/galaxy/objectstore/irods.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/objectstore/irods.py b/lib/galaxy/objectstore/irods.py index aaf672ca0c4..76f80aeb83b 100644 --- a/lib/galaxy/objectstore/irods.py +++ b/lib/galaxy/objectstore/irods.py @@ -82,7 +82,7 @@ def parse_config_xml(config_xml): _config_xml_error("cache") cache_size = float(c_xml[0].get("size", -1)) staging_path = c_xml[0].get("path", None) - cache_updated_data = string_as_bool(c_xml.get("cache_updated_data", "True")) + cache_updated_data = string_as_bool(c_xml[0].get("cache_updated_data", "True")) attrs = ("type", "path") e_xml = config_xml.findall("extra_dir") From 9858c5971b6889482f63506092f7236dd5209f12 Mon Sep 17 00:00:00 2001 From: "Yakubov, Sergey" Date: Thu, 16 Nov 2023 10:11:50 -0500 Subject: [PATCH 3/8] irods - download file even if already in cache --- lib/galaxy/objectstore/irods.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/objectstore/irods.py b/lib/galaxy/objectstore/irods.py index 76f80aeb83b..b4744ca09cc 100644 --- a/lib/galaxy/objectstore/irods.py +++ b/lib/galaxy/objectstore/irods.py @@ -445,7 +445,7 @@ class IRODSObjectStore(DiskObjectStore, CloudConfigMixin): collection_path = f"{self.home}/{subcollection_name}" data_object_path = f"{collection_path}/{data_object_name}" - options = {kw.DEST_RESC_NAME_KW: self.resource} + options = {kw.FORCE_FLAG_KW: "", kw.DEST_RESC_NAME_KW: self.resource} try: cache_path = self._get_cache_path(rel_path) From d930fc638def999faa1ddb47582ba2c5a16e962e Mon Sep 17 00:00:00 2001 From: Sergey Yakubov <17157816+SergeyYakubov@users.noreply.github.com> Date: Thu, 16 Nov 2023 10:25:49 -0500 Subject: [PATCH 4/8] update comment about file check for cloud object store Co-authored-by: John Davis --- lib/galaxy/objectstore/cloud.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/objectstore/cloud.py b/lib/galaxy/objectstore/cloud.py index 888df3549f4..625b1f9547e 100644 --- a/lib/galaxy/objectstore/cloud.py +++ b/lib/galaxy/objectstore/cloud.py @@ -640,7 +640,7 @@ class Cloud(ConcreteObjectStore, CloudConfigMixin): # if not os.path.exists(cache_path): # os.makedirs(cache_path) # return cache_path - # Check if the file exists in the cache first + # Check if the file exists in the cache first, always pull if file size in cache is zero if self._in_cache(rel_path) and (dir_only or os.path.getsize(self._get_cache_path(rel_path)) > 0): return cache_path # Check if the file exists in persistent storage and, if it does, pull it into cache From d0eef6ab3e6a34b559635c6745c6e138387b5e56 Mon Sep 17 00:00:00 2001 From: "Yakubov, Sergey" Date: Thu, 16 Nov 2023 13:22:09 -0500 Subject: [PATCH 5/8] use cache_updated_data=True for s3 tests --- test/integration/objectstore/_base.py | 6 +- ...est_remote_objectstore_cache_operations.py | 67 +++++++++++-------- 2 files changed, 44 insertions(+), 29 deletions(-) diff --git a/test/integration/objectstore/_base.py b/test/integration/objectstore/_base.py index f85b77cb0be..95796bd6f0b 100644 --- a/test/integration/objectstore/_base.py +++ b/test/integration/objectstore/_base.py @@ -102,7 +102,7 @@ class BaseSwiftObjectStoreIntegrationTestCase(BaseObjectStoreIntegrationTestCase "port": OBJECT_STORE_PORT, "access_key": OBJECT_STORE_ACCESS_KEY, "secret_key": OBJECT_STORE_SECRET_KEY, - "cache_updated_data": False, + "cache_updated_data": cls.updateCacheData(), } ) ) @@ -111,3 +111,7 @@ class BaseSwiftObjectStoreIntegrationTestCase(BaseObjectStoreIntegrationTestCase def setUp(self): super().setUp() self.dataset_populator = DatasetPopulator(self.galaxy_interactor) + + @classmethod + def updateCacheData(cls): + return True diff --git a/test/integration/objectstore/test_remote_objectstore_cache_operations.py b/test/integration/objectstore/test_remote_objectstore_cache_operations.py index 212ac517f88..cf3c9391a1a 100644 --- a/test/integration/objectstore/test_remote_objectstore_cache_operations.py +++ b/test/integration/objectstore/test_remote_objectstore_cache_operations.py @@ -33,34 +33,6 @@ class TestCacheOperation(BaseSwiftObjectStoreIntegrationTestCase): self.upload_dataset() assert files_count(self.object_store_cache_path) == 1 - @skip_without_tool("create_2") - def test_cache_populated_after_tool_run(self): - history_id = self.dataset_populator.new_history() - running_response = self.dataset_populator.run_tool_raw( - "create_2", - {"sleep_time": 0}, - history_id, - ) - result = self.dataset_populator.wait_for_tool_run( - run_response=running_response, history_id=history_id, assert_ok=False - ).json() - details = self.dataset_populator.get_job_details(result["jobs"][0]["id"], full=True).json() - assert details["state"] == "ok" - # check files in the cache are empty after job finishes (they are created by Galaxy before the tool executes) - assert files_count(self.object_store_cache_path) == 2 - hda1_details = self.dataset_populator.get_history_dataset_details(history_id, assert_ok=True, hid=1) - hda2_details = self.dataset_populator.get_history_dataset_details(history_id, assert_ok=True, hid=2) - assert os.path.getsize(hda1_details["file_name"]) == 0 - assert os.path.getsize(hda2_details["file_name"]) == 0 - # get dataset content - this should trigger pulling to the cache - content1 = self.dataset_populator.get_history_dataset_content(history_id, dataset_id=hda1_details["dataset_id"]) - content2 = self.dataset_populator.get_history_dataset_content(history_id, dataset_id=hda2_details["dataset_id"]) - assert content1 == "1\n" - assert content2 == "2\n" - # check files in the cache are not empty now - assert os.path.getsize(hda1_details["file_name"]) != 0 - assert os.path.getsize(hda2_details["file_name"]) != 0 - def test_cache_repopulated(self): hda = self.upload_dataset() assert files_count(self.object_store_cache_path) == 1 @@ -107,3 +79,42 @@ class TestCacheOperation(BaseSwiftObjectStoreIntegrationTestCase): assert files_count(self.object_store_cache_path) == 1 only_file = next(iter(get_files(self.object_store_cache_path))) assert os.path.getsize(only_file) == 4 + + +class TestCacheOperationWithNoCacheUpdate(BaseSwiftObjectStoreIntegrationTestCase): + @classmethod + def updateCacheData(cls): + return False + + def tearDown(self): + shutil.rmtree(self.object_store_cache_path) + os.mkdir(self.object_store_cache_path) + return super().tearDown() + + @skip_without_tool("create_2") + def test_cache_populated_after_tool_run(self): + history_id = self.dataset_populator.new_history() + running_response = self.dataset_populator.run_tool_raw( + "create_2", + {"sleep_time": 0}, + history_id, + ) + result = self.dataset_populator.wait_for_tool_run( + run_response=running_response, history_id=history_id, assert_ok=False + ).json() + details = self.dataset_populator.get_job_details(result["jobs"][0]["id"], full=True).json() + assert details["state"] == "ok" + # check files in the cache are empty after job finishes (they are created by Galaxy before the tool executes) + assert files_count(self.object_store_cache_path) == 2 + hda1_details = self.dataset_populator.get_history_dataset_details(history_id, assert_ok=True, hid=1) + hda2_details = self.dataset_populator.get_history_dataset_details(history_id, assert_ok=True, hid=2) + assert os.path.getsize(hda1_details["file_name"]) == 0 + assert os.path.getsize(hda2_details["file_name"]) == 0 + # get dataset content - this should trigger pulling to the cache + content1 = self.dataset_populator.get_history_dataset_content(history_id, dataset_id=hda1_details["dataset_id"]) + content2 = self.dataset_populator.get_history_dataset_content(history_id, dataset_id=hda2_details["dataset_id"]) + assert content1 == "1\n" + assert content2 == "2\n" + # check files in the cache are not empty now + assert os.path.getsize(hda1_details["file_name"]) != 0 + assert os.path.getsize(hda2_details["file_name"]) != 0 From b342f4ccb65bedef4ae81a2f9f3aa6baf7b2e07a Mon Sep 17 00:00:00 2001 From: "Yakubov, Sergey" Date: Fri, 17 Nov 2023 12:08:28 -0500 Subject: [PATCH 6/8] rename _sync_cache -> sync_cache --- lib/galaxy/model/none_like.py | 2 +- lib/galaxy/tool_util/data/__init__.py | 2 +- test/unit/app/jobs/dynamic_tool_destination/mockGalaxy.py | 2 +- test/unit/app/tools/test_wrappers.py | 2 +- test/unit/data/datatypes/test_connectivity_table.py | 2 +- test/unit/data/datatypes/test_validation.py | 2 +- test/unit/data/datatypes/util.py | 6 +++--- test/unit/tool_util/data/test_tool_data_bundles.py | 2 +- 8 files changed, 10 insertions(+), 10 deletions(-) diff --git a/lib/galaxy/model/none_like.py b/lib/galaxy/model/none_like.py index 7fa7175d7f3..80e96c16b7e 100644 --- a/lib/galaxy/model/none_like.py +++ b/lib/galaxy/model/none_like.py @@ -38,5 +38,5 @@ class NoneDataset(RecursiveNone): def missing_meta(self): return False - def get_file_name(self, _sync_cache=True): + def get_file_name(self, sync_cache=True): return "None" diff --git a/lib/galaxy/tool_util/data/__init__.py b/lib/galaxy/tool_util/data/__init__.py index 94e27437939..849be7bb90e 100644 --- a/lib/galaxy/tool_util/data/__init__.py +++ b/lib/galaxy/tool_util/data/__init__.py @@ -891,7 +891,7 @@ class DirectoryAsExtraFiles(HasExtraFiles): class OutputDataset(HasExtraFiles, Protocol): ext: str - def get_file_name(self, _sync_cache=True) -> str: + def get_file_name(self, sync_cache=True) -> str: ... diff --git a/test/unit/app/jobs/dynamic_tool_destination/mockGalaxy.py b/test/unit/app/jobs/dynamic_tool_destination/mockGalaxy.py index 919eecfc642..17f4ce9cf72 100644 --- a/test/unit/app/jobs/dynamic_tool_destination/mockGalaxy.py +++ b/test/unit/app/jobs/dynamic_tool_destination/mockGalaxy.py @@ -43,7 +43,7 @@ class Dataset: def get_metadata(self): return self.metadata - def get_file_name(self, _sync_cache=True): + def get_file_name(self, sync_cache=True): return self.file_name_ diff --git a/test/unit/app/tools/test_wrappers.py b/test/unit/app/tools/test_wrappers.py index 96c18253e87..69cca55f204 100644 --- a/test/unit/app/tools/test_wrappers.py +++ b/test/unit/app/tools/test_wrappers.py @@ -312,7 +312,7 @@ class MockDataset: self.ext = MOCK_DATASET_EXT self.tags = [] - def get_file_name(self, _sync_cache=True): + def get_file_name(self, sync_cache=True): return MOCK_DATASET_PATH diff --git a/test/unit/data/datatypes/test_connectivity_table.py b/test/unit/data/datatypes/test_connectivity_table.py index 521ecb59c13..b73ad1fdb92 100644 --- a/test/unit/data/datatypes/test_connectivity_table.py +++ b/test/unit/data/datatypes/test_connectivity_table.py @@ -9,7 +9,7 @@ from galaxy.util import galaxy_directory @pytest.fixture def dataset(): class MockDataset: - def get_file_name(self, _sync_cache=True): + def get_file_name(self, sync_cache=True): return os.path.join(galaxy_directory(), "test-data/1.ct") return MockDataset() diff --git a/test/unit/data/datatypes/test_validation.py b/test/unit/data/datatypes/test_validation.py index 0b4164dd5e1..5ce7e214953 100644 --- a/test/unit/data/datatypes/test_validation.py +++ b/test/unit/data/datatypes/test_validation.py @@ -60,5 +60,5 @@ class MockDataset: self.file_name_ = file_name self.datatype = datatype - def get_file_name(self, _sync_cache=True): + def get_file_name(self, sync_cache=True): return self.file_name_ diff --git a/test/unit/data/datatypes/util.py b/test/unit/data/datatypes/util.py index df12959a6b8..ff0de253899 100644 --- a/test/unit/data/datatypes/util.py +++ b/test/unit/data/datatypes/util.py @@ -13,7 +13,7 @@ class MockDatasetDataset: self.purged = False self.file_name_ = file_name - def get_file_name(self, _sync_cache=True): + def get_file_name(self, sync_cache=True): return self.file_name_ def set_file_name(self, file_name): @@ -23,7 +23,7 @@ class MockDatasetDataset: class MockMetadata: file_name_: Optional[str] = None - def get_file_name(self, _sync_cache=True): + def get_file_name(self, sync_cache=True): return self.file_name_ def set_file_name(self, file_name): @@ -37,7 +37,7 @@ class MockDataset: self.dataset = None self.file_name_: Optional[str] = None - def get_file_name(self, _sync_cache=True): + def get_file_name(self, sync_cache=True): return self.file_name_ def set_file_name(self, file_name): diff --git a/test/unit/tool_util/data/test_tool_data_bundles.py b/test/unit/tool_util/data/test_tool_data_bundles.py index f4926156a32..2e1a1f27174 100644 --- a/test/unit/tool_util/data/test_tool_data_bundles.py +++ b/test/unit/tool_util/data/test_tool_data_bundles.py @@ -76,7 +76,7 @@ class OutputDataset: extra_files_path: str ext: str = "data_manager_json" - def get_file_name(self, _sync_cache=True) -> str: + def get_file_name(self, sync_cache=True) -> str: return self.file_name_ def extra_files_path_exists(self) -> bool: From 59be4046a91894b63b4b6f2b614dd1a7342d70f9 Mon Sep 17 00:00:00 2001 From: "Yakubov, Sergey" Date: Fri, 17 Nov 2023 12:13:49 -0500 Subject: [PATCH 7/8] comment changes to irods force write option --- lib/galaxy/objectstore/irods.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/galaxy/objectstore/irods.py b/lib/galaxy/objectstore/irods.py index b4744ca09cc..d2ac18fdca9 100644 --- a/lib/galaxy/objectstore/irods.py +++ b/lib/galaxy/objectstore/irods.py @@ -445,6 +445,9 @@ class IRODSObjectStore(DiskObjectStore, CloudConfigMixin): collection_path = f"{self.home}/{subcollection_name}" data_object_path = f"{collection_path}/{data_object_name}" + # we need to allow irods to override already existing zero-size output files created + # in object store cache during job setup (see also https://github.com/galaxyproject/galaxy/pull/17025#discussion_r1394517033) + # TODO: get rid of this flag when Galaxy stops pre-creating those output files in cache options = {kw.FORCE_FLAG_KW: "", kw.DEST_RESC_NAME_KW: self.resource} try: From 892c9b1888878bddb5798b47cfa01d33208113bd Mon Sep 17 00:00:00 2001 From: "Yakubov, Sergey" Date: Fri, 17 Nov 2023 12:23:29 -0500 Subject: [PATCH 8/8] document object store cache configuration --- .../config/sample/object_store_conf.xml.sample | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/lib/galaxy/config/sample/object_store_conf.xml.sample b/lib/galaxy/config/sample/object_store_conf.xml.sample index e6eeeaeabcb..2d726077a16 100644 --- a/lib/galaxy/config/sample/object_store_conf.xml.sample +++ b/lib/galaxy/config/sample/object_store_conf.xml.sample @@ -7,6 +7,19 @@ backends to the distributed and hierarchical object stores (including distributed and hierarchical themselves). --> + +