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

modernize containers #1780

Merged
merged 1 commit into from
Jul 11, 2024
Merged

modernize containers #1780

merged 1 commit into from
Jul 11, 2024

Conversation

acxz
Copy link
Contributor

@acxz acxz commented Jul 9, 2024

use compose spec, use single image repo with tags for various configs, create a docker hub push script, clean up/slim down container files

resolves #1779

Running this PR on my personal docker hub account: https://hub.docker.com/r/acxz/gtsam/tags

@ProfFan ProfFan requested a review from dellaert July 9, 2024 13:43
Copy link
Collaborator

@ProfFan ProfFan left a comment

Choose a reason for hiding this comment

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

I 2nd this, thanks Akash!

Copy link
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

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

Awesome! But, I have never heard of podman - I'd prefer if the docs were written with docker (and tested to make sure the podman flags were not podman specific).

@acxz
Copy link
Contributor Author

acxz commented Jul 10, 2024

Glad you all like the PR!

Docker does have a huge mindshare as they started the whole container revolution, but I have personally found podman easier to install and more applicable due to rootless daemonless capability (although I imagine Docker may have fixed that security vulnerability by now).

Nonetheless I agree with you and have removed mentions of alternative container tools, but kept general phrasing such as container engine and compose spec implementation for dependencies.

@acxz
Copy link
Contributor Author

acxz commented Jul 10, 2024

I imagine getting the containers on Docker Hub will need to be done by someone with access to the BORG Lab organization. The image repositories this PR replaces can/should be removed.

Some repositories might need to change their base images, such as gtdynamics (github link). There is also the manylinux image, that may need to be changed.

I also just noticed a copy of the container related stuff in our docker-images repo. Does that repo need to be cleaned up with the submission of this PR? (at the very least remove the duplicate images gtsam and ubuntu-boost-tbb and update corresponding base images.)

@acxz
Copy link
Contributor Author

acxz commented Jul 10, 2024

One more thing regarding future work: I can imagine setting up a github action or other github-fu to build and push a container image to docker hub whenever a new gtsam git release is made. Outside the scope of this PR, but just a thought to prevent stale images. Github might have something that makes this dead simple.

use compose spec, use single image repo with tags for various configs, create a docker hub push script, clean up/slim down container files
Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

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

Thanks! This looks pretty great. I can update the docker images on Dockerhub once this is approved.

General comments: I am of the opinion that container files should be in their own repository. That way we decouple container update and support from the actual code repository. I do this with much success in https://github.com/varunagrawal/docker-images where I have containers beyond just GTSAM.

@varunagrawal
Copy link
Collaborator

varunagrawal commented Jul 10, 2024

I imagine getting the containers on Docker Hub will need to be done by someone with access to the BORG Lab organization. The image repositories this PR replaces can/should be removed.

I'll do that after we merge this.

Some repositories might need to change their base images, such as gtdynamics (github link). There is also the manylinux image, that may need to be changed.

Yeah this is going to be a headache. :)

I also just noticed a copy of the container related stuff in our docker-images repo. Does that repo need to be cleaned up with the submission of this PR? (at the very least remove the duplicate images gtsam and ubuntu-boost-tbb and update corresponding base images.)

Should have made my comment here. I strongly believe that the docker-images repo should be the container repo. That way we can add Github Actions for easy deployment and manage the connection between that repo and Dockerhub in a clean way, without adding more complexity to the GTSAM repo. @acxz if you have the bandwidth, I'd love to do this so we can update all the images we support.

Building containers takes a LOOONG time and that just makes CI on this repo take more time which makes rapid iteration difficult, especially since we don't have a commercial Github license here.
attn: @dellaert @ProfFan

@acxz
Copy link
Contributor Author

acxz commented Jul 11, 2024

Should have made my comment here. I strongly believe that the docker-images repo should be the container repo. That way we can add Github Actions for easy deployment and manage the connection between that repo and Dockerhub in a clean way, without adding more complexity to the GTSAM repo. @acxz if you have the bandwidth, I'd love to do this so we can update all the images we support.

Having the containers as part of a separate repo from the main GTSAM library makes sense to me as well. I'd just suggest a name change to the repo to be container-images or borglab-containers instead of `docker-images.

I do believe that the action you are proposing be held off until this PR is merged. In my opinion, this PR should be merged first and the newer images be pushed to Docker Hub. After that, if the core GTSAM team decides so, the docker//containers/ folder here should be created as a pull request to the current docker-images folder. Once that is merged in, the existing docker/containers folder would be deleted from here and all further borglab container development could happen in that repo.

I don't necessarily want to hold off on getting this work published to Docker Hub because of organizational movement (even though I agree with such movement), especially considering the fact that I may not have the bandwidth to create a proper PR to the docker-images repo as there would be the gtdynamics and the manylinux configurations I would have to worry about, which I am not working on directly.

@varunagrawal
Copy link
Collaborator

@acxz I like that plan and I agree with it. I think I'm good to merge this PR.

@varunagrawal varunagrawal merged commit cc2e8de into borglab:develop Jul 11, 2024
31 checks passed
@acxz acxz deleted the containers branch July 11, 2024 23:33
@dellaert
Copy link
Member

dellaert commented Sep 5, 2024

I'm looking at this again now, and I'm now question the wisdom of not having a standalone image like ubuntu-boost-tbb was, and which is still mentioned on docker hub, despite someone having removed the actual image? It would be nice to be able to create a VS code devcontainer on an image that you know has all the pre-requisites to locally build develop. So:

  • can we re-instate ubuntu-boost-tbb ?
  • how can we split the new Dockerfile to reflect the base image and the"fully-built" images (cake and eat it too)

@cntaylor
Copy link
Contributor

cntaylor commented Sep 5, 2024

Is this helpful?

In this repo I have a multi-stage Docker file that first creates a stage with all of the libraries installed to make GTSAM. From there it branches to a "doc", "debug", and "release" Docker container, allowing you to locally serve up docs, or run GTSAM in debug or release.

I think this answers your second question about building a Dockerfile that both builds the base image, but allows you to take advantage of it if it is already there. Feel free to use it if helpful.

@varunagrawal
Copy link
Collaborator

The docker-images repo still has the Ubuntu base image. I didn't realize this PR removed that image because I wanted to use it in the CI to cut down on build time.

The good news is that docker-images not only has the images but is connected to dockerhub so the images are automatically built and ready to download. I configured our OSS support from Docker.

@varunagrawal
Copy link
Collaborator

@cntaylor that's really helpful! Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modernize GTSAM containers
5 participants