Merge chado fix #17 - Githubissues

nathandunn commented 7 years ago

This should integrate the important pieces from #17.

[ ] add pg_isready for chado, as well
[ ] copying ROOT.war is unnecessary each time, but should be done after redoing build
[ ] don't remove or install elsewhere useful binaries (data prep, loading, etc.)
[ ] remove postgresql launch if non-local?
[ ] fork apollo-only to another repository? (PITA, but better for users?)

===

[x] evaluate feedback here: https://github.com/erasche/docker-apollo/blob/master/launch.sh#L10
[x] fix loading bug
[x] confirm de novo installlation
[x] confirm that chado is not re-run
[x] merge into apollo-only relevant changes

nathandunn commented 7 years ago

I meant should fix #17.

nathandunn commented 7 years ago

FYI @fikipollo and @erasche. Please comment when you have a moment. I am currently testing. I tried to error on the side of more configurability.

The nice thing about this that we don't reload chado each time unless its not there (I think).

nathandunn commented 7 years ago

I haven't finished testing. Wanted to get general feedback first. I just know on the image that is there, that the chado stuff was hard-coded AND the chado script was run every time, both of which were undesirable.

I'll make sure it works on subsequent reloads and novel images before merging.

hexylena commented 7 years ago

Ah, I assumed this was "pre-merge feedback" and was quite strict in my comments. Had I known this was general feedback, I would have made different comments.

Yep, that is definitely undesirable. The unified images gets much less testing from me.

nathandunn commented 7 years ago

No worries. All comments appreciated.

nathandunn commented 7 years ago

o:merge_chado_fix_10
[ ok ] Starting PostgreSQL 9.4 database server: main.
/var/run/postgresql:5432 - accepting connections
Postgres is up, loading chado
Apollo database not found, creating...
Password: 
Session terminated, terminating shell... ...terminated.
Password: 
psql: fe_sendauth: no password supplied
Password:

nathandunn commented 7 years ago

@erasche / @fikipollo I think it is working correctly now. Please test as you have time. It is up on quay:

quay.io/gmod/docker-apollo:merge_chado_fix_10

I'll merge this into the apollo-only version after I merge this in.

fikipollo commented 7 years ago

Hi @nathandunn and @erasche

Sorry I was travelling and I couldn't reply your messages but it was a really interesting discussion!

@nathandunn ok, I will test it, looks great!

nathandunn commented 7 years ago

Thanks.

Feel free to push changes to the PR.

I’ve only tested the default values.

Nathan

On Jul 3, 2017, at 10:43 AM, Rafa Hernández notifications@github.com wrote:

Hi @nathandunn https://github.com/nathandunn and @erasche https://github.com/erasche Sorry I was travelling and I couldn't reply your messages but it was a really interesting discussion!

@nathandunn https://github.com/nathandunn ok, I will test it, looks great!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/GMOD/docker-apollo/pull/18#issuecomment-312585875, or mute the thread https://github.com/notifications/unsubscribe-auth/AAt2quZluCIxgB7ZTuwtuBKoRHZ5z_lkks5sKKmxgaJpZM4OJTSl.

nathandunn commented 7 years ago

@fikipollo I just pushed some documentation changes to suggest how to persist database volumes locally.

fikipollo commented 7 years ago

Hi @nathandunn

it looks great! I'm going to test it but looking at the files it should work. Some comments that may help.

In launch.sh we use service postgresql start but if we are using an external postgres service, Do we want to launch the local service? It is not a big problem, but it would consume resources.

For pg_isready just say that if we are waiting for postgres for Webapollo but not for Chado, again it's not a big deal to add a line for checking if Chado's postgres service is running, makes sense for me.

Finally I don't understand why do we need to copy the WAR file in the webapps dir everytime we launch the docker, Would it not be enough replacing the ROOT.war file once, right after calling to build.sh in the Dockerfile?

Aditionally, I found that, although removing the apollo sources after building will reduce the size of the image, we are loosing some binaries that could useful for preparing the data for apollo (prepare refseq for example). Maybe the idea is that the user should input the data already prepared but it would also possible to prepare the data in a interactive terminal in the container (e.g. using docker exec.../bin/bash). Maybe you could keep at least the binaries of apollo?

Now I leave for holidays so I won't answer messages for a while! Many thanks for the docker and all the job, we are looking forward to use it in our lab! :smile:

nathandunn commented 7 years ago

@fikipollo This is excellent feedback on all counts. I've been creating a separate branch called apollo-only to mirror what @erasche has been doing. However, maybe I should just create a separate repository. However, your points all still stand.

I'll update the todo-list to integrate / retest these points.

nathandunn commented 7 years ago

FYI TODO-list: https://github.com/GMOD/docker-apollo/pull/18#issue-239478363

hexylena commented 7 years ago

copying ROOT.war is unnecessary each time, but should be done after redoing build

So, here's my thoughts on this.

If you do this, then you'd have to modify the logic for context_path, you'd have to unpack the folder to a different location. That's fine, not a big change (just an mv of a folder at boot-time)
But it makes the layer bigger. Why send this uncompressed directory structure, when the zipped WAR file could be sent instead and given that decompression takes a miniscule amount of time at runtime compared to the rest of the boot process for apollo, it's pretty equivalent.

don't remove or install elsewhere useful binaries (data prep, loading, etc.)

I'd argue that people should be building their jbrowse instances outside of the apollo server container, that seems like it is mixing functionality. If you want to build the instances inside of a container, just use the jbrowse image, instead of an image dedicated to running an Apollo instance. Single purpose containers are more ideal.

However, I can understand the idea of "oh, I want to make a non-reproducible apollo instance using docker exec statements, and just build-up the jbrowse inside of the container". So maybe there's an argument for re-installing the tools. I've made a fork of my images that supports this (docker pull quay.io/erasche/apollo:unified-alpine-tools)

In launch.sh we use service postgresql start but if we are using an external postgres service, Do we want to launch the local service? It is not a big problem, but it would consume resources. remove postgresql launch if non-local?

Or, do like I do, and just add in the postgres server in the unified image, and leave it out in the apollo-only image. Thanks to docker, it was like two lines to do this! (It could be similarly easy for the ubuntu based images) No conditional logic required.

fork apollo-only to another repository? (PITA, but better for users?)

Please do not do this. Just do branches like I do instead of "another repository". Another repo is worse for users / developers trying to keep things in sync.

(@nathandunn if you wanted, you could save yourself the development time and just merge in everything in erasche/apollo since I've already written most of these features.

Alternatively, to save even more time and effort / greater de-duplication of efforts, we could make the gmod/docker-apollo repo more of a community project (like the registry), and I'd be happy to make all of my regular changes here, rather than in my fork. I've already put a lot of work into creating my images, they're tiny and very nice for end-users to start playing with. That might give you more time to focus on Apollo features.)

nathandunn commented 7 years ago

@erasche Much appreciated thoughts on this.

1 - yes, gmod/docker-apollo should be community (I moved you from "write" to admin) 2 - so, branches are fine, so long as we document how to get stuff (definitely less work for me)

3 - there are two goals with this repo (for me):

a (master) - A naive user can come in and just start using Apollo with minimal setup. This is why I like having the tool remained installed within the image, this way a user doesn't have to install random perl pre-requisites on their local system, they are just there.

b (apollo-only) - A more advanced user can integrate it into their docker environment easily without breaking anything or bringing in a lot of additional cruft.

I'm fine bringing your changes into the gmod/docker-apollo as a different PR (and closing this one) so long as they solve both (a) and (b) above.

hexylena commented 7 years ago

@nathandunn

1 - yes, gmod/docker-apollo should be community (I moved you from "write" to admin)

Ah, ok, super. I'll work on this tonight. I didn't realise I even had write access.

2 - so, branches are fine, so long as we document how to get stuff (definitely less work for me)

With quay setup like it is, it's very easy to just say docker pull quay.io/gmod/docker-apollo or docker pull quay.io/gmod/docker-apollo:minimal. Would you be interested in renaming the quay repo to be just apollo instead of docker-apollo? Shorter / less redundant.

a (master) - A naive user can come in and just start using Apollo with minimal setup. This is why I like having the tool remained installed within the image, this way a user doesn't have to install random perl pre-requisites on their local system, they are just there.

b (apollo-only) - A more advanced user can integrate it into their docker environment easily without breaking anything or bringing in a lot of additional cruft.

I'm fine bringing your changes into the gmod/docker-apollo as a different PR (and closing this one) so long as they solve both (a) and (b) above.

yes, totally accomplish-able! I tend to consider admins first, so my master was the minimal image, but I'm very happy to switch this around, especially because I'd like to move towards more branches for more use cases, and I'd expect people deploy tags in preference to :laster / :master. I'd love to implement branches for a matrix of Apollo version x (unified + tools / unified - tools / minimal).

I'll open PRs tonight.

nathandunn commented 7 years ago

Sounds awesome. "Minimal" is fine as well.

Yeah, so long as we document the branches on master I think that is fine.

I'll close this whenever you create the other PR's.

Thanks for doing that.

GMOD / docker-apollo

Merge chado fix #17 #18