MonashBioinformaticsPlatform / LFQ-Analyst

The repo for LFQ-Analyst
https://bioinformatics.erc.monash.edu/apps/LFQ-Analyst/
GNU General Public License v3.0
28 stars 12 forks source link

Stop shiny-server starting when the container runs #19

Closed TomHarrop closed 1 year ago

TomHarrop commented 1 year ago

Hi,

Running this container on Galaxy seems to launch two shiny servers, one from the call to LFQAnalyst(), and one from the base image.

Touching /etc/services.d/shiny-server/down disables the server in the base image (documented here)

It also means we can use port 3838 as specified in the shiny base image so we no longer need EXPOSE 8888 .

This is not urgent but if you could change it next time you update lfq-analyst, we will switch back to the default port.

Thanks!

Hailey-Z commented 1 year ago

@TomHarrop Thanks Tom, I will inform you in the next update.

pansapiens commented 1 year ago

I've not investigated this myself yet, but we need to ensure this change doesn't break the production service or usual service container conventions (as well as simplifying deployment under Galaxy !). It's typical that Docker containers start the service they are designed to run by default (eg docker run haileyzhang/lfq-analyst:v1.2.6 should run a shiny app by default, probably under shiny-server).

Is it possible that the Galaxy wrapper needs to override the Docker default run command (or --entrypoint) so that it can run the app (calling LFQAnalyst()) without the default shiny-server server starting ? The base image seems to do CMD ["init"] but you should be able to override this.

If docker run haileyzhang/lfq-analyst:v1.2.6 is starting two instances of the shiny app then that is an issue that should be addressed.

TomHarrop commented 1 year ago

Hi @pansapiens , AFAIK this branch is the Galaxy-specific Dockerfile so it shouldn't affect deployment elsewhere. @Hailey-Z please correct me if I'm wrong.

You can see how we call LFQAnalyst() here: https://github.com/usegalaxy-au/tools-au/blob/dfd3073602a18d811557fa60f552b39298d30a32/tools/interactivetool_lfqanalyst/interactivetool_lfqanalyst.xml#L43-L48

I'll ping @neoformit to answer your question about two instances, because he's the person who noticed it.

Hailey-Z commented 1 year ago

Thanks, Andrew and Tom. Yes, Tom is correct, we have this separate branch run_lfq for Galaxy only, and also a specific docker image tagged as v1.2.6_galaxy for their deployment.

neoformit commented 1 year ago

The Galaxy tool needs to launch an instance of LFQ-Analyst at runtime, in order to pass the input files selected by the user. This configuration works fine for Galaxy (as far as Tom has reported) but, correct, the container would be inappropriate for regular consumers.

pansapiens commented 1 year ago

Fine to have two different builds for the moment, but ideally we should avoid having a special branch or container version if we can avoid it.

I'm not familiar with the Galaxy tool wrapper config to be able to suggest the change, but I suspect the $run_lfqanalyst script defined in the<configfile>section is running inside the container using the equivalent of docker exec, after the container has already been launched with docker run. If there is a way to override the default way Galaxy calls docker run, like docker run haileyzhang/lfq-analyst:v1.2.6 /usr/local/bin/Rscript '$run_lfqanalyst' or docker run --entrypoint=/usr/local/bin/Rscript haileyzhang/lfq-analyst:v1.2.6 '$run_lfqanalyst', AFAIK this should prevent calling the default init and result in only one R instance (and no shiny-server). There may be other examples of rocker/shiny apps wrapped for Galaxy that demonstrate this, since I expect it would be a common issue with rocker/shiny base images.

I've confirmed that there is only one instance of shiny-server running when launching container via: docker run haileyzhang/lfq-analyst:v1.2.6.

TomHarrop commented 1 year ago

We just need a way to pass input data. We're currently doing that with LFQAnalyst(). If you can add that functionality to the main container, we'll use that instead.

Hailey-Z commented 1 year ago

Thanks for pointing this out, Andrew @pansapiens. The Galaxy version has changed part of the UI. For long-term maintenance, I'll think of a way to integrate the two branches.