From 2a66a39ace1afed85d449677402cec78850bd39b Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Tue, 11 Feb 2020 11:54:27 +0100 Subject: [PATCH 1/7] Add test that should provoke https://github.com/galaxyproject/galaxy/issues/9074 --- test/unit/tools/test_tool_shed_repository_cache.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/unit/tools/test_tool_shed_repository_cache.py b/test/unit/tools/test_tool_shed_repository_cache.py index 383d8351191..c14b2058f35 100644 --- a/test/unit/tools/test_tool_shed_repository_cache.py +++ b/test/unit/tools/test_tool_shed_repository_cache.py @@ -54,3 +54,13 @@ def test_get_installed_repository(tool_shed_repository_cache, repos, tool_conf_r assert repo else: assert repo is None + + +def test_repo_cache_expunge(tool_shed_repository_cache, repos): + tool_shed_repository_cache.rebuild() + assert len(tool_shed_repository_cache.repositories) == 10 + repo = tool_shed_repository_cache.repositories[0] + repo.name = 'new name' + tool_shed_repository_cache.app.install_model.session.flush() + tool_shed_repository_cache.app.install_model.session.remove() + assert repo.changeset_revision == "1" From ff319b5fcdb503dc5404bc39baca782d78304055 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Tue, 11 Feb 2020 11:54:56 +0100 Subject: [PATCH 2/7] Fix ParentInstanceDetached due to expire_on_commit --- lib/galaxy/tools/cache.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/galaxy/tools/cache.py b/lib/galaxy/tools/cache.py index 768d1b1ab07..e501bf3c84a 100644 --- a/lib/galaxy/tools/cache.py +++ b/lib/galaxy/tools/cache.py @@ -149,12 +149,22 @@ class ToolShedRepositoryCache(object): self.repos_by_tuple[(repository.tool_shed, repository.owner, repository.name)].append(repository) def rebuild(self): - self.repositories = self.app.install_model.context.current.query(self.app.install_model.ToolShedRepository).options( + session = self.app.install_model.context.current + self.repositories = session.query(self.app.install_model.ToolShedRepository).options( defer(self.app.install_model.ToolShedRepository.metadata), joinedload('tool_dependencies').subqueryload('tool_shed_repository').options( defer(self.app.install_model.ToolShedRepository.metadata) ), ).all() + for r in self.repositories: + # We use sqlalchemy's expire_on_commit option for sessions, + # which means that attributes are fetched from the database + # after a commit occurs. + # The session used for establishing the cache however may have been closed, + # leading to ParentInstanceDetached errors. + # Expunging the repo prevents the refresh and should be + # safe since the cache will be rebuilt upon repository installations and uninstallations. + session.expunge(r) repos_by_tuple = defaultdict(list) for repository in self.repositories + self.local_repositories: repos_by_tuple[(repository.tool_shed, repository.owner, repository.name)].append(repository) From 098419b9b442c8556012d5cc68a2ad1d9657a5ff Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Tue, 11 Feb 2020 12:21:39 +0100 Subject: [PATCH 3/7] Ensure repository metadata isn't kept in memory --- test/unit/tools/test_tool_shed_repository_cache.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/unit/tools/test_tool_shed_repository_cache.py b/test/unit/tools/test_tool_shed_repository_cache.py index c14b2058f35..81da72c0a08 100644 --- a/test/unit/tools/test_tool_shed_repository_cache.py +++ b/test/unit/tools/test_tool_shed_repository_cache.py @@ -1,4 +1,5 @@ import pytest +from sqlalchemy.orm.exc import DetachedInstanceError from .conftest import create_repo @@ -64,3 +65,7 @@ def test_repo_cache_expunge(tool_shed_repository_cache, repos): tool_shed_repository_cache.app.install_model.session.flush() tool_shed_repository_cache.app.install_model.session.remove() assert repo.changeset_revision == "1" + with pytest.raises(DetachedInstanceError): + # Make sure this still raises DetachedInstanceError, + # keeping this in memory would be expensive + repo.metadata From 82a06af664aa14857f12bb866b075e4c2adf1a01 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Tue, 11 Feb 2020 12:24:40 +0100 Subject: [PATCH 4/7] Explain why the test does what it does --- test/unit/tools/test_tool_shed_repository_cache.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/unit/tools/test_tool_shed_repository_cache.py b/test/unit/tools/test_tool_shed_repository_cache.py index 81da72c0a08..f479d0d0f5f 100644 --- a/test/unit/tools/test_tool_shed_repository_cache.py +++ b/test/unit/tools/test_tool_shed_repository_cache.py @@ -60,9 +60,11 @@ def test_get_installed_repository(tool_shed_repository_cache, repos, tool_conf_r def test_repo_cache_expunge(tool_shed_repository_cache, repos): tool_shed_repository_cache.rebuild() assert len(tool_shed_repository_cache.repositories) == 10 + # Modify and commit a repo, will expire in memory attributes of orm objects (unless expunged) repo = tool_shed_repository_cache.repositories[0] repo.name = 'new name' tool_shed_repository_cache.app.install_model.session.flush() + # remove session, so access to expired attributes will raise exception tool_shed_repository_cache.app.install_model.session.remove() assert repo.changeset_revision == "1" with pytest.raises(DetachedInstanceError): From f4e32636e11f3b662ac5f260bd133926b9a34789 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Tue, 11 Feb 2020 12:35:16 +0100 Subject: [PATCH 5/7] Make sure tool dependency relationships are accessibly as well --- test/unit/tools/conftest.py | 9 +++++++++ test/unit/tools/test_tool_shed_repository_cache.py | 1 + 2 files changed, 10 insertions(+) diff --git a/test/unit/tools/conftest.py b/test/unit/tools/conftest.py index a7de3f768bc..ab8d702131e 100644 --- a/test/unit/tools/conftest.py +++ b/test/unit/tools/conftest.py @@ -65,4 +65,13 @@ def create_repo(app, changeset, installed_changeset, config_filename=None): repository.deleted = False repository.uninstalled = False app.install_model.context.add(repository) + app.install_model.context.flush() + tool_dependency = tool_shed_install.ToolDependency( + name='Name', + version='100', + type='package', + status='ok', + tool_shed_repository_id=repository.id, + ) + app.install_model.context.add(tool_dependency) return repository diff --git a/test/unit/tools/test_tool_shed_repository_cache.py b/test/unit/tools/test_tool_shed_repository_cache.py index f479d0d0f5f..a8db483deb8 100644 --- a/test/unit/tools/test_tool_shed_repository_cache.py +++ b/test/unit/tools/test_tool_shed_repository_cache.py @@ -67,6 +67,7 @@ def test_repo_cache_expunge(tool_shed_repository_cache, repos): # remove session, so access to expired attributes will raise exception tool_shed_repository_cache.app.install_model.session.remove() assert repo.changeset_revision == "1" + assert repo.tool_dependencies[0].name == 'Name' with pytest.raises(DetachedInstanceError): # Make sure this still raises DetachedInstanceError, # keeping this in memory would be expensive From 8a8c0990dd1e79169377ca731ea4ac122c492b98 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Tue, 11 Feb 2020 14:57:24 +0100 Subject: [PATCH 6/7] Give ToolShedRepositoryCache.rebuild its own session --- lib/galaxy/tools/cache.py | 34 ++++++++----------- .../tools/test_tool_shed_repository_cache.py | 4 +-- 2 files changed, 16 insertions(+), 22 deletions(-) diff --git a/lib/galaxy/tools/cache.py b/lib/galaxy/tools/cache.py index e501bf3c84a..36ce418ab75 100644 --- a/lib/galaxy/tools/cache.py +++ b/lib/galaxy/tools/cache.py @@ -149,26 +149,20 @@ class ToolShedRepositoryCache(object): self.repos_by_tuple[(repository.tool_shed, repository.owner, repository.name)].append(repository) def rebuild(self): - session = self.app.install_model.context.current - self.repositories = session.query(self.app.install_model.ToolShedRepository).options( - defer(self.app.install_model.ToolShedRepository.metadata), - joinedload('tool_dependencies').subqueryload('tool_shed_repository').options( - defer(self.app.install_model.ToolShedRepository.metadata) - ), - ).all() - for r in self.repositories: - # We use sqlalchemy's expire_on_commit option for sessions, - # which means that attributes are fetched from the database - # after a commit occurs. - # The session used for establishing the cache however may have been closed, - # leading to ParentInstanceDetached errors. - # Expunging the repo prevents the refresh and should be - # safe since the cache will be rebuilt upon repository installations and uninstallations. - session.expunge(r) - repos_by_tuple = defaultdict(list) - for repository in self.repositories + self.local_repositories: - repos_by_tuple[(repository.tool_shed, repository.owner, repository.name)].append(repository) - self.repos_by_tuple = repos_by_tuple + try: + session = self.app.install_model.context.current.session_factory() + self.repositories = session.query(self.app.install_model.ToolShedRepository).options( + defer(self.app.install_model.ToolShedRepository.metadata), + joinedload('tool_dependencies').subqueryload('tool_shed_repository').options( + defer(self.app.install_model.ToolShedRepository.metadata) + ), + ).all() + repos_by_tuple = defaultdict(list) + for repository in self.repositories + self.local_repositories: + repos_by_tuple[(repository.tool_shed, repository.owner, repository.name)].append(repository) + self.repos_by_tuple = repos_by_tuple + finally: + session.close() def get_installed_repository(self, tool_shed=None, name=None, owner=None, installed_changeset_revision=None, changeset_revision=None, repository_id=None): if repository_id: diff --git a/test/unit/tools/test_tool_shed_repository_cache.py b/test/unit/tools/test_tool_shed_repository_cache.py index a8db483deb8..ae99298292e 100644 --- a/test/unit/tools/test_tool_shed_repository_cache.py +++ b/test/unit/tools/test_tool_shed_repository_cache.py @@ -60,11 +60,11 @@ def test_get_installed_repository(tool_shed_repository_cache, repos, tool_conf_r def test_repo_cache_expunge(tool_shed_repository_cache, repos): tool_shed_repository_cache.rebuild() assert len(tool_shed_repository_cache.repositories) == 10 - # Modify and commit a repo, will expire in memory attributes of orm objects (unless expunged) + # Modify and commit a repo, will expire in memory attributes of orm objects (unless using a different session) repo = tool_shed_repository_cache.repositories[0] repo.name = 'new name' tool_shed_repository_cache.app.install_model.session.flush() - # remove session, so access to expired attributes will raise exception + # remove session, should demonstrate the separate session is in use tool_shed_repository_cache.app.install_model.session.remove() assert repo.changeset_revision == "1" assert repo.tool_dependencies[0].name == 'Name' From 4b1cd45c5bb2c686f2cfb14e6af22a1a510fb7bb Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Tue, 18 Feb 2020 12:36:41 +0100 Subject: [PATCH 7/7] Prevent tool execution getting stuck in history when a chromInfo dataset is in paused state If a chromInfo dataset is paused (or in other non-terminal states) execution will not start until the dataset is ready, which may be never. So instead we prefer datasets in OK state and ignore those that will never be OK. In theory that may fix some odd issues where jobs never start running. --- lib/galaxy/managers/context.py | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/lib/galaxy/managers/context.py b/lib/galaxy/managers/context.py index d8fb528ee0b..6699b80432d 100644 --- a/lib/galaxy/managers/context.py +++ b/lib/galaxy/managers/context.py @@ -180,12 +180,24 @@ class ProvidesHistoryContext(object): # The API presents a Bunch for a history. Until the API is # more fully featured for handling this, also return None. return None - datasets = self.sa_session.query(self.app.model.HistoryDatasetAssociation) \ - .filter_by(deleted=False, history_id=self.history.id, extension="len") + non_ready_or_ok = set(self.app.model.Dataset.non_ready_states) + non_ready_or_ok.add(self.app.model.HistoryDatasetAssociation.states.OK) + datasets = self.sa_session.query( + self.app.model.HistoryDatasetAssociation + ).filter_by( + deleted=False, + history_id=self.history.id, + extension="len" + ).filter( + self.app.model.HistoryDatasetAssociation._state.in_(non_ready_or_ok), + ) + valid_ds = None for ds in datasets: - if dbkey == ds.dbkey: - return ds - return None + if ds.dbkey == dbkey: + if ds.state == self.app.model.HistoryDatasetAssociation.states.OK: + return ds + valid_ds = ds + return valid_ds @property def db_builds(self):