Consolidate GPU destinations - #1931
Conversation
Replace `condor_docker_gpu_pxe` and `condor_singularity_gpu_pxe` with a single containerized GPU destination `condor_container_gpu` that inherits from `condor_container`. In addition, make `condor_docker_gpu_pxe_divide4` inherit from `condor_container`. Simplifies the TPV configuration for GPU jobs (and fixes problems with a few edge cases), closing usegalaxy-eu/issues#928.
| scheduling: | ||
| require: | ||
| - gpu-divided |
There was a problem hiding this comment.
@mira-miracoli Will shared GPUs actually work with Singularity containers?
There was a problem hiding this comment.
Given this destination actually needs a tag and we currently only have two tools with that tag that are explicit docker I don't think it will be a problem. But agreed that we should probably check if this works with singularity
There was a problem hiding this comment.
I don't think it will work with Singularity, at least we never tested this, afaik.
There was a problem hiding this comment.
I will then add a "reject: singularity" and set "singularity_enabled: false" for this destination. It'll be interesting to see what happens when TPV finds both accept: singularity and reject: singularity 😆.
| docker_net: bridge | ||
| docker_auto_rm: true | ||
| outputs_to_working_directory: false | ||
| container_monitor_result: callback |
There was a problem hiding this comment.
Ins't this needed only for embedded pulsar?
There was a problem hiding this comment.
Most likely yes, see #719 (comment). That's there because for review I literally "dumped" the previous conf in the sense that it's as close as possible to the original. But yes, if we can fine-tune it and remove cruft we should.
There was a problem hiding this comment.
I think this is not 100% correct. container_monitor_result is used for ITs. It has nothing todo with embedded-pulsar imho. We decided for security reasons to put ITs into embedded-pulsar, but this setting is used by ITs imho.
There was a problem hiding this comment.
So the question is, do we want to run ITs in this destination. If not we don't need it.
There was a problem hiding this comment.
I think we could remove all docker_net, docker_auto_rm, outputs_to_working_directory (this one only from this PR, we can rebase #719 onto master and apply the change there instead) and container_monitor_result.
There was a problem hiding this comment.
When I posted my comments, but GitHub had not been updated with @bgruening comments 😅. I am seeing them now.
In our current configuration, interactive tools require embedded pulsar, I think it's safe to remove it (and thus use the default). This destination won't run embedded Pulsar jobs.
| docker_sudo: false | ||
| docker_net: bridge | ||
| docker_auto_rm: true | ||
| outputs_to_working_directory: false |
There was a problem hiding this comment.
Shouldn't we try to set this to true?
Configure Galaxy to run jobs in a container and enable
outputs_to_working_directory. Then the tool will execute in an environment that allows write access only for the job working dir. All other paths will be accessible read only.
If we need this setting to be false only in interactive tools I think we shouldn't enable it for all gpu tools.
There was a problem hiding this comment.
I think @bgruening agrees with you #719 (comment). I suggest removing it (see this comment) from this PR and rebasing the other, old PR onto the current status of the master branch and apply it on top of that.
There was a problem hiding this comment.
Haha, yes lets switch this if possible.
We can also merge, test that its working in general and then switch the boolean in a second PR.
There was a problem hiding this comment.
I would then switch the boolean first and then merge this. It might be easier to identify anything breaking
There was a problem hiding this comment.
I have tested now a couple tools with outputs_to_working_directory: true
Its:
- Rstudio (working)
- Jupyter Notebook (working)
Normal Docker GPU tools:
- 3dtress segmentation
- Parabriks - there is a error here, Ig we should address this in the future (made a bu report)
- DiffDock - stderr says success(?)
- LibCarna
Tbh if this is a security issue and some tools can't run with this option true, I would rather then annotate them directly on the tool.
There was a problem hiding this comment.
Tbh if this is a security issue and some tools can't run with this option true, I would rather then annotate them directly on the tool.
If we are unsure about this setting we still have #719. Imo I'd go ahead then with this as it is, we have quite a lot of breakage going around at the moment and I think we should delay this change a bit to keep the "rate of complaints" under control 😄 for the time being.
Remove `docker_sudo`, `docker_net`, `docker_auto_rm`, `outputs_to_working_directory`, `container_monitor_result`. Some of them were copied over from embedded Pulsar in the past. The defaults should be good for regular jobs that do not run in embedded Pulsar.
Sharing GPUs has not been tested with Singularity.
4 participants
Replace
condor_docker_gpu_pxeandcondor_singularity_gpu_pxewith a single containerized GPU destinationcondor_container_gputhat inherits fromcondor_container.In addition, make
condor_docker_gpu_pxe_divide4inherit fromcondor_container.Simplifies the TPV configuration for GPU jobs (and fixes problems with a few edge cases), closing https://github.com/usegalaxy-eu/issues/issues/928.