Closed drosofff closed 7 years ago
I am not sure the miniconda role is required anymore. At least in this PR, it does not seem to be required.
It is still required. When you startup galaxy on an exported volume you can easily run into problems with file locking because we start multiple galaxy processes at the same time, and each one will attempt to install conda. So we can introduce a single startup using run.sh, but the more transparent way to do this is to use this role. We should update the version that is going to be installed, galaxy is installing 4.2.13, and we should probably also use the python3 variant (which is also the galaxy default).
Importantly it fixes the nasty bug I mentioned to @mvdbeek at gcc2017 with the new msp_sr_bowtie wrapper (skipping need of a python wrapper)
Did you figure out what the problem is? In theory this change should not affect whether or not a wrapper works.
Did you figure out what the problem is? In theory this change should not affect whether or not a wrapper works.
The wrappers works indeed (as can been seen in https://travis-ci.org/ARTbio/tools-artbio/jobs/249435176). But the tool does not install correctly on Galaxy instances that implement conda 3.16 (GKS) or 3.19.0 (Plastisipi). This is the issue of conda envs paths truncated. I changed conda to 4.2.13 in a deployed GKS instance and it fixed the problem (I was able to precisely track the problem in the conda bin environments). I'll do it also for plastisipi that is currently in 17_05 testing, but I wait for end of jobs currently running on that instance.
I tested this PR successfully, but there was no exported volume that is right.
Before doing that, I have looked to the simplest approach consisting in updating the miniconda role. I switched to my idea of letting Galaxy doing the job because I did not find the 4.2.13 version to be downloaded from https://repo.continuum.io/miniconda/. It's not a super good reason I agree, but I like the idea to let galaxy doing its stuff. If it's possible to find a turn around (conda pip install ? or installing 4.2.15 ?), I am fine. On the other hand, one can argue that installation of conda by Galaxy could be robust enough to cop with multiple process trying to install conda at the same time ?
It's not a super good reason I agree, but I like the idea to let galaxy doing its stuff.
Sorry, this only works well when using the run.sh method, because we don't really have a safe way to create a lock (needed to prevent parallel installations getting into each other's way) in galaxy. That is an area of active work, but we're not there yet. So one option is running run.sh, but if you specify 4.2.13 and python3 in the all
file there is no need to look into galaxy's source code (https://github.com/galaxyproject/galaxy/blob/dev/lib/galaxy/tools/deps/conda_util.py#L32) to know what is getting installed, and that is the idea of declarative configuration management.
So one option is running run.sh, but if you specify 4.2.13 and python3 in the all file there is no need to look into galaxy's source code to know what is getting installed, and that is the idea of declarative configuration management.
All right ! If it is that easy, it's a non-issue, we could have started by this. I'll test it.
This is more or less what I expected because 4.2.13 is not available in repo.continuum.io :
TASK [miniconda-role : miniconda installer is downloaded] **********************
[WARNING]: Using world-readable permissions for temporary files Ansible needs to create when becoming an unprivileged user. This may be
insecure. For information on securing this, see https://docs.ansible.com/ansible/become.html#becoming-an-unprivileged-user
fatal: [35.187.167.242]: FAILED! => {"changed": false, "dest": "/tmp/Miniconda3-4.2.13-Linux-x86_64.sh", "failed": true, "msg": "Request failed", "response": "HTTP Error 404: Not Found", "state": "absent", "status_code": 404, "url": "https://repo.continuum.io/miniconda/Miniconda3-4.2.13-Linux-x86_64.sh"}
It's 4.2.12, my bad.
Yep, currently testing
I am not sure the miniconda role is required anymore. At least in this PR, it does not seem to be required. Importantly it fixes the nasty bug I mentioned to @mvdbeek at gcc2017 with the new msp_sr_bowtie wrapper (skipping need of a python wrapper)
I would remove the commented lines and put back trackster install to True if Merge PR