From 98f367344ebe1497f1fcd6201c3829df7d1cc52e Mon Sep 17 00:00:00 2001 From: John Chilton Date: Tue, 6 Sep 2022 18:09:33 -0400 Subject: [PATCH 1/5] Type fixes for galaxy.tools.data --- lib/galaxy/tools/data/__init__.py | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/lib/galaxy/tools/data/__init__.py b/lib/galaxy/tools/data/__init__.py index 0437e58d254..f67512e8417 100644 --- a/lib/galaxy/tools/data/__init__.py +++ b/lib/galaxy/tools/data/__init__.py @@ -17,7 +17,11 @@ import string import time from glob import glob from tempfile import NamedTemporaryFile -from typing import List +from typing import ( + BinaryIO, + List, + Optional, +) import refgenconf import requests @@ -41,6 +45,8 @@ TOOL_DATA_TABLE_CONF_XML = """ class ToolDataPathFiles: + update_time: float + def __init__(self, tool_data_path): self.tool_data_path = os.path.abspath(tool_data_path) self.update_time = 0 @@ -67,7 +73,7 @@ class ToolDataPathFiles: ) self.update_time = time.time() except Exception: - log.exception() + log.exception("Failed to update _tool_data_path_files") self._tool_data_path_files = set() def exists(self, path): @@ -520,7 +526,7 @@ class TabularToolDataTable(ToolDataTable, Dictifiable): filename = f"{corrected_filename}.sample" found = True - errors = [] + errors: List[str] = [] if found: self.extend_data_with(filename, errors=errors) self._update_version() @@ -647,7 +653,7 @@ class TabularToolDataTable(ToolDataTable, Dictifiable): if not self.allow_duplicate_entries: self._deduplicate_data() - def parse_file_fields(self, filename, errors=None, here="__HERE__"): + def parse_file_fields(self, filename, errors: Optional[List[str]] = None, here="__HERE__"): """ Parse separated lines from file and return a list of tuples. @@ -783,10 +789,11 @@ class TabularToolDataTable(ToolDataTable, Dictifiable): if filename is None: # If we reach this point, there is no data table with a corresponding .loc file. raise MessageException( - f"Unable to determine filename for persisting data table '{self.name}' values: '{self.fields}'." + f"Unable to determine filename for persisting data table '{self.name}' values: '{fields}'." ) else: log.debug("Persisting changes to file: %s", filename) + data_table_fh: BinaryIO with FileLock(filename): try: if os.path.exists(filename): @@ -802,8 +809,8 @@ class TabularToolDataTable(ToolDataTable, Dictifiable): except OSError as e: log.exception("Error opening data table file (%s): %s", filename, e) raise - fields = f"{self.separator.join(fields)}\n" - data_table_fh.write(fields.encode("utf-8")) + fields_collapsed = f"{self.separator.join(fields)}\n" + data_table_fh.write(fields_collapsed.encode("utf-8")) def _remove_entry(self, values): From 019db37411da61c0d1ee27788df390799d365b23 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Wed, 7 Sep 2022 15:06:05 -0400 Subject: [PATCH 2/5] Unit tests for tool_data... --- lib/galaxy/managers/tool_data.py | 3 +- lib/galaxy/tools/data/__init__.py | 8 ++ lib/galaxy/tools/data/_schema.py | 7 ++ test/unit/app/tools/test_tool_data.py | 156 ++++++++++++++++++++++++++ 4 files changed, 172 insertions(+), 2 deletions(-) create mode 100644 test/unit/app/tools/test_tool_data.py diff --git a/lib/galaxy/managers/tool_data.py b/lib/galaxy/managers/tool_data.py index 3e42c630a93..03f0caa9bbc 100644 --- a/lib/galaxy/managers/tool_data.py +++ b/lib/galaxy/managers/tool_data.py @@ -31,8 +31,7 @@ class ToolDataManager: def index(self) -> ToolDataEntryList: """Return all tool data tables.""" - data_tables = [table.to_dict() for table in self.data_tables.values()] - return ToolDataEntryList.construct(__root__=data_tables) + return self._app.tool_data_tables.index() def show(self, table_name: str) -> ToolDataDetails: """Get details of a given data table""" diff --git a/lib/galaxy/tools/data/__init__.py b/lib/galaxy/tools/data/__init__.py index f67512e8417..7cffd2eae27 100644 --- a/lib/galaxy/tools/data/__init__.py +++ b/lib/galaxy/tools/data/__init__.py @@ -33,6 +33,10 @@ from galaxy.util.dictifiable import Dictifiable from galaxy.util.filelock import FileLock from galaxy.util.renamed_temporary_file import RenamedTemporaryFile from galaxy.util.template import fill_template +from ._schema import ( + ToolDataEntry, + ToolDataEntryList, +) log = logging.getLogger(__name__) @@ -102,6 +106,10 @@ class ToolDataTableManager(Dictifiable): continue self.load_from_config_file(single_config_filename, self.tool_data_path, from_shed_config=False) + def index(self) -> ToolDataEntryList: + data_tables = [ToolDataEntry(**table.to_dict()) for table in self.data_tables.values()] + return ToolDataEntryList.construct(__root__=data_tables) + def __getitem__(self, key): return self.data_tables.__getitem__(key) diff --git a/lib/galaxy/tools/data/_schema.py b/lib/galaxy/tools/data/_schema.py index 5fbb8511b10..f684452a18a 100644 --- a/lib/galaxy/tools/data/_schema.py +++ b/lib/galaxy/tools/data/_schema.py @@ -1,6 +1,7 @@ from typing import ( Dict, List, + Optional, ) from pydantic import Field @@ -28,6 +29,12 @@ class ToolDataEntryList(Model): title="A list with details on individual data tables.", ) + def find_entry(self, name: str) -> Optional[ToolDataEntry]: + for entry in self.__root__: + if entry.name == name: + return entry + return None + class ToolDataDetails(ToolDataEntry): columns: List[str] = Field( diff --git a/test/unit/app/tools/test_tool_data.py b/test/unit/app/tools/test_tool_data.py new file mode 100644 index 00000000000..a12741e12a9 --- /dev/null +++ b/test/unit/app/tools/test_tool_data.py @@ -0,0 +1,156 @@ +import pytest + +from galaxy.tools.data import ToolDataTableManager + +LOC_ALPHA_CONTENTS = """ +data1 data1name ${__HERE__}/data1/entry.txt +data2 data2name ${__HERE__}/data2/entry.txt +""" + + +LOC_ALPHA_CONTENTS_V2 = """ +data1 data1name ${__HERE__}/data1/entry.txt +data2 data2name ${__HERE__}/data2/entry.txt +data3 data3name ${__HERE__}/data3/entry.txt +""" + + +LOC_BETA_CONTENTS_1 = """ +beta1 beta1name ${__HERE__}/beta1/entry.txt +""" + + +LOC_BETA_CONTENTS_2 = """ +beta2 beta2name ${__HERE__}/beta2/entry.txt +""" + + +TOOL_DATA_TABLE_CONF_XML = """ + + value, name, path + +
+ +""" + + +MERGED_TOOL_DATA_TABLE_CONF_XML_1 = """ + + value, name, path + +
+ +""" + + +MERGED_TOOL_DATA_TABLE_CONF_XML_2 = """ + + value, name, path + +
+ +""" + + +@pytest.fixture +def tdt_manager(tmp_path) -> ToolDataTableManager: + _write_loc_files(tmp_path) + conf = tmp_path / "tool_data_table_conf.xml" + conf.write_text(TOOL_DATA_TABLE_CONF_XML) + return ToolDataTableManager(tmp_path, conf) + + +@pytest.fixture +def merged_tdt_manager(tmp_path) -> ToolDataTableManager: + _write_loc_files(tmp_path) + conf1 = tmp_path / "tool_data_table_conf_1.xml" + conf1.write_text(MERGED_TOOL_DATA_TABLE_CONF_XML_1) + conf2 = tmp_path / "tool_data_table_conf_2.xml" + conf2.write_text(MERGED_TOOL_DATA_TABLE_CONF_XML_2) + return ToolDataTableManager(tmp_path, f"{conf1},{conf2}") + + +def _write_loc_files(tmp_path): + loc1 = tmp_path / "testalpha.loc" + loc1.write_text(LOC_ALPHA_CONTENTS) + + loc2 = tmp_path / "testbeta1.loc" + loc2.write_text(LOC_BETA_CONTENTS_1) + + loc3 = tmp_path / "testbeta2.loc" + loc3.write_text(LOC_BETA_CONTENTS_2) + + data1 = tmp_path / "data1" + data1.mkdir() + data1_entry = data1 / "entry.txt" + data1_entry.write_text("This is data 1.") + + data2 = tmp_path / "data2" + data2.mkdir() + data2_entry = data2 / "entry.txt" + data2_entry.write_text("This is data 2.") + + data3 = tmp_path / "data3" + data3.mkdir() + data3_entry = data3 / "entry.txt" + data3_entry.write_text("This is data 3.") + + beta1 = tmp_path / "beta1" + beta1.mkdir() + beta1_entry = beta1 / "entry.txt" + beta1_entry.write_text("This is beta 1.") + + beta2 = tmp_path / "beta2" + beta2.mkdir() + beta2_entry = beta2 / "entry.txt" + beta2_entry.write_text("This is beta 2.") + + +def test_data_tables_as_dictionary(tdt_manager): + assert "testalpha" in tdt_manager.data_tables + assert "testdelta" not in tdt_manager.data_tables + + +def test_to_dict(tdt_manager): + as_dict = tdt_manager.to_dict() + assert "testalpha" in as_dict + assert "testdelta" not in as_dict + testalpha_as_dict = as_dict["testalpha"] + assert "columns" in testalpha_as_dict + + +def test_index(tdt_manager): + index = tdt_manager.index() + assert len(index.__root__) >= 1 + entry = index.find_entry("testalpha") + assert entry + entry = index.find_entry("testomega") + assert not entry + + +def test_reload(tdt_manager, tmp_path): + assert len(tdt_manager["testalpha"].data) == 2 + loc1 = tmp_path / "testalpha.loc" + loc1.write_text(LOC_ALPHA_CONTENTS_V2) + tdt_manager.reload_tables() + assert len(tdt_manager["testalpha"].data) == 3 + + +def test_reload_by_path(tdt_manager, tmp_path): + assert len(tdt_manager["testalpha"].data) == 2 + loc1 = tmp_path / "testalpha.loc" + loc1.write_text(LOC_ALPHA_CONTENTS_V2) + tdt_manager.reload_tables(path=str(loc1)) + assert len(tdt_manager["testalpha"].data) == 3 + + +def test_reload_by_name(tdt_manager, tmp_path): + assert len(tdt_manager["testalpha"].data) == 2 + loc1 = tmp_path / "testalpha.loc" + loc1.write_text(LOC_ALPHA_CONTENTS_V2) + tdt_manager.reload_tables("testalpha") + assert len(tdt_manager["testalpha"].data) == 3 + + +def test_merging_tables(merged_tdt_manager): + assert len(merged_tdt_manager["testbeta"].data) == 2 From 1e664c32066697cae6652cdd634b0b0ba75cb5e9 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Thu, 8 Sep 2022 11:35:54 -0400 Subject: [PATCH 3/5] More tool data table typing and testing. --- lib/galaxy/managers/tool_data.py | 13 ++++-- lib/galaxy/tools/data/__init__.py | 59 ++++++++++++++++++--------- lib/galaxy/util/__init__.py | 5 ++- test/unit/app/tools/test_tool_data.py | 7 ++++ 4 files changed, 60 insertions(+), 24 deletions(-) diff --git a/lib/galaxy/managers/tool_data.py b/lib/galaxy/managers/tool_data.py index 03f0caa9bbc..170f3f61144 100644 --- a/lib/galaxy/managers/tool_data.py +++ b/lib/galaxy/managers/tool_data.py @@ -1,5 +1,6 @@ from pathlib import Path from typing import ( + cast, Dict, Optional, ) @@ -9,6 +10,7 @@ from galaxy.structured_app import StructuredApp from galaxy.tools.data import ( TabularToolDataField, TabularToolDataTable, + ToolDataTable, ) from galaxy.tools.data._schema import ( ToolDataDetails, @@ -26,7 +28,7 @@ class ToolDataManager: self._app = app @property - def data_tables(self) -> Dict[str, TabularToolDataTable]: + def data_tables(self) -> Dict[str, ToolDataTable]: return self._app.tool_data_tables.data_tables def index(self) -> ToolDataEntryList: @@ -61,7 +63,7 @@ class ToolDataManager: def delete(self, table_name: str, values: Optional[str] = None) -> ToolDataDetails: """Removes an item from a data table""" - data_table = self._data_table(table_name) + data_table = self._tabular_data_table(table_name) if not values: raise exceptions.RequestParameterInvalidException("Invalid values for data table item specified.") @@ -75,14 +77,17 @@ class ToolDataManager: data_table.remove_entry(split_values) return self._reload_data_table(table_name) - def _data_table(self, table_name: str) -> TabularToolDataTable: + def _tabular_data_table(self, table_name: str) -> TabularToolDataTable: + return cast(TabularToolDataTable, self._data_table(table_name)) + + def _data_table(self, table_name: str) -> ToolDataTable: try: return self.data_tables[table_name] except KeyError: raise exceptions.ObjectNotFound(f"No such data table {table_name}") def _data_table_field(self, table_name: str, field_name: str) -> TabularToolDataField: - out = self._data_table(table_name).get_field(field_name) + out = self._tabular_data_table(table_name).get_field(field_name) if out is None: raise exceptions.ObjectNotFound(f"No such field {field_name} in data table {table_name}.") return out diff --git a/lib/galaxy/tools/data/__init__.py b/lib/galaxy/tools/data/__init__.py index 7cffd2eae27..4bca090c6e8 100644 --- a/lib/galaxy/tools/data/__init__.py +++ b/lib/galaxy/tools/data/__init__.py @@ -18,9 +18,14 @@ import time from glob import glob from tempfile import NamedTemporaryFile from typing import ( + Any, BinaryIO, + Dict, List, Optional, + Set, + Type, + Union, ) import refgenconf @@ -56,12 +61,12 @@ class ToolDataPathFiles: self.update_time = 0 @property - def tool_data_path_files(self): + def tool_data_path_files(self) -> Set[str]: if time.time() - self.update_time > 1: self.update_files() return self._tool_data_path_files - def update_files(self): + def update_files(self) -> None: try: content = os.walk(self.tool_data_path) self._tool_data_path_files = set( @@ -80,7 +85,7 @@ class ToolDataPathFiles: log.exception("Failed to update _tool_data_path_files") self._tool_data_path_files = set() - def exists(self, path): + def exists(self, path: str) -> bool: path = os.path.abspath(path) if path in self.tool_data_path_files: return True @@ -88,11 +93,20 @@ class ToolDataPathFiles: return os.path.exists(path) +ConfigFilesT = Union[str, os.PathLike, List[Union[str, os.PathLike]]] + + class ToolDataTableManager(Dictifiable): """Manages a collection of tool data tables""" + data_tables: Dict[str, "ToolDataTable"] + def __init__( - self, tool_data_path, config_filename=None, tool_data_table_config_path_set=None, other_config_dict=None + self, + tool_data_path: str, + config_filename: Optional[ConfigFilesT] = None, + tool_data_table_config_path_set=None, + other_config_dict=None, ): self.tool_data_path = tool_data_path # This stores all defined data table entries from both the tool_data_table_conf.xml file and the shed_tool_data_table_conf.xml file @@ -110,41 +124,43 @@ class ToolDataTableManager(Dictifiable): data_tables = [ToolDataEntry(**table.to_dict()) for table in self.data_tables.values()] return ToolDataEntryList.construct(__root__=data_tables) - def __getitem__(self, key): + def __getitem__(self, key: str): return self.data_tables.__getitem__(key) - def __setitem__(self, key, value): + def __setitem__(self, key: str, value): return self.data_tables.__setitem__(key, value) - def __contains__(self, key): + def __contains__(self, key: str): return self.data_tables.__contains__(key) - def get(self, name, default=None): + def get(self, name: str, default=None): try: return self[name] except KeyError: return default - def set(self, name, value): + def set(self, name: str, value): self[name] = value - def get_tables(self): + def get_tables(self) -> Dict[str, "ToolDataTable"]: return self.data_tables - def to_dict(self): + def to_dict(self, view: str = "collection", value_mapper=None): return {name: data_table.to_dict(view="export") for name, data_table in self.data_tables.items()} - def to_json(self, path): + def to_json(self, path: Union[str, os.PathLike]) -> None: with open(path, "w") as out: out.write(json.dumps(self.to_dict())) @classmethod - def from_dict(cls, d): + def from_dict(cls, d: Dict[str, Any]): tdtm = cls.__new__(cls) tdtm.data_tables = {name: ToolDataTable.from_dict(data) for name, data in d.items()} return tdtm - def load_from_config_file(self, config_filename, tool_data_path, from_shed_config=False): + def load_from_config_file( + self, config_filename: ConfigFilesT, tool_data_path: Union[str, os.PathLike], from_shed_config: bool = False + ): """ This method is called under 3 conditions: @@ -155,9 +171,12 @@ class ToolDataTableManager(Dictifiable): Galaxy instance. In this case, we have 2 entry types to handle, files whose root tag is , for example: """ table_elems = [] + config_filenames: List[Union[str, os.PathLike]] if not isinstance(config_filename, list): - config_filename = [config_filename] - for filename in config_filename: + config_filenames = [config_filename] + else: + config_filenames = config_filename + for filename in config_filenames: tree = util.parse_xml(filename) root = tree.getroot() for table_elem in root.findall("table"): @@ -297,8 +316,9 @@ class ToolDataTableManager(Dictifiable): return list(table_names) -class ToolDataTable: +class ToolDataTable(Dictifiable): type_key: str + data: List @classmethod def from_elem( @@ -409,7 +429,7 @@ class ToolDataTable: return self._update_version() -class TabularToolDataTable(ToolDataTable, Dictifiable): +class TabularToolDataTable(ToolDataTable): """ Data stored in a tabular / separated value format on disk, allows multiple files to be merged but all must have the same column definitions: @@ -1101,4 +1121,5 @@ def expand_here_template(content, here=None): # Registry of tool data types by type_key -tool_data_table_types = {cls.type_key: cls for cls in [TabularToolDataTable, RefgenieToolDataTable]} +tool_data_table_types_list: List[Type[ToolDataTable]] = [TabularToolDataTable, RefgenieToolDataTable] +tool_data_table_types = {cls.type_key: cls for cls in tool_data_table_types_list} diff --git a/lib/galaxy/util/__init__.py b/lib/galaxy/util/__init__.py index aaab5c45b36..2529ac26c7a 100644 --- a/lib/galaxy/util/__init__.py +++ b/lib/galaxy/util/__init__.py @@ -31,12 +31,15 @@ from email.mime.multipart import MIMEMultipart from email.mime.text import MIMEText from hashlib import md5 from os.path import relpath +<<<<<<< HEAD from pathlib import Path from typing import ( Any, Optional, overload, ) +======= +>>>>>>> 18a6e028d7 (More tool data table typing and testing.) from urllib.parse import ( urlencode, urlparse, @@ -284,7 +287,7 @@ def unique_id(KEY_SIZE=128): return md5(random_bits).hexdigest() -def parse_xml(fname: typing.Union[str, Path], strip_whitespace=True, remove_comments=True): +def parse_xml(fname: typing.Union[str, os.PathLike], strip_whitespace=True, remove_comments=True): """Returns a parsed xml tree""" parser = None if remove_comments and LXML_AVAILABLE: diff --git a/test/unit/app/tools/test_tool_data.py b/test/unit/app/tools/test_tool_data.py index a12741e12a9..b6ef78d781f 100644 --- a/test/unit/app/tools/test_tool_data.py +++ b/test/unit/app/tools/test_tool_data.py @@ -154,3 +154,10 @@ def test_reload_by_name(tdt_manager, tmp_path): def test_merging_tables(merged_tdt_manager): assert len(merged_tdt_manager["testbeta"].data) == 2 + + +def test_to_json(merged_tdt_manager, tmp_path): + json_path = tmp_path / "as_json.json" + assert not json_path.exists() + merged_tdt_manager.to_json(json_path) + assert json_path.exists() From 84a21b29f2b37174a8e7200cc6dfb5084aeefa92 Mon Sep 17 00:00:00 2001 From: Bjoern Gruening Date: Sun, 11 Sep 2022 12:22:00 +0200 Subject: [PATCH 4/5] fix bad merge --- lib/galaxy/util/__init__.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/lib/galaxy/util/__init__.py b/lib/galaxy/util/__init__.py index 2529ac26c7a..cc013e89623 100644 --- a/lib/galaxy/util/__init__.py +++ b/lib/galaxy/util/__init__.py @@ -31,15 +31,12 @@ from email.mime.multipart import MIMEMultipart from email.mime.text import MIMEText from hashlib import md5 from os.path import relpath -<<<<<<< HEAD from pathlib import Path from typing import ( Any, Optional, overload, ) -======= ->>>>>>> 18a6e028d7 (More tool data table typing and testing.) from urllib.parse import ( urlencode, urlparse, @@ -1021,7 +1018,7 @@ def asbool(obj): return bool(obj) -def string_as_bool(string: typing.Any) -> bool: +def string_as_bool(string: Any) -> bool: if str(string).lower() in ("true", "yes", "on", "1"): return True else: @@ -1047,7 +1044,7 @@ def string_as_bool_or_none(string): return False -def listify(item, do_strip=False) -> typing.List[typing.Any]: +def listify(item, do_strip=False) -> typing.List[Any]: """ Make a single item a single item list. From 32fdbfa8d7cb60fd0368967616bdd9337537033e Mon Sep 17 00:00:00 2001 From: Bjoern Gruening Date: Sun, 11 Sep 2022 13:05:50 +0200 Subject: [PATCH 5/5] fix linting --- lib/galaxy/util/__init__.py | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/galaxy/util/__init__.py b/lib/galaxy/util/__init__.py index cc013e89623..6d94baee4a0 100644 --- a/lib/galaxy/util/__init__.py +++ b/lib/galaxy/util/__init__.py @@ -31,7 +31,6 @@ from email.mime.multipart import MIMEMultipart from email.mime.text import MIMEText from hashlib import md5 from os.path import relpath -from pathlib import Path from typing import ( Any, Optional,