diff --git a/.github/workflows/pr-checks.yaml b/.github/workflows/pr-checks.yaml index a12b545..931d8f4 100644 --- a/.github/workflows/pr-checks.yaml +++ b/.github/workflows/pr-checks.yaml @@ -39,6 +39,7 @@ jobs: conda env create -f environment.yml eval "$(conda shell.bash hook)" conda activate cellxgene-gateway + pip install markupsafe==2.0.1 # temporary workaround for jinja2-2.11.3 calling soft_unicode in markupsafe python setup.py install - name: Run tests diff --git a/README.md b/README.md index 6494c25..c823294 100644 --- a/README.md +++ b/README.md @@ -76,6 +76,7 @@ Optional environment variables: * `GATEWAY_EXTRA_SCRIPTS` - JSON array of script paths, will be embedded into each page and forwarded with `--scripts` to cellxgene server * `GATEWAY_ENABLE_ANNOTATIONS` - Set to `true` or to `1` to enable cellxgene annotations. * `GATEWAY_ENABLE_BACKED_MODE` - Set to `true` or to `1` to load AnnData in file-backed mode. This saves memory and speeds up launch time but may reduce overall performance. +* `S3_ENABLE_LISTINGS_CACHE` - Set to `true` or to `1` to cache listings of S3 folders for performance. Can be overridden by setting `filecrawl.html?refresh=true` query parameter. If any of the following optional variables are set, [ProxyFix](https://werkzeug.palletsprojects.com/en/1.0.x/middleware/proxy_fix/) will be used. * `PROXY_FIX_FOR` - Number of upstream proxies setting X-Forwarded-For diff --git a/cellxgene_gateway/gateway.py b/cellxgene_gateway/gateway.py index 4075437..11302ec 100644 --- a/cellxgene_gateway/gateway.py +++ b/cellxgene_gateway/gateway.py @@ -22,9 +22,7 @@ from flask import ( send_from_directory, url_for, ) -from flask_api import status from werkzeug.middleware.proxy_fix import ProxyFix -from werkzeug.utils import secure_filename from cellxgene_gateway import env, flask_util from cellxgene_gateway.backend_cache import BackendCache diff --git a/cellxgene_gateway/items/s3/s3item_source.py b/cellxgene_gateway/items/s3/s3item_source.py index aec9eca..130df19 100644 --- a/cellxgene_gateway/items/s3/s3item_source.py +++ b/cellxgene_gateway/items/s3/s3item_source.py @@ -7,9 +7,11 @@ # OR CONDITIONS OF ANY KIND, either express or implied. See the License for # the specific language governing permissions and limitations under the License. -from os.path import basename, dirname, join +import os +from os.path import basename, dirname from typing import List +import flask import s3fs from cellxgene_gateway import dir_util @@ -18,6 +20,10 @@ from cellxgene_gateway.items.item_source import ItemSource, LookupResult from cellxgene_gateway.items.s3.s3item import S3Item +def truthy(val: str): + return val.lower() in ["true", "1"] + + class S3ItemSource(ItemSource): def __init__( self, @@ -28,10 +34,9 @@ class S3ItemSource(ItemSource): annotation_file_suffix=".csv", ): self._name = name - disable_cache = os.environ.get("S3_DISABLE_LISTINGS_CACHE", "false").lower() - assert disable_cache in ['0', '1', 'false', 'true'] - self.use_listings_cache = disable_cache.lower() not in ["0", "false"] - + enable_cache = os.environ.get("S3_ENABLE_LISTINGS_CACHE", "false").lower() + assert enable_cache in ["0", "1", "false", "true"] + self.use_listings_cache = truthy(enable_cache) self.s3 = s3fs.S3FileSystem(use_listings_cache=self.use_listings_cache) if bucket.startswith("s3://"): raise Exception( @@ -71,6 +76,13 @@ class S3ItemSource(ItemSource): item_tree = self.scan_directory("" if filter is None else filter) return item_tree + @property + def refresh(self): + return ( + truthy(flask.request.args.get("refresh", default="false")) + or not self.use_listings_cache + ) + def scan_directory(self, directory_key="") -> dict: url = self.url(directory_key) @@ -79,7 +91,7 @@ class S3ItemSource(ItemSource): s3key_map = dict( (self.remove_bucket(filepath), "s3://" + filepath) - for filepath in sorted(self.s3.ls(url, refresh=not self.use_listings_cache)) + for filepath in sorted(self.s3.ls(url, refresh=self.refresh)) ) def is_annotation_dir(dir_s3key): @@ -170,7 +182,9 @@ class S3ItemSource(ItemSource): self.make_s3item_from_key( basename(annotation), self.remove_bucket(annotation), True ) - for annotation in sorted(self.s3.ls(annotations_fullpath, refresh=not self.use_listings_cache)) + for annotation in sorted( + self.s3.ls(annotations_fullpath, refresh=self.refresh) + ) if annotation.endswith(self.annotation_file_suffix) and self.s3.isfile("s3://" + annotation) ] diff --git a/tests/items/s3/test_s3item_source.py b/tests/items/s3/test_s3item_source.py index 10b62ea..7dcd124 100644 --- a/tests/items/s3/test_s3item_source.py +++ b/tests/items/s3/test_s3item_source.py @@ -24,7 +24,10 @@ class TestScanDirectory(unittest.TestCase): ) @patch("s3fs.S3FileSystem") - def test__GIVEN_multilevel_bucket_THEN_properly_recurses_suburls(self, s3func): + @patch("flask.request") + def test__GIVEN_multilevel_bucket_THEN_properly_recurses_suburls( + self, requestMock, s3func + ): class S3Mock: def exists(path): if path in [ @@ -38,7 +41,8 @@ class TestScanDirectory(unittest.TestCase): return True raise Exception("exists called with " + path) - def ls(path): + def ls(path, refresh): + assert refresh == True if path == "s3://my-bucket/": return [ "my-bucket/lvl1", @@ -79,6 +83,7 @@ class TestScanDirectory(unittest.TestCase): raise Exception("isfile called with " + path) s3func.return_value = S3Mock + requestMock.args.get.return_value = "true" source = S3ItemSource("my-bucket") tree = source.scan_directory()