Skip to content
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.

Sign up for GitHub

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

Syntax for conditionally loading toolbox items. #18004

Closed
wants to merge 1 commit into from
Closed

Syntax for conditionally loading toolbox items. #18004

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions lib/galaxy/tool_util/toolbox/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,12 @@ def load_item(
panel_dict = self._tool_panel
if integrated_panel_dict is None:
integrated_panel_dict = self._integrated_tool_panel
load_if = item.get("if")
if load_if == "interactivetools_enable":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to duplicate in the tool conf the info provided by tool_type="interactive" in the tool, not sure I like it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get why you feel that way but I disagree. I feel like the toolbox says this is what should be loaded and it should be an error if you are attempting to load something that isn't configured properly. It only feels like duplicated information because we aren't serious enough about tool errors - we just ignore stuff and keep going with loading the tool panel.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you would suggest Galaxy should not start if a tool doesn't load correctly, so "just ignore stuff and keep going with loading the tool panel" seems the right approach to me.
We could present the tool load failure errors in the admin UI instead of keeping them in the logs, but that's another discussion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is what we do for every other configuration error - it is what an application should do if it is misconfigured IMO. I'm closing this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but these are user plugins .. say ITs are on the tool shed one day (nothing is actually stopping a "rogue" actor from uploading ITs). If an admin installs a tool and restarts the server it won't restart. That is a heavy price to pay for forcing that the application config is correct.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're never going to load all user defined tools into a toolbox like this and these are admin configured tools. The file I updated in this PR is literally hand crafted.

if not self.app.config.interactivetools_enable:
raise ValueError("Trying to load an InteractiveTool, but InteractiveTools are not enabled.")
elif load_if:
raise ValueError(f"Unknown conditional tool load condition '{load_if}'")
if item_type == "tool":
self._load_tool_tag_set(
item,
Expand Down
4 changes: 2 additions & 2 deletions test/functional/tools/sample_tool_conf.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,8 +237,8 @@
<tool file="multiple_versions_changes_v01.xml" />
<tool file="multiple_versions_changes_v02.xml" />

<tool file="interactivetool_simple.xml" />
<tool file="interactivetool_two_entry_points.xml" />
<tool file="interactivetool_simple.xml" if="interactivetools_enable" />
<tool file="interactivetool_two_entry_points.xml" if="interactivetools_enable" />
<tool file="converter_target_datatype.xml" />

<!-- Tools interesting only for building up test workflows. -->
Expand Down
Loading