Skip to content

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.

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