Make pip_smoke_test.sh more robust against false positives (#801)

This makes `pip_smoke_test.sh` robust against two types of false positives that I realized it might produce:

1.  Testing for the post-pip-package-installation presence of a tensorboard binary with `which tensorboard` could falsely return true if tensorboard was anywhere on the path (e.g. installed to the system outside the virtualenv), meaning it didn't actually guarantee that the tensorboard command used later in the script would be the one from the pip package.

2. Testing for whether URLs are accessible at `localhost:6006` (by default) could falsely succeed if there was another pre-existing tensorboard instance running locally on port 6006 (which is common for a developer machine since that's the default tensorboard port), since the accesses don't really have a way to check that they're talking to the freshly launched tensorboard.  Note that the newly launched tensorboard actually dies on startup in this case (since it can't bind to the requested port) but it's hidden since it's been backgrounded.

This fixes the first one by checking directly for a `tensorboard` binary inside the virtualenv's `bin` directory, and fixes the second one by ensuring that we wait for the `tensorboard` binary to start up successfully and print its serving URL before proceeding with the test.  That also means we can extract the serving URL automatically, which makes it possible to set the default port to 0 (aka requesting an unused local port from the kernel) which should eliminate the risk of interference in the default case.

Author: nfelt

tensorboard/pip_package/pip_smoke_test.sh

@@ -30,7 +30,7 @@ die() {
 }
 
 PY_VERSION=2
-TEST_PORT=6006
+TEST_PORT=0
 NUM_RETRIES=20
 while [[ "$#" -gt 0 ]]; do
   if [[ "$1" == "--python3" ]]; then
@@ -89,7 +89,8 @@ echo "Activating virtualenv at ${VENV_TMP_DIR}"
 echo
 
 export VIRTUAL_ENV="${VENV_TMP_DIR}"
-export PATH="${VENV_TMP_DIR}/bin:${PATH}"
+export VENV_BIN_DIR="${VENV_TMP_DIR}/bin"
+export PATH="${VENV_BIN_DIR}:${PATH}"
 unset PYTHON_HOME
 
 echo
@@ -122,17 +123,42 @@ elif [[ "${PY_VERSION}" == 3 ]]; then
 fi
 
 # Check tensorboard binary path.
-TB_BIN_PATH=$(which tensorboard)
-if [[ -z ${TB_BIN_PATH} ]]; then
-  die "ERROR: Cannot find tensorboard binary path after installing tensorboard pip package."
+TB_BIN_PATH="${VENV_BIN_DIR}/tensorboard"
+if ! [[ -x "${TB_BIN_PATH}" ]]; then
+  die "ERROR: No tensorboard binary found after installing tensorboard pip package."
 fi
 
+# Start TensorBoard running in the background
+TMP_LOG_OUTPUT=${PIP_TMP_DIR}/output.log
 TMP_LOGDIR=$(mktemp -d --suffix _tensorboard_logdir)
-tensorboard --port="${TEST_PORT}" --logdir="${TMP_LOGDIR}" &
+tensorboard --host=localhost --port="${TEST_PORT}" --logdir="${TMP_LOGDIR}" \
+  >${TMP_LOG_OUTPUT} 2>&1 &
 TB_PID=$!
 
 echo
-echo "tensorboard binary should be running at pid ${TB_PID}"
+echo "waiting for tensorboard binary to start up..."
+echo
+
+# Wait until the binary has printed its serving URL so we know that it's
+# accessible and which port it's running on.
+while true; do
+  if ! ps -p $TB_PID >/dev/null 2>&1; then
+    echo
+    echo "TensorBoard exited unexpected, printing logs:"
+    echo "============================================="
+    cat ${TMP_LOG_OUTPUT}
+    echo "============================================="
+    exit 1
+  fi
+  TB_URL=$(grep -o -m 1 -E 'http://localhost:[0-9]+' ${TMP_LOG_OUTPUT} || true)
+  if [[ -n "${TB_URL}" ]]; then
+    break
+  fi
+  sleep 1
+done
+
+echo
+echo "tensorboard binary (pid ${TB_PID}) running at ${TB_URL}"
 echo
 
 test_access_url() {
@@ -174,14 +200,15 @@ test_access_url() {
 }
 
 TEST_URL_FAILED=0
-test_access_url "http://localhost:${TEST_PORT}/data/logdir" || TEST_URL_FAILED=1
-test_access_url "http://localhost:${TEST_PORT}" || TEST_URL_FAILED=1
+test_access_url "${TB_URL}/data/logdir" || TEST_URL_FAILED=1
+test_access_url "${TB_URL}" || TEST_URL_FAILED=1
 
 echo
 echo "Terminating tensorboard binary at pid ${TB_PID}"
 echo
 
 kill -9 "${TB_PID}"
+wait "${TB_PID}" 2>/dev/null || true  # Wait to suppress "Killed" message.
 
 echo
 if [[ "${TEST_URL_FAILED}" == 0 ]]; then