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.

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

Set docker_enabled and docker tag for eu tools that can only run with docker #18

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cat-bro
Copy link
Collaborator

@cat-bro cat-bro commented Apr 26, 2023

@nuwang will it matter that the abstract tool being inherited is lowest in the list?

@cat-bro cat-bro requested a review from nuwang April 26, 2023 14:51
@nuwang
Copy link
Member

nuwang commented Apr 26, 2023

It shouldn't matter but I guess it would be nice to change the formatter to send those to the top.

@@ -2293,6 +2307,13 @@ tools:
mem: 8
Extract genomic DNA 1:
cores: 3
__docker_tool:
Copy link
Member

@nuwang nuwang Apr 26, 2023

Choose a reason for hiding this comment

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

I think it may make sense to rename this to __docker_only_tool, as most tools can run with docker. Also, would this run with singularity also? If so, perhaps it should be renamed to container_only_tool?

Suggested change
__docker_tool:
__docker_only_tool:

In general, I've been wondering about the best way to support docker, especially for scenarios like these where the container hasn't worked and we've had to override it manually: https://github.com/galaxyproject/galaxy-helm/blob/697f744f1258f5efccaf52cc3be79e1177ad79fc/galaxy/values.yaml#L524

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a very good point. These tools all have only a docker type requirement in the tag but I forgot that these tools can also run with singularity, so setting require: [docker] is too restrictive, as is setting the docker_enabled param. I don't know if this PR makes sense - perhaps this logic belongs in local configuration.

Copy link
Member

@nuwang nuwang Apr 27, 2023

Choose a reason for hiding this comment

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

What if we just rename require: [docker] to require: [container]? The specific configuration for which container etc. is left to local configuration.
I think this information being in the shared database is useful, because you can't realistically expect to run these tools otherwise?

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

Successfully merging this pull request may close these issues.

2 participants