Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added dockerfile [biohackathon] #10681

Merged
merged 6 commits into from
Jan 12, 2021
Merged

Conversation

nuwang
Copy link
Member

@nuwang nuwang commented Nov 10, 2020

This PR adds a dockerfile for Galaxy that allows a minimal container to be built from the current directory using
docker build . -t galaxy/galaxy-k8s:latest. The resultant image is suitable for running tests, for Kubernetes or extending with additional functionality.

Methods of customizing the resultant image are documented here: https://github.com/galaxyproject/galaxy-docker-k8s/ and the dockerfile included herewith has been moved from the galaxy-docker-k8s repo.

Closes: galaxyproject/galaxy-docker-k8s#4

This PR requires:
galaxyproject/ansible-galaxy#117
galaxyproject/galaxy-docker-k8s#18

@nuwang nuwang changed the title Added dockerfile Added dockerfile [biohackathon] Nov 10, 2020
@galaxybot galaxybot added this to the 21.01 milestone Nov 10, 2020
Dockerfile Outdated
RUN rm -rf \
.ci \
.git \
.venv/bin/node \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need node for expression tools.

Copy link
Member Author

@nuwang nuwang Nov 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... That probably means that expression tools don't work in k8s at the moment.
Which of these folders do you think we can safely zap? client/node_modules is probably ok I'm guessing?
Ping @ic4f and @afgane

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nuwang Sorry, I missed this. Yes, I think it is safe to drop client/node_modules - that's what we did back in 2019.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think expression tools are mapped to another container in k8s ... we did fix this last year for the covid workflows.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this work needs that extra K8S context for expression tools I've renamed the Dockerfile to ".k8s_ci.Dockerfile" instead of ".ci.Dockerfile" as I suggested elsewhere.

Dockerfile Outdated
@@ -0,0 +1,112 @@
# Stage 1:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should live in the root directory ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where were you thinking? Currently, docker build . works as expected from the root directory.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rename it to say... .ci.Dockerfile? Then you can just pass in the -f argument with docker build I think.

Copy link
Member

@mvdbeek mvdbeek Jan 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add to that, dockerhub let you configure the location of the Dockerfile for automatic builds as well.

@jmchilton
Copy link
Member

I don't think this belongs here unless @bgruening tells me otherwise - there is a different community supported docker container with a large user base, long history of solid user support, and that has proven useful tool for both development tasks and production tasks. I put a lot of work into making that container work with in injected external Galaxy codebase - that is why I thumbs up'ed the linked issue - it was a cool and useful feature and it was capable of doing that without having to reside in this directory.

@nuwang
Copy link
Member Author

nuwang commented Nov 13, 2020

I think that ideally, a Dockerfile should live in the root directory of Galaxy, which would be useful for testing branches and other basic purposes. Perhaps the docker-galaxy-stable image could be that Dockerfile, although my intuition is that it's not, since its use-case is more comprehensive that merely building a basic Galaxy container. If @bgruening thinks there's a better path, am happy to defer. Ideally, we would also put in some work to make the docker-stable image and minimal image converge (possibly extend one off the other), and although we had a few conversations on this, I think we need to make a more concerted effort to make it happen.

@bgruening
Copy link
Member

I stopped arguing about this, sorry. If people think they need to redo everything please do. The problem with the expression tools and the node has been discussed and implemented in oher containers last year already.

Ideally, we would also put in some work to make the docker-stable image and minimal image converge (possibly extend one off the other), and although we had a few conversations on this, I think we need to make a more concerted effort to make it happen.

Agreed! :)

@nuwang
Copy link
Member Author

nuwang commented Nov 16, 2020

I've made a PR that attempts to unify these two strands of work: bgruening/docker-galaxy-stable#573
Hopefully, this will enable us to coordinate better and row in the same direction.

@mvdbeek
Copy link
Member

mvdbeek commented Jan 7, 2021

Seems there is some unresolved discussion here, I'll push it to 21.05. I still think a root level Dockerfile here is maybe the wrong message, but if this is somewhere in a folder with a Readme clarifying what this is and what its intended use is I think we can merge this.

@mvdbeek mvdbeek modified the milestones: 21.01, 21.05 Jan 7, 2021
@almahmoud
Copy link
Member

almahmoud commented Jan 11, 2021

Regarding

I stopped arguing about this, sorry. If people think they need to redo everything please do. The problem with the expression tools and the node has been discussed and implemented in oher containers last year already.

First, on a personal note, I want to point out that "If people think they need to redo everything please do" is particularly irritating/annoying/demoralizing and I do not see any way in which it can be interpreted to be constructive. I understand it is probably the culmination of a lot of previous conflict, but are we really at the point where our team communicates by taking passive-aggressive stabs at each other on GitHub? We're all supposed to be on the same team, but for the past 2 years I feel like I can't say Docker and Galaxy in the same sentence without creating problems left and right, despite not even understanding why/where this conflict started, and it's quite frankly exhausting. On a more substantive note, I don't know of anyone who wants to do extra work when we're all already overworked trying to meet deadlines and different project/grant goals, and, while I realize this might be my lack of context and not knowing all the historical arguments, two things that come to mind are 1) I genuinely do not know of what existing working alternative is supposedly being ignored (any time I've tried the galaxy-docker-stable image with Kubernetes, it either didn't come up with various errors or came up after many tweaks and took a long time to start/restart, although I admit it's been many months since I tried last) and 2) the minimal image is essentially a wrapper around the ansible galaxy playbook (which I've heard so many people say is what the community is aiming to keep maintaining), so the idea that it's "redoing everything" when it's a decently simple Dockerfile using that playbook is kind of ludicrous at least from my limited perspective. Anyway, if there are better alternatives that we're blatantly missing, I honestly 100% volunteer to never think or touch a Docker image ever again if someone else wants to take that on. From my perspective, if we want Galaxy on Kubernetes to keep working, we need a Docker image that is as small as possible, that starts up in a matter of seconds, and that works in a cluster. Beyond that, I personally do not care where that image comes from, and I do not think other people are "attached" to any solution either (although I can only speak for myself obviously), we just need something that works reliably and that requires at most little tweaks when new versions come out, assuming we don't want to drop all of our grants/projects that need Kubernetes (which tbh if we wanted to do, I'm also 100% all for it, especially if it's always an annoying afterthought for everyone and there's only 4-5 of us that actually care about it working). Anyway, I digress... My point is just that I, and I'm sure others, don't want to be in the crossfire of old arguments anymore, and would like us to make a team decision on how to move forward and stop having this kind of passive-aggressive conflict that just exacerbates the problem.

Seems there is some unresolved discussion here

On a more general note, there obviously seems to be a lot of unresolved discussion but I am not sure when it started or what it's really about, and would like to propose everyone put their historical differences and arguments aside so that we actually have a new conversation about this at some point soon, cause as far as I know we have two working groups waiting on this to go somewhere... (and that somewhere could be just removing the Dockerfile altogether if that's what people want... we can very easily build the image on every commit with a hook that is outside of the Galaxy repo. The push to add it in the Galaxy repo was to consolidate the effort and try to use Dockerhub's auto-build feature for Galaxy repo, but that's not a requisite for what we need to do. This approach was simply more convenient and when we had the conversation with the testing-hardening group for Q1 priorities, everyone seemed to agree that auto-build being better than an action hook for this)...

I personally don't know how much arguing has happened that I don't know of, or who has beef with whom, but as far as my memory can remember, I've never heard any real arguments or discussion on this topic, just passive aggressiveness, and everyone walking on egg shells to not step on others' toes but never actually understanding who is offended by what and what needs to be done for everyone to be happy, and none of it seems constructive or healthy for the team... personally, I'd like more context as to what these arguments are even about, and to actually understand who is pushing/pulling towards what goal, and I specifically mean towards something not just against something else.
In that sense, can I propose a reset of all previous conflicts/arguments/conversations and having a new constructive conversation from scratch with everyone involved where we decide what is best for the team and the projects we should all be aiming towards, to minimize the in-fighting and have better team dynamics? I know we might not all see eye-to-eye on everything but we can at least understand what people are pushing for and why, and not just know there are opposing camps without knowing who's in those camps or why they even exist... having to walk on eggshells any time Galaxy and Docker are in the same sentence is exhausting, especially when there is so little context to what these arguments are even about...

That's my naive annoyed two cents ¯\_(ツ)_/¯

P.S.: I explicitly want to say that I have absolutely no desire to lead this conversation, cause that sounds like an exhausting task, but do genuinely think we're not going to get anywhere unless we have a real conversation about this

@mvdbeek
Copy link
Member

mvdbeek commented Jan 12, 2021

I'm sorry for being a bit vague with my last comment. In the context of preparing the 21.05 release we are pushing all PRs that aren't ready yet or that don't necessarily need to land in a given release. If they do become ready before the branch date we will of course include them. What I meant with

Seems there is some unresolved discussion here

is that the node and .dockerignore comment should be addressed (or dismissed, that is certainly an option).
We do want the image in core Galaxy, as we discussed in our last meeting. @jmchilton is going to take a look at these things today.
The remaining comments here pre-date that discussion.

bgruening/docker-galaxy-stable#573 seems like a good initiative (which was the suggestion in #10681 (comment)) from where I'm standing.

@jmchilton jmchilton removed the triage label Jan 12, 2021
@jmchilton jmchilton merged commit 0e3a55f into galaxyproject:dev Jan 12, 2021
@mvdbeek
Copy link
Member

mvdbeek commented Jan 12, 2021

Should we enable builds on dockerhub now ? If yes, what should the image name be ?

@galaxyproject galaxyproject deleted a comment from github-actions bot Jan 12, 2021
@almahmoud
Copy link
Member

Just putting my thoughts out there because I was never directly part of those conversations in the past that I can remember, although I have no energy to fight about the name and it doesn't affect anything functionally if this is a non-starter for anyone, so I'm personally fine with any reasonable name: my opinion is that it should just be galaxy/galaxy (since it is meant to be a basic container that can run galaxy code) especially if it's used as the base image for galaxy-docker-stable (which I personally think should be named galaxy-standalone potentially, given that, from my understanding, it is meant to be a standalone container running not just Galaxy but also its dependent services like postgres?, although i understand if renaming that is off the table for consistency and I obviously have 0 say in it so idk just throwing that idea out there). I think we've used galaxy/galaxy-k8s for the longest as of now and tried galaxy/galaxy-min[imal] (primarily motivated by the fact that the image is not only used for k8s but for things like testing in https://github.com/galaxyproject/usegalaxy-tools for example) until now, but to my understanding both galaxy-minimal and galaxy were criticized, so we've stuck with -k8s I think?

Thanks for getting this merged, we can probably discuss naming and enable DockerHub when we meet tomorrow to talk about the whole Docker-Galaxy problem in general? I don't understand all the implications/problems around it so that's just my naive reasoning above, but ultimately it can be whatever is best for everyone since it's purely aesthetic not blocking any other work

@nuwang
Copy link
Member Author

nuwang commented Jan 13, 2021

I think galaxy-min would make sense since it would match the name used in PR: bgruening/docker-galaxy-stable#573 but no strong preference.

afgane added a commit that referenced this pull request Feb 5, 2021
Nuwan Goonasekera (@nuwang) has been an active member of the Galaxy community for nearly a decade. He has made numerous and significant contributions across the spectrum of Galaxy Project repositories: [bioblend](https://github.com/galaxyproject/bioblend/commits?author=nuwang), [ansible-galaxy](https://github.com/galaxyproject/ansible-galaxy/commits?author=nuwang), [galaxy-helm]https://github.com/galaxyproject/galaxy-helm/commits?author=nuwang), and others. 

He actively helps users and other team members by answering questions via chat and issues on GitHuh, providing constructive feedback and dedicating time to follow up, eg. https://gitter.im/galaxyproject/wg-deployment?at=601ae0a655359c58bf1c8ff7, https://gitter.im/galaxyproject/FederatedGalaxy?at=6019abed428d9727dd4ebacb, CloudVE/cloudbridge#258. 

He has made a number of improvements to the Galaxy codebase demonstrating his grasp and commitment to the core project:
- https://github.com/galaxyproject/galaxy/pulls?page=2&q=is%3Apr+author%3Anuwang
The following pull requests exemplify his contributions over the years:
- #10681
- #9738
- #1814

I propose we add Nuwan to the Galaxy committers group to both recognize his contributions and benefit more directly from his expertise. 

@galaxyproject/core please vote.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to build an image within the Galaxy source tree
8 participants