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

Docker update #172

Merged
merged 10 commits into from
Jul 20, 2020
Merged

Docker update #172

merged 10 commits into from
Jul 20, 2020

Conversation

spficklin
Copy link
Member

@spficklin spficklin commented Jul 10, 2020

This PR updates the Docker file to support the RScripts and the 3D python viewer so that users who want to use the Docker image to avoid installing all of the dependencies can do so.

This PR makes the following changes:

  • In the Makefile:
    • the lastest images now have ACE hardcoded to 3.2.0 rather than develop. This is because the develop branch can have, at any point in time, experimental code. I think the latest image should be the latest of development version of KINC and not the latest of of ACE.
    • I changed the NVIDIA_HEADLESS option to be consistent between the cpu and gpu tagged images.
    • I added a new KINC_R_REVISION argument.
  • In the Dockerfile
    • I updated section to behave differently for newer version of KINC. For example in 3.4.2 we have a much simplified make procedure that if not used does not install the scripts in the bin folder.
    • I added installation of R, KINC.R
  • In the Documentation
    • I adjust the text to reflect the new additions for the docker images. However, I'm not 100% sure when to tell the user to use the -cpu and -gpu tagged images. That should be included as the current image name isn't correct in the commands.
    • I changed the docker command from nvidia-docker to just docker with the --gpus all option. I wasn't able to run the image with nvidia-docker as that didn't exist on my machine.

Unfortunately, all of these changes makes the image rather large and take a long time to build.

@spficklin spficklin requested a review from bentsherman July 10, 2020 05:52
@spficklin
Copy link
Member Author

spficklin commented Jul 10, 2020

@bentsherman would you mind looking this over? If you don't have time for a functional review if you can just check the changes and see if you are okay with them?

The only outstanding item on this is clarification about the -cpu and -gpu tags on the images which is missing in the documentation.

@spficklin
Copy link
Member Author

Also, once this PR is merged and after we do the 3.4.2 release we can rebuild the images for DockerHub. For the 3.4.2 release we just have this PR and the other #171 . Then we can release!

@spficklin
Copy link
Member Author

Oh, one other note.... If you try to build the images you'll see some warnings when building KINC about unused variables. That has been fixed in #171 so no need to try to clean that up. It will go away once all the PRs are merged.

Copy link
Member

@bentsherman bentsherman left a comment

Choose a reason for hiding this comment

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

I left my review via inline comments. Just a few things that need to be changed. The docker image will likely be a lot bigger because of the additional R dependencies, but hopefully that will all be removed once the R scripts are migrated to Python. A few extra notes:

  • Make sure that you can actually use the KINC GUI from the docker image. I know you can run web servers from a docker image but I have never tried native GUI applications. If KINC GUI doesn't work then you should leave the GUI=no option in the build and specify that in the documentation.
  • The CPU builds are for CPU-only machines, and it installs the NVIDIA headless drivers instead of the actual drivers so that KINC loads correctly. You may recall users in the past reporting issues with running KINC without a GPU, and this is the workaround that I developed for that.

docker/Dockerfile Show resolved Hide resolved
docker/Dockerfile Show resolved Hide resolved
docker/Makefile Outdated Show resolved Hide resolved
docker/Makefile Outdated Show resolved Hide resolved
docker/Makefile Outdated Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
@spficklin
Copy link
Member Author

Hi @bentsherman. I believe I have resolved all of the issues you raised on this PR. I did update the Dockerfile a bit more to make it easier to build just a cpu or gpu version. Also I added a new develop tag and set latest to point to the actual latest release rather than the develop branch. Other than that, the only other changes I made were to fix issues.

Can you review again? I'm hoping to do a release on Monday for Iris.

@spficklin
Copy link
Member Author

Thanks @bentsherman

@spficklin spficklin merged commit 0b12c3b into develop Jul 20, 2020
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.

2 participants