-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Dynamic tool fixes #18085
Dynamic tool fixes #18085
Conversation
Missing return statement makes API always return null
There is no way of setting active and hidden from posted payload
lib/galaxy/managers/tools.py
Outdated
@@ -110,6 +110,8 @@ def create_tool(self, trans, tool_payload, allow_load=True): | |||
tool_path=tool_path, | |||
tool_directory=tool_directory, | |||
uuid=uuid, | |||
active=tool_payload.get("active", True), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would there be a use case when we want to specify False
for either one of these? Their default values are True
. And if we do, we probably don't want to specify the default value here and include it only if it has a value, otherwise rely on the database to supply the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I do not get the point of your question? Do you mean it's redundant to default the get() to True? If we only do get("active") then it will return None I suppose. Is the following DAO layer able to translate None to the default value then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, out of curiosity ... I've been digging into this dynamic tool part because I need to dynamically generate tools to display in the tool panel according to the logged user (and only for the session), transforming data fetched from an external service and without rebooting the galaxy server.
Is this a use case meant for these dynamic tools? Are there any ways of doing this with the current implementation of Galaxy? Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to further explain ... I was hoping that by setting hidden to false the tools would have appeared in the tool panel but I've noticed that it probably does not work like that...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, so far they're only meant (and tested) for running jobs and workflows. You could probably implement a selector that looks at /api/dyanmic_tools
to list these, but note that this functionality is also restricted to admins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any ref to the code for selectors? Maybe I could write one such selector to populate the tool panel with ephemeral tools generated on the fly for the logged user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about that, you could probably stuff this in as a custom panel view in
galaxy/lib/galaxy/tool_util/toolbox/base.py
Lines 1334 to 1351 in 33011b2
def to_panel_view(self, trans, view="default_panel_view", **kwds): | |
""" | |
Create a panel view representation of the toolbox. | |
Uses the structure: | |
{section_id: { section but with .tools=List[all tool ids] }, ...}} | |
""" | |
if view == "default_panel_view": | |
view = self._default_panel_view | |
view_contents: Dict[str, Dict] = {} | |
panel_elts = self.tool_panel_contents(trans, view=view, **kwds) | |
for elt in panel_elts: | |
# Only use cache for objects that are Tools. | |
if hasattr(elt, "tool_type"): | |
view_contents[elt.id] = self.get_tool_to_dict(trans, elt, tool_help=False) | |
else: | |
kwargs = dict(trans=trans, link_details=True, tool_help=False, toolbox=self, only_ids=True) | |
view_contents[elt.id] = elt.to_dict(**kwargs) | |
return view_contents |
__dynamic_tools__
. Then you'd have to serialize the dynamic tools in a structure like
{
"__dynamic_tools__": {
"model_class": "ToolSection",
"id": "__dynamic_tools__",
"name": "Dynamic Tools",
"version": "",
"description": null,
"links": null,
"tools": [
"$dynamic_tool_id_1"
]
}
}
That'll make these tools selectable here:
Before embarking on that I'd also check that the tool form is rendered correctly for dynamic tools (at https://galaxy_url/?tool_id=$tool_id).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I do not get the point of your question? Do you mean it's redundant to default the get() to True? If we only do get("active") then it will return None I suppose. Is the following DAO layer able to translate None to the default value then?
Sorry, I made it confusing! So yes, it is redundant to specify True
as the default value here since it is already specified in the database. The potential problem is that if at some point we change the default, we'll have to remember to change it in both places, otherwise that would introduce a bug. My suggestion was to include these arguments in the call to self.create
only if they have values, otherwise let the database handle the default. However, it's even simpler than that - include them regardless, just don't set the default value in get
: it returns None
by default, in which case the database will use true
, by default, when inserting the row.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done .... pipelines restarting ...
Remove redundant defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the fix, @dcore94!
A missing return statement made the dynamic_tool API show endpoint always return null.
There is no way of passing hidden/active settings for dynamic tools through the API. Thay always resolve to the default settings which could only be changed directly on the DB.
How to test the changes?
(Select all options that apply)
curl --location 'http://localhost:8080/api/dynamic_tools/{tool_id}'
License