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

Jump to bottom

[22.05] Reload built-in converters on toolbox reload #16948

Closed
wants to merge 3 commits into from
Closed

[22.05] Reload built-in converters on toolbox reload #16948

wants to merge 3 commits into from

Conversation

bernt-matthias
Copy link
Contributor

Hopefully fixes #16947

Maybe load_builtin_converters should be included in the load_lib_tools or Toolbox init function?

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.

@github-actions github-actions bot added this to the 23.2 milestone Oct 31, 2023
@bernt-matthias bernt-matthias changed the base branch from dev to release_22.05 October 31, 2023 11:30
@bernt-matthias bernt-matthias changed the title Reload built-in converters on toolbox reload [22.05] Reload built-in converters on toolbox reload Oct 31, 2023
nsoranzo
nsoranzo reviewed Nov 9, 2023
View reviewed changes
lib/galaxy/queue_worker.py Outdated Show resolved Hide resolved
@bernt-matthias bernt-matthias force-pushed the topic/converters_reload_22.05 branch from a7c7ede to f447811 Compare November 10, 2023 12:14
nsoranzo
nsoranzo reviewed Nov 10, 2023
View reviewed changes
lib/galaxy/tool_util/toolbox/base.py Outdated Show resolved Hide resolved
lib/galaxy/tool_util/toolbox/base.py Outdated
@@ -24,6 +24,7 @@
NullDependencyManager,
)
from galaxy.tool_util.loader_directory import looks_like_a_tool
from galaxy.tools.special_tools import load_lib_tools
Copy link
Member

Choose a reason for hiding this comment

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

You cannot import stuff from galaxy.tools in galaxy.tool_util, otherwise the package hierarchy breaks. Can you please move lib/galaxy/tools/special_tools.py to lib/galaxy/tool_util/special_tools.py and update the import?

Maybe the load_lib_tools changes should go to dev and keep here only the load_builtin_converters part?

@bernt-matthias bernt-matthias force-pushed the topic/converters_reload_22.05 branch from f447811 to 095e9c1 Compare November 10, 2023 13:47
@bernt-matthias bernt-matthias force-pushed the topic/converters_reload_22.05 branch from f439a48 to 8958492 Compare November 10, 2023 15:29
@nsoranzo
Copy link
Member

The unit test failures are real unfortunately, we may need to initialize display_builtin_converters to False for some tests, or make more specific assertions.

@bernt-matthias
Copy link
Contributor Author

I think it's better to fix tests. Have I met what you meant by "make more specific assertions"?

@bernt-matthias
fix tests:
1fad559 
- extend mock app config by display_builtin_converters
@bernt-matthias bernt-matthias force-pushed the topic/converters_reload_22.05 branch from 479df28 to 1fad559 Compare November 20, 2023 18:05
@bernt-matthias
Copy link
Contributor Author

Hi @nsoranzo .. how should we continue here? Anything I can/should do here?

@nsoranzo
Copy link
Member

With all the (probably unrelated) test failures, it is a bit difficult to find out if there is anything that we should worry about. Can you maybe compare with recent 22.05 test runs https://github.com/galaxyproject/galaxy/actions?query=branch%3Arelease_22.05 ?

@mvdbeek
Copy link
Member

mvdbeek commented Dec 12, 2023

Or target a newer branch ? I don't feel comfortable with these changes targeting a stable release

@bernt-matthias
Copy link
Contributor Author

Or target a newer branch ?

Since I'm already running this on my production instance I would also be happy with 23.2 / dev.

But more generally: Wouldn't it be better to provide the converters as an extra tool config (or commented section in the tool config sample) file that admins could load if desired? It would also solve #17076. Only downside is that we need to keep this file up-to-date .. but converters are only updated seldom.

@mvdbeek
Copy link
Member

mvdbeek commented Dec 12, 2023

Converters are loaded via the datatypes config, let's not mess with that.

@bernt-matthias
Copy link
Contributor Author

Superseded by #17209

@bernt-matthias bernt-matthias deleted the topic/converters_reload_22.05 branch December 19, 2023 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nsoranzo nsoranzo nsoranzo approved these changes

@natefoo natefoo natefoo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
23.2
Development

Successfully merging this pull request may close these issues.

Built-in converters disappear on toolbox reload
4 participants
@bernt-matthias @nsoranzo @mvdbeek @natefoo