gonum / blas

A BLAS implementation for Go [DEPRECATED]
172 stars 16 forks source link

Change Travis CI to use Docker for OpenBLAS #174

Closed jonlawlor closed 8 years ago

jonlawlor commented 8 years ago

This change uses a docker image to test OpenBLAS instead of building the library during every test.

This involves more network data transfer but it results in tests that are ~1.5 minutes to run instead of 6 minutes. It should be possible to use a smaller docker image in principle - currently the docker image is based on ubuntu, and we could use alpine linux instead. However, I was unsuccessful at getting OpenBLAS to build there. It would probably be useful outside of CI testing to have a minimal docker image that can perform efficient matrix algebra in go.

I’ve also restructured the testing scripts to use an install.sh and test.sh for each of the os & bias library combinations, and a common install script for each of the OS’s.

Currently the docker image resides at hub.docker.com/jonlawlor/gonum-docker-openblas but before merging I would like to move the image into a gonum repo.

This should be immediately applicable to the lapack library as well.

jonlawlor commented 8 years ago

Hmm, it is failing on the check-generate.sh call. Hold on while I troubleshoot it...

jonlawlor commented 8 years ago

OK, fixed the check-generate issue. PTAL @btracey and @kortschak.

If you are alright with this kind of approach then I'd also like to create the github/gonum/docker-something repo for holding a dockerfile to construct a docker image with openblas already compiled, and also create the hub.docker.com/gonum/docker-something repo to hold the images. Then I'll change this branch to match.

jonlawlor commented 8 years ago

Poke @kortschak @btracey regarding this and gonum/lapack#116

kortschak commented 8 years ago

This seems OK to me. It would be nice if the PR were two commits, one for the restructure and then after that one for the docker changes, but don't bother if it's a lot of work.

I'm curious about why the diff changes were necessary.

jonlawlor commented 8 years ago

The docker environment is much easier to set up with an environment file. During testing, the install.sh script writes the travis repo slug ("gonum/blas" in this case) to the env.list file, and then tells docker to use it for additional environment variables. Because the file changed during the test, diff picks it up, and then the test fails. I figure it isn't intended to check for changes in the .travis directory.

Alternatively I could have a cleanup step that resets the env.list file, which now that I think of it is probably a better approach. Or I could filter to *.go files in the diff call, but I wasn't sure that we would only ever have .go.

It isn't too much work to make a PR with 2 commits, one for restructuring and one for docker integration. This change is only for test performance so the value add is smaller and the downside of a mistake is bigger as it could allow bad code to pass CI. Another day or two to get it right is ok.

jonlawlor commented 8 years ago

Closing in favor of #184