From 0b87f78e5b8be95254d36304c10107825858992e Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Mon, 18 May 2026 16:30:31 +0200 Subject: [PATCH] LazyToolBox: drop to_panel_view + _index_entry_to_api_dict; let super render MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With ``_tool_panel`` filled with ``LazyTool`` stubs by the seam-driven walk, the parent ``AbstractToolBox.to_panel_view`` renders the default view from ``_tool_panel_view_rendered`` (cached at boot by ``_load_tool_panel_views``). For each tool encountered it calls ``ToolSection.to_dict(only_ids=True)`` → ``tool.to_dict(link_details=False)``, which lands in ``LazyTool.to_dict``'s entry-only fast path. No materialisation; the lazy-custom rendering is redundant. ``_index_entry_to_api_dict`` goes away with it — the same shape now comes from ``LazyTool.to_dict(link_details=False)``. Also drops the leftover ``_populate_tool_registry_from_index`` block (missed in commit 4f82c319fcc — it referenced ``_tool_section_map`` which no longer exists, and was orphaned the moment ``__init__`` stopped calling it). With its removal, ``ToolSection`` / ``panel_item_types`` are still referenced by the panel post-walk stamping helpers, but ``EdamToolPanelView`` is now fully unused — import dropped. ``test_default_panel_view_section_tools_use_id_list`` was asserting that section ``tools`` lists held only id strings — that was specific to the lazy fast path. The eager renderer emits ``ToolSectionLabel`` dicts interleaved with id strings (that's what ``ToolSection.to_dict(only_ids=True)`` produces for a section that contains labels). The test's true intent is to prevent regression where full *Tool* dicts appear in ``tools``; loosen the assertion to accept id strings + ``ToolSectionLabel`` dicts but no other shape. 10/11 integration suite still passing; unit and tox clean. --- lib/galaxy/tools/lazy_toolbox.py | 196 ------------------- test/integration/test_tool_source_storage.py | 21 +- 2 files changed, 13 insertions(+), 204 deletions(-) diff --git a/lib/galaxy/tools/lazy_toolbox.py b/lib/galaxy/tools/lazy_toolbox.py index 1c140ba85d1..b7b7be3ae2a 100644 --- a/lib/galaxy/tools/lazy_toolbox.py +++ b/lib/galaxy/tools/lazy_toolbox.py @@ -43,7 +43,6 @@ from galaxy.tool_util.toolbox.panel import ( panel_item_types, ToolSection, ) -from galaxy.tool_util.toolbox.views.edam import EdamToolPanelView from . import ( create_tool_from_source, ToolBox, @@ -632,100 +631,6 @@ class LazyToolBox(ToolBox): fallback_tool_id=stored.tool_id, ) - def _populate_tool_registry_from_index(self) -> None: - """ - Populate _tools_by_id with None placeholders from index. - - This allows has_tool() checks to work without loading Tool objects. - The actual Tool objects are loaded on-demand in get_tool(). - """ - if self._tool_index is None: - return - - # Update index entries with section info from tool_conf.xml - # Build reverse map: short_id -> section_info for faster lookup - if hasattr(self, "_tool_section_map"): - short_id_to_section: dict[str, tuple] = {} - for map_tool_id, mapped_section in self._tool_section_map.items(): - # Store exact ID - short_id_to_section[map_tool_id] = mapped_section - # For guids, also store the short tool ID - short_id = extract_short_id_from_guid(map_tool_id) - if short_id and short_id != map_tool_id and short_id not in short_id_to_section: - short_id_to_section[short_id] = mapped_section - else: - short_id_to_section = {} - - section_updates = 0 - section_info: Optional[tuple] - for tool_id, entry in self._tool_index.entries.items(): - section_info = None - - # Try exact match first - if tool_id in short_id_to_section: - section_info = short_id_to_section[tool_id] - - if section_info: - section_id, section_name = section_info - if section_id and not entry.panel_section_id: - entry.panel_section_id = section_id - entry.panel_section_name = section_name - section_updates += 1 - - # Store None as placeholder - actual Tool loaded on demand - self._tools_by_id[tool_id] = None # type: ignore[assignment] - - # Initialize version tracking. Walk every version we indexed so - # ``has_tool(tool_id, exact=True)`` and version-aware lookups - # behave the same as the eager toolbox (which registers each - # ``Tool`` instance per version). - self._tool_versions_by_id.setdefault(tool_id, {}) - versions = self._tool_index.entries_by_version.get(tool_id, {entry.version or "": entry}) - for version_key in versions.keys(): - if version_key: - self._tool_versions_by_id[tool_id][version_key] = None # type: ignore[assignment] - - # Lineage is built lazily by ``LazyLineageMap`` from - # ``_tool_index.entries_by_version`` on first ``get()``; no - # boot-time seeding pass is needed. - - # Add to panel if section info available - if entry.panel_section_id and entry.panel_section_id in self._tool_panel: - section = self._tool_panel[entry.panel_section_id] - if isinstance(section, ToolSection): - self._tool_panel.record_section_for_tool_id(tool_id, entry.panel_section_id, section.name or "") - - # Debug: check for mismatches - index_ids = set(self._tool_index.entries.keys()) if self._tool_index else set() - map_ids = set(self._tool_section_map.keys()) if hasattr(self, "_tool_section_map") else set() - matched = index_ids & map_ids - log.info( - f"Section map has {len(map_ids)} entries, index has {len(index_ids)} entries, {len(matched)} matched, {section_updates} updated" - ) - if map_ids and index_ids: - # Show sample IDs from each for comparison - log.info(f" Sample index IDs: {list(index_ids)[:3]}") - log.info(f" Sample map IDs: {list(map_ids)[:3]}") - - # Mirror ``_tool_panel``'s section/label structure into - # ``_integrated_tool_panel`` so static panel views can resolve - # sections via ``closest_section`` (which searches the integrated - # panel). Tools stay deferred — the lazy panel materialises each - # section's elems on first access. Without this seed, static views - # for the test-tool sections (e.g. ``test``, ``filter``, - # ``test_section_multi``) fail to find any section and render - # empty. - for key, value in list(self._tool_panel.items()): - if key in self._integrated_tool_panel: - continue - if isinstance(value, ToolSection): - self._integrated_tool_panel[key] = ToolSection( - {"id": value.id, "name": value.name, "version": value.version or ""} - ) - else: - # Labels and other non-tool elements copy by reference. - self._integrated_tool_panel[key] = value - # === Override get_tool for lazy loading === @overload @@ -1548,104 +1453,3 @@ class LazyToolBox(ToolBox): log.debug("LazyToolBox.to_dict: returning %d tools (in_panel=False)", len(rval)) return rval - - def to_panel_view(self, trans, view="default_panel_view", **kwds) -> dict[str, dict]: - """Render a panel view's API response. - - For the default view we build the response straight from - ``_tool_index.entries`` — no Tool instantiation, no per-tool - re-parse. Going through the parent's ``tool_panel_contents`` -> - ``apply_view`` -> ``get_tool_to_dict`` path would lazy-load every - indexed tool at request time, which is exactly what defeated - startup in the prior commit (``test_job_recovery::test_recovery`` - spent ~14 minutes lazy-loading 500+ tools per restart and never - reached "ready"). - - Static panel views (configured via ``panel_views`` / - ``panel_views_dir``) are scoped to a specific small set of - ```` entries; for those we let the parent walk - ``apply_view`` so its ``ToolBoxRegistry.get_tool`` lazy-loads - only the requested few. - """ - resolved_view = view - if resolved_view == "default_panel_view": - resolved_view = self._default_panel_view(trans) - - view_def = (self._tool_panel_views or {}).get(resolved_view) if hasattr(self, "_tool_panel_views") else None - # The default panel view is whatever ``AbstractToolBox.__init__`` (or our - # legacy override) registered for the "full tool panel" slot — it - # isn't an Edam view and isn't a configured StaticToolPanelView. - # ``DefaultToolPanelView`` is a *locally-defined* class inside - # ``AbstractToolBox.__init__`` now that we call ``super().__init__()``, - # so we can't ``isinstance``-check it; negate the two known - # specialisations instead. - from galaxy.tool_util.toolbox.views.static import StaticToolPanelView - - is_default_view = view_def is not None and not isinstance(view_def, (EdamToolPanelView, StaticToolPanelView)) - if view_def is None or is_default_view: - # Default view — render from index entries cheaply. - view_contents: dict[str, dict] = {} - sections: dict[str, dict[str, Any]] = {} - uncategorized: list[dict[str, Any]] = [] - if self._tool_index is None: - return {} - include_hidden = bool(kwds.get("include_hidden", False)) - for entry in self._tool_index.entries.values(): - if entry.hidden and not include_hidden: - continue - tool_dict = self._index_entry_to_api_dict(entry) - section_id = entry.panel_section_id - if section_id: - section = sections.get(section_id) - if section is None: - # Mirror ``ToolSection.to_dict(only_ids=True)`` which - # the eager toolbox calls for the default view: a - # ``"tools"`` key with the list of tool ids (not - # ``"elems"`` with full dicts). Tests like - # ``test_tools::test_index`` walk - # ``tool_or_section["tools"]`` to flatten sections; - # an ``"elems"`` payload makes ``upload1`` (a - # sectioned tool) invisible to the flatten loop. - section = { - "id": section_id, - "name": entry.panel_section_name or section_id, - "model_class": "ToolSection", - "tools": [], - } - sections[section_id] = section - section["tools"].append(tool_dict["id"]) - else: - uncategorized.append(tool_dict) - for section_id, section_dict in sections.items(): - view_contents[section_id] = section_dict - for tool_dict in uncategorized: - view_contents[tool_dict["id"]] = tool_dict - return view_contents - - # Non-default view (EDAM, static): defer to the parent. With the - # eager pipeline filling ``_integrated_tool_panel`` with ``LazyTool`` - # stubs, EDAM's ``walk_loaded_tools`` reads ``tool.edam_operations`` - # / ``edam_topics`` from the entry surface (no parse) and - # ``_load_tool_panel_views`` (run by ``super().__init__``) has - # already cached the rendered panel in ``_tool_panel_view_rendered``. - return super().to_panel_view(trans, view=view, **kwds) - - def _index_entry_to_api_dict(self, entry: ToolIndexEntry) -> dict[str, Any]: - """Convert an index entry to the format expected by /api/tools.""" - return { - "id": entry.id, - "name": entry.name, - "version": entry.version, - "description": entry.description, - "labels": entry.labels if entry.labels else [], - "edam_operations": entry.edam_operations if entry.edam_operations else [], - "edam_topics": entry.edam_topics if entry.edam_topics else [], - "hidden": entry.hidden, - "model_class": "Tool", - "panel_section_id": entry.panel_section_id, - "panel_section_name": entry.panel_section_name, - # Minimal fields that indicate this is from index - "link": f"/api/tools/{entry.id}", - "min_width": -1, - "target": "galaxy_main", - } diff --git a/test/integration/test_tool_source_storage.py b/test/integration/test_tool_source_storage.py index 278a266835d..5da43268d2d 100644 --- a/test/integration/test_tool_source_storage.py +++ b/test/integration/test_tool_source_storage.py @@ -242,11 +242,13 @@ class TestLazyToolBoxApi(BaseToolSourceStorageIntegrationTestCase): # --- Default-panel response shape --------------------------------------- def test_default_panel_view_section_tools_use_id_list(self): - # Pins the section-shape fix that ``ToolSection.to_dict(only_ids=True)`` - # emits: each section dict has a ``tools`` key holding a list of - # tool-id strings (not full Tool dicts under ``elems``). Regression - # for ``test_tools::test_index`` which walks ``tool_or_section["tools"]`` - # to flatten sections; a different shape makes upload1 invisible. + # Pins ``ToolSection.to_dict(only_ids=True)``: each section dict has a + # ``tools`` key holding tool-id strings (interleaved with + # ``ToolSectionLabel`` dicts where the conf places labels). The + # regression this guards against is *Tool* dicts appearing in + # ``tools`` — that breaks ``test_tools::test_index``, which walks + # ``tool_or_section["tools"]`` to flatten sections and would + # otherwise miss sectioned tools like ``upload1``. response = self._get("tool_panels/default") self._assert_status_code_is(response, 200) panel = response.json() @@ -256,9 +258,12 @@ class TestLazyToolBoxApi(BaseToolSourceStorageIntegrationTestCase): if isinstance(entry, dict) and entry.get("model_class") == "ToolSection": sections_seen += 1 assert "tools" in entry, f"section {entry_id} missing 'tools' key" - assert all( - isinstance(t, str) for t in entry["tools"] - ), f"section {entry_id} should hold tool ids as strings, got {entry['tools'][:3]}" + for item in entry["tools"]: + if isinstance(item, str): + continue + assert ( + isinstance(item, dict) and item.get("model_class") == "ToolSectionLabel" + ), f"section {entry_id} should hold tool-id strings or ToolSectionLabel dicts, got {item!r}" # Sanity: the framework conf has at least one section, otherwise the # assertion above never ran. assert sections_seen > 0, "expected at least one section in default panel view"