diff --git a/lib/galaxy/app_unittest_utils/galaxy_mock.py b/lib/galaxy/app_unittest_utils/galaxy_mock.py index 9f4fe40139e..d69a6c763af 100644 --- a/lib/galaxy/app_unittest_utils/galaxy_mock.py +++ b/lib/galaxy/app_unittest_utils/galaxy_mock.py @@ -5,6 +5,7 @@ Mock infrastructure for testing ModelManagers. import os import shutil import tempfile +from collections.abc import Hashable from typing import ( Any, cast, @@ -335,6 +336,7 @@ class MockTrans: self.__user = user self.security = self.app.security self.history = history + self._short_term_cache: dict[tuple[Hashable, ...], Any] = {} self.request: Any = Bunch( headers={}, @@ -349,6 +351,12 @@ class MockTrans: def tag_handler(self): return self.app.tag_handler + def set_cache_value(self, args: tuple[Hashable, ...], value: Any): + self._short_term_cache[args] = value + + def get_cache_value(self, args: tuple[Hashable, ...], default: Any = None) -> Any: + return self._short_term_cache.get(args, default) + def check_csrf_token(self, payload): pass diff --git a/lib/galaxy/managers/context.py b/lib/galaxy/managers/context.py index 1f6447623fd..b610448f78c 100644 --- a/lib/galaxy/managers/context.py +++ b/lib/galaxy/managers/context.py @@ -37,7 +37,10 @@ A method that requires a user but not a history should declare its # more checks against this issue. import abc import string -from collections.abc import Callable +from collections.abc import ( + Callable, + Hashable, +) from json import dumps from typing import ( Any, @@ -201,12 +204,12 @@ class ProvidesUserContext(ProvidesAppContext): workflow_building_mode: Literal[1, True, False] = False galaxy_session: Optional[GalaxySession] = None _tag_handler: Optional[GalaxyTagHandlerSession] = None - _short_term_cache: dict[tuple[str, ...], Any] + _short_term_cache: dict[tuple[Hashable, ...], Any] - def set_cache_value(self, args: tuple[str, ...], value: Any): + def set_cache_value(self, args: tuple[Hashable, ...], value: Any): self._short_term_cache[args] = value - def get_cache_value(self, args: tuple[str, ...], default: Any = None) -> Any: + def get_cache_value(self, args: tuple[Hashable, ...], default: Any = None) -> Any: return self._short_term_cache.get(args, default) @property diff --git a/lib/galaxy/tools/parameters/basic.py b/lib/galaxy/tools/parameters/basic.py index 9ea2bcae2f1..c4b61049981 100644 --- a/lib/galaxy/tools/parameters/basic.py +++ b/lib/galaxy/tools/parameters/basic.py @@ -102,6 +102,7 @@ from .workflow_utils import ( if TYPE_CHECKING: from sqlalchemy.orm import Session + from galaxy.managers.context import ProvidesHistoryContext from galaxy.model import ( History, HistoryItem, @@ -1874,6 +1875,11 @@ def _carried_state_label(value) -> str: return "not in current history" +# Sentinel distinguishing a populated cache entry (always a ``(rows, total)`` +# tuple) from a cache miss. +_PAGE_CACHE_MISS: Any = object() + + class BaseDataToolParameter(ToolParameter): multiple: bool @@ -1969,6 +1975,68 @@ class BaseDataToolParameter(ToolParameter): self._acceptable_extensions_cache = accepted return accepted + def _paginated_visible_datasets( + self, + trans: "ProvidesHistoryContext", + history: "History", + *, + extensions: Optional[set[str]], + valid_states: Optional[tuple[str, ...]], + search: Optional[str] = None, + offset: int = 0, + limit: int = 50, + ) -> tuple[list[HistoryDatasetAssociation], int]: + """``history.paginated_active_visible_datasets`` memoized on the request. + + Building one form with many ``data`` parameters (most notably the workflow + Run form, which renders every step in a single request) otherwise re-issues + the same paginated SQL against the same, unchanging history once per + parameter -- O(parameters) round-trips, slow even on an empty history + (issue #22927). The results are cached by signature on the request context's + short-term cache (see ``ProvidesHistoryContext.set_cache_value``); the cache + is shared across every step's proxy work context for the request and never + outlives it, so the history cannot change underneath it. + """ + key = ( + "data_param_hda_page", + history.id, + frozenset(extensions) if extensions is not None else None, + tuple(valid_states) if valid_states is not None else None, + search or None, + offset, + limit, + ) + cached = trans.get_cache_value(key, _PAGE_CACHE_MISS) + if cached is not _PAGE_CACHE_MISS: + return cached + result = history.paginated_active_visible_datasets( + extensions=extensions, valid_states=valid_states, search=search, offset=offset, limit=limit + ) + trans.set_cache_value(key, result) + return result + + def _paginated_dataset_collections( + self, + trans: "ProvidesHistoryContext", + history: "History", + *, + visible_only: bool, + search: Optional[str] = None, + offset: int = 0, + limit: int = 50, + ) -> tuple[list[HistoryDatasetCollectionAssociation], int]: + """``history.paginated_active_dataset_collections`` memoized on the request + context's short-term cache (see :meth:`_paginated_visible_datasets`).""" + key = ("data_param_hdca_page", history.id, bool(visible_only), search or None, offset, limit) + cached = trans.get_cache_value(key, _PAGE_CACHE_MISS) + if cached is not _PAGE_CACHE_MISS: + return cached + result = history.paginated_active_dataset_collections( + visible_only=visible_only, search=search, offset=offset, limit=limit + ) + trans.set_cache_value(key, result) + return result + def _uses_python_options_filter(self) -> bool: """True iff matching depends on per-row Python state that cannot be pushed to SQL (dynamic options with ``options_filter_attribute``, or a @@ -1993,7 +2061,9 @@ class BaseDataToolParameter(ToolParameter): chunk_size = MAX_OPTIONS_PAGE_SIZE db_offset = 0 while True: - rows, total = history.paginated_active_visible_datasets( + rows, total = self._paginated_visible_datasets( + trans, + history, extensions=self._acceptable_extensions(), valid_states=dataset_matcher_factory.valid_input_states, offset=db_offset, @@ -2013,17 +2083,19 @@ class BaseDataToolParameter(ToolParameter): chunk_size = MAX_OPTIONS_PAGE_SIZE db_offset = 0 while True: - rows, total = history.paginated_active_dataset_collections( + collection_rows, total = self._paginated_dataset_collections( + trans, + history, visible_only=True, offset=db_offset, limit=chunk_size, ) - if not rows: + if not collection_rows: return None - for hdca in rows: + for hdca in collection_rows: if dataset_collection_matcher.hdca_match(hdca): return hdca - db_offset += len(rows) + db_offset += len(collection_rows) if db_offset >= total: return None @@ -2517,7 +2589,7 @@ class DataToolParameter(BaseDataToolParameter): job_input_values = util.listify(other_values.get(self.name)) job_input_values = self._page_hda_matches( - builder, history, dataset_matcher, dataset_matcher_factory, job_input_values + trans, builder, history, dataset_matcher, dataset_matcher_factory, job_input_values ) unresolved = self._pin_live_hda_inputs(builder, history, dataset_matcher, job_input_values) self._carry_unresolved_inputs(builder, history, unresolved) @@ -2553,6 +2625,7 @@ class DataToolParameter(BaseDataToolParameter): def _page_hda_matches( self, + trans, builder: DataOptionsBuilder, history, dataset_matcher, @@ -2569,7 +2642,9 @@ class DataToolParameter(BaseDataToolParameter): valid_states = dataset_matcher_factory.valid_input_states def hda_query(*, offset, limit): - return history.paginated_active_visible_datasets( + return self._paginated_visible_datasets( + trans, + history, extensions=acceptable_extensions, valid_states=valid_states, search=hda_search, @@ -2677,8 +2752,8 @@ class DataToolParameter(BaseDataToolParameter): _offset, _limit, hdca_search = builder.page("hdca") def hdca_query(*, offset, limit): - return history.paginated_active_dataset_collections( - visible_only=True, search=hdca_search, offset=offset, limit=limit + return self._paginated_dataset_collections( + trans, history, visible_only=True, search=hdca_search, offset=offset, limit=limit ) def hdca_filter(hdca): @@ -2902,8 +2977,8 @@ class DataCollectionToolParameter(BaseDataToolParameter): history_query = self._history_query(trans) def hdca_query(*, offset, limit): - return history.paginated_active_dataset_collections( - visible_only=False, search=hdca_search, offset=offset, limit=limit + return self._paginated_dataset_collections( + trans, history, visible_only=False, search=hdca_search, offset=offset, limit=limit ) def hdca_filter(hdca): diff --git a/lib/galaxy/webapps/base/webapp.py b/lib/galaxy/webapps/base/webapp.py index 76980af1c34..704df9aecc0 100644 --- a/lib/galaxy/webapps/base/webapp.py +++ b/lib/galaxy/webapps/base/webapp.py @@ -7,6 +7,7 @@ import os import re import socket import time +from collections.abc import Hashable from contextlib import ExitStack from http.cookies import CookieError from typing import ( @@ -337,7 +338,7 @@ class GalaxyWebTransaction(base.DefaultWebTransaction, context.ProvidesHistoryCo self.galaxy_session = None self.error_message = None self.host = self.request.host - self._short_term_cache: dict[tuple[str, ...], Any] = {} + self._short_term_cache: dict[tuple[Hashable, ...], Any] = {} # set any cross origin resource sharing headers if configured to do so self.set_cors_headers() diff --git a/lib/galaxy/work/context.py b/lib/galaxy/work/context.py index 953328c4b0c..d1a5831ddfc 100644 --- a/lib/galaxy/work/context.py +++ b/lib/galaxy/work/context.py @@ -1,4 +1,5 @@ import abc +from collections.abc import Hashable from typing import ( Any, Literal, @@ -39,13 +40,19 @@ class WorkRequestContext(ProvidesHistoryContext): workflow_building_mode=False, url_builder=None, galaxy_session: Optional["GalaxySession"] = None, + short_term_cache: Optional[dict[tuple[Hashable, ...], Any]] = None, ): self._app = app self.__user = user self.__user_current_roles: Optional[list[Role]] = None self.__history = history self._url_builder = url_builder - self._short_term_cache: dict[tuple[str, ...], Any] = {} + # When proxying an existing transaction (see ``proxy_work_context_for_history``) + # share its request-scoped cache so work done across proxies of the same + # request -- e.g. every step of a workflow Run form build -- is reused. + self._short_term_cache: dict[tuple[Hashable, ...], Any] = ( + short_term_cache if short_term_cache is not None else {} + ) self.workflow_building_mode = workflow_building_mode self.galaxy_session = galaxy_session @@ -195,4 +202,5 @@ def proxy_work_context_for_history( url_builder=trans.url_builder, workflow_building_mode=workflow_building_mode, galaxy_session=trans.galaxy_session, + short_term_cache=trans._short_term_cache, ) diff --git a/test/integration/test_workflow_refactoring.py b/test/integration/test_workflow_refactoring.py index 8b626ab22e7..951edb02250 100644 --- a/test/integration/test_workflow_refactoring.py +++ b/test/integration/test_workflow_refactoring.py @@ -1,5 +1,6 @@ import contextlib import json +from collections.abc import Hashable from typing import ( Any, ) @@ -1052,6 +1053,7 @@ class MockTrans(ProvidesAppContext): self.history = None self.workflow_building_mode = workflow_building_modes.ENABLED self.tag_handler = app.tag_handler + self._short_term_cache: dict[tuple[Hashable, ...], Any] = {} @property def galaxy_session(self): diff --git a/test/unit/app/tools/test_execution.py b/test/unit/app/tools/test_execution.py index 42260bb4516..54c0b5d008e 100644 --- a/test/unit/app/tools/test_execution.py +++ b/test/unit/app/tools/test_execution.py @@ -1,7 +1,11 @@ """Test Tool execution and state handling logic.""" from collections import OrderedDict -from typing import cast +from collections.abc import Hashable +from typing import ( + Any, + cast, +) import webob.exc from sqlalchemy import select @@ -206,6 +210,7 @@ class MockTrans: self.sa_session = self.app.model.context self.url_builder = None self.galaxy_session = None + self._short_term_cache: dict[tuple[Hashable, ...], Any] = {} def get_history(self, **kwargs): return self.history