Skip to content

Remove deprecated tool document cache #20510

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

nsoranzo
Copy link
Member

Close #20432 .

Also:

  • Improve type annotations of tool-related code
  • Small refactorings
  • Drop dynamic tool from toolbox._tools_by_uuid dict when deactivated (see commit for details)

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

nsoranzo added 3 commits June 18, 2025 16:26
Fix the following API test failure:

```
______________________ TestToolsApi.test_tool_deactivate _______________________

self = <galaxy_test.api.test_tools.TestToolsApi object at 0x7f4808a67b50>

    def test_tool_deactivate(self):
        # Create tool.
        tool_response = self.dataset_populator.create_tool(MINIMAL_TOOL_NO_ID)
        self._assert_has_keys(tool_response, "id", "uuid", "active")
        assert tool_response["active"]
        deactivate_response = self.dataset_populator.deactivate_dynamic_tool(tool_response["uuid"])
        assert not deactivate_response["active"]

        # Run tool.
        history_id = self.dataset_populator.new_history()
        response = self._run(history_id=history_id, tool_uuid=tool_response["uuid"], assert_ok=False)
        # Get a 404 when trying to run a deactivated tool.
>       self._assert_status_code_is(response, 404)
...
E       AssertionError: Request status code (200) was not expected value 404.
```

caused by this addition in the previous commit in
`lib/galaxy/tool_util/toolbox/base.py::AbstractToolBox.get_tool()`:

```python
+            tool_uuid = tool_uuid if isinstance(tool_uuid, UUID) else UUID(tool_uuid)
             tool_from_uuid = self._get_tool_by_uuid(tool_uuid)
```

Before this change, `tool_uuid` was passed to `self._get_tool_by_uuid()`
as a string instead of being transformed into a UUID. This meant that
in:

```python
    def _get_tool_by_uuid(self, tool_uuid: UUID) -> Union["Tool", None]:
        if tool_uuid in self._tools_by_uuid:
            return self._tools_by_uuid[tool_uuid]

        dynamic_tool = self.app.dynamic_tool_manager.get_tool_by_uuid(tool_uuid)
        if dynamic_tool:
            return self.load_dynamic_tool(dynamic_tool)

        return None
```

the first `if` would always fail (because the `_tools_by_uuid` dict
keys are UUIDs), the dynamic tool would then be loaded from
the database again, and the `active` attribute would be correctly
checked in `load_dynamic_tool()`.

After the mentioned change, the `tool_uuid` is instead correctly
found in the `_tools_by_uuid` dict, but the `active` attribute is
never checked.
Therefore the fix is to remove a dynamic tool from `_tools_by_uuid`
when it is deactivated.
@nsoranzo nsoranzo added kind/enhancement kind/refactoring cleanup or refactoring of existing code, no functional changes labels Jun 18, 2025
@github-actions github-actions bot added this to the 25.1 milestone Jun 18, 2025
@nsoranzo
Copy link
Member Author

Failing tests unrelated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate and remove ToolDocumentCache
1 participant