-
Notifications
You must be signed in to change notification settings - Fork 161
Dockerized tests in order to run in Ubuntu 16.04 (required by tf >= 1.5) #143
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
Dockerized tests in order to run in Ubuntu 16.04 (required by tf >= 1.5) #143
Conversation
1603dc2 to
b1d8a57
Compare
e5ae076 to
e0e9c7b
Compare
|
Travis is under maintenance, I will check again later. |
|
|
||
| jdk: oraclejdk8 | ||
|
|
||
| sudo: required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we still need sudo at this point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I believe we do. It makes Travis to run inside VM instead of a container. I thought you needed in order to run docker.
I can double check what happens if I run with sudo required = False
thunterdb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few changes for understanding better what is happening.
At a higher level, I am concern by the complexity of the whole thing, because we have all these layers now:
- our code + spark
- conda to control the python env
- docker to control the conda env
- travis to control the tests
The README should be update to reflect that the recommended (supported) way to build and test stuff is using the provided conda environment, and we should all default to that.
Also, do we need docker? Is this just for travis? I hope we can get rid of that as soon as possible, this adds an unwelcome layer of indirection.
|
|
||
| cache: | ||
| directories: | ||
| - $HOME/.ivy2/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking at the logs, the maven deps don't seem cached anymore
| export CONDA_URL="repo.continuum.io/miniconda/Miniconda3-latest-Linux-x86_64.sh"; | ||
| export PYSPARK_PYTHON=python3; | ||
| fi | ||
| - docker run -e "JAVA_HOME=/usr/lib/jvm/java-8-openjdk-amd64" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what you mean? The command starts a docker container. It might not be the best way, I am not that familiar with the docker. I am open to suggestions.
| - docker exec -t ubuntu-test bash -c "apt-get update && apt-get upgrade -y" | ||
| - docker exec -t ubuntu-test bash -c "apt-get install -y curl bzip2 openjdk-8-jdk" | ||
| # download and set up miniconda | ||
| - docker exec -t ubuntu-test bash -c " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we move these scripts into ./bin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but they were in the same place before. I only moved the execution inside docker.
| # Run the python unit tests. | ||
| - sbt -Dspark.version=$SPARK_VERSION -Dscala.version=$SCALA_BINARY_VERSION tfs_testing/assembly | ||
| - SPARK_HOME=$HOME/.cache/spark-versions/$SPARK_BUILD ./python/run-tests.sh | ||
| - docker exec -t ubuntu-test bash -c " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same thing here
|
Thanks for the review @tjhunter! I am not sure I understand your concerns. To clarify the situation:
I chose the docker path cause it seemed to be the least possible change required to get the tests running again. The docker code can definitely be simplified. I did it this way partly cause I am not a master of docker and partly because this closely mimics the previous state - i.e. all the commands are there in the same order, the only difference is we now send them to the docker container. Personally, I don't see a big problem with running tests inside Docker. It gives us greater control of the environment and I believe it is quite common for people to do that, including tests run inside Travis. We can spend some time to figure out how to do it properly (e.g. get the maven cache working again) but in the short term, at least this way we can run the tests successfully. But I am really open to suggestions. Feel free to scrap this PR or make changes to it if you have a better solution. |
|
@tomasatdatabricks for clarification, it is great that our tests are running again, and the cost of some extra complexity with docker is certainly worth this. This looks good for me, but can you add the instructions in the README, like in this section: |
|
Feel free to merge after that, it looks good as far as I am concerned. |
|
Ok, will do. Thanks Tim! |
bad5f7b to
01f2d8d
Compare
01f2d8d to
8f2716a
Compare
|
@tomasatdatabricks I am merging. Thanks for the work! |
Run tests in docker environment with Ubuntu 16.04.