TensorFlow installer #1

Sergey-Pravdyukov · 2017-03-06T15:34:44Z

In this case used command to install TensorFlow for Python 2.7

yeputons

This review is my personal opinion, which may be one-sided, irrelevant to real goals, or blatantly wrong. I'm sorry for that. Additions for per-line comments:

This script works for apt-get-based distributions of Linux only. I'm also not sure that it will work on all versions of Debian, because Debian's stable can have some pretty old software.
It does not contain any error handling at all. E.g. apt-get errors are ignored, and user typing errors are silently ignored. I'd suggest adding special options to the script to make it fail loudly at the very least.
Bash is known (by me :) for being very obscure and dangerous in terms of handling spaces and command line arguments in general. E.g. one should not mix $@, $*, and "$@" - all of them mean different things. Google has developed its own code style for making more robust shell scripts. I personally like their recommendations and recommend following them (or some other style guide).
I personally find bash scripts very hard to scale and debug. Basically, what you want here is to install some packages with dependencies - some subset of a package manager's functionality. Maybe writing a special metapackage is a better idea, if we aim for Debian-based systems only? Not sure at all. Some examples of probable functionality which may arise in the future:

Detect already installed software.
Avoid overriding existing files.
Work under different systems without much change (e.g. use pacman instead of apt-get and slightly different package names, while still calling pip).
Install software for a single user only and provide a separate list of global dependencies from a package manager.
Add a UI (e.g. `dialog).
Dynamically adjusted defaults (e.g. default to Python2 on one system and to Python3 on a newer system).

Maybe (just maybe) switching to some better programming language before it's too late would be a wise decision. Of course, one should understand final goals first.

yeputons · 2017-03-14T17:42:35Z

installator.sh

@@ -0,0 +1,68 @@
+#!/bin/bash
+[ "$UID" -eq 0 ] || { echo "This script must be run as root."; exit 1;}


Average user may not know what is root. I suggest adding an example command in the message.

yeputons · 2017-03-14T17:43:04Z

installator.sh

@@ -0,0 +1,68 @@
+#!/bin/bash
+[ "$UID" -eq 0 ] || { echo "This script must be run as root."; exit 1;}
+echo "Determine how to install TensorFlow: "


Helpful comment like "if you don't know, choose 2" or, even better, a default option, would be nice.

yeputons · 2017-03-14T17:44:11Z

installator.sh

+echo "2 'native' pip"
+echo "3 Docker"
+read mechanismId
+case $mechanismId in


If user mistypes or accidentally presses Enter (which happens a lot), the script will silently fail. Or, even worse: it will partially install packages and silently fail then. I suggest creating a function which re-tries reading until user types in something correctly. Or, at the very least, add an error message.

yeputons · 2017-03-14T17:45:09Z

installator.sh

+read mechanismId
+case $mechanismId in
+  1) 
+    sudo apt-get install python-pip python-dev python-virtualenv 


No sudo required - we are already root. Also, what if the installation fails (e.g. no internet connection or user accidentally typed "n")? I suggest adding -u option for bash (see general review comment). I also suggest adding -y key for apt-get so it does not ask the user any questions.

yeputons · 2017-03-14T17:47:39Z

installator.sh

+case $mechanismId in
+  1) 
+    sudo apt-get install python-pip python-dev python-virtualenv 
+    targetDirectory=$1


Looks like the script expects an argument. It's not obvious. I suggest adding either a comment in the very beginning, or (better) explicit check that the argument is present and it's an existing directory. Or ask for directory name right here.

yeputons · 2017-03-14T17:50:51Z

installator.sh

+    read pythonVersion
+    case $pythonVersion in
+      1)
+        sudo -H pip uninstall tensorflow


uninstall was not present in the first branch. Why so? Maybe you wanted --reinstall?

There is no such option as --reinstall.

UPD: I was absent-minded and wrote unnecessary comand.

@Sergey-Pravdyukov there is a similar one

yeputons · 2017-03-14T17:51:14Z

installator.sh

+    esac
+    ;;
+  3)
+    echo "Docker Community Edition for Ubuntu"


Such message is missing in previous cases and it's quite unclear. E.g. verb is lacking.

yeputons · 2017-03-14T17:52:33Z

installator.sh

+    echo "Docker Community Edition for Ubuntu"
+    sudo apt-get -y install apt-transport-https ca-certificates curl
+    curl -fsSL https://download.docker.com/linux/ubuntu/gpg | sudo apt-key add -
+    sudo add-apt-repository "deb [arch=amd64] https://download.docker.com/linux/ubuntu $(lsb_release -cs) stable"


What if we want to install on a 32-bit system (e.g. some cheap VPS)?

yeputons · 2017-03-14T17:52:59Z

installator.sh

+
+    sudo apt-get -y install docker-ce
+
+    sudo docker run hello-world


Looks like this command is used for testing purposes, but user will only see "Hello World" and may be confused.

yeputons · 2017-03-14T17:54:00Z

installator.sh

+    echo "2 If you plan to run TensorFlow programs as Jupyter notebooks"
+    echo "3 If you'd like to run TensorBoard inside the container"
+    read usingMethod
+    echo "Copy/paste this URL into your browser and choose Tag Name (without 'gpu' substring in the name)"


I'd provide an example tag. Or, even better, default tag - there are four screens of tags available. How about simply taking latest-py3 as the default?

yeputons · 2017-03-14T18:11:20Z

One more idea: an unexperienced user probably has little difference from a script which just chooses the first option for all questions asked. So, your installation script can be automatically (partially) tested on different supported systems, if necessary.

Sergey-Pravdyukov added 3 commits March 6, 2017 18:26

Installer with virtualenv mechanism

194e0a3

Added Python 3.n support

3903a5d

Added Python 3.n support

02a087a

Sergey-Pravdyukov force-pushed the CPU_supported_installer branch from 3903a5d to 02a087a Compare March 7, 2017 14:08

Added 'native' pip mechanism

4436b74

Sergey-Pravdyukov changed the title ~~Installer with virtualenv mechanism~~ Installer with virtualenv and native 'pip' mechanisms Mar 12, 2017

Added Docker mechanism

b649061

Sergey-Pravdyukov changed the title ~~Installer with virtualenv and native 'pip' mechanisms~~ Installer with some mechanisms Mar 12, 2017

yeputons reviewed Mar 14, 2017

View reviewed changes

Fixed some bugs: added hints, removed sudo, refactored code etc

eef2c1e

Sergey-Pravdyukov force-pushed the CPU_supported_installer branch from 4e3fb11 to eef2c1e Compare March 19, 2017 10:33

Sergey-Pravdyukov added 2 commits March 20, 2017 14:59

Fixed tabulation

0a848a6

Added Anaconda mechanism

29674b5

Sergey-Pravdyukov changed the title ~~Installer with some mechanisms~~ C Mar 21, 2017

Sergey-Pravdyukov changed the title C CPU supported installer Mar 21, 2017

Added CUDA installation

55770c6

Sergey-Pravdyukov changed the title ~~CPU supported installer~~ TensorFlow installer Apr 20, 2017

Fixed Anaconda installation

d849f46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TensorFlow installer #1

TensorFlow installer #1

Sergey-Pravdyukov commented Mar 6, 2017

yeputons left a comment

yeputons Mar 14, 2017

yeputons Mar 14, 2017

yeputons Mar 14, 2017

yeputons Mar 14, 2017

yeputons Mar 14, 2017

yeputons Mar 14, 2017

Sergey-Pravdyukov Mar 18, 2017 •

edited

Loading

Sergey-Pravdyukov Mar 18, 2017

yeputons Mar 18, 2017

yeputons Mar 14, 2017

yeputons Mar 14, 2017

yeputons Mar 14, 2017

yeputons Mar 14, 2017

yeputons commented Mar 14, 2017

		@@ -0,0 +1,68 @@
		#!/bin/bash
		[ "$UID" -eq 0 ] \|\| { echo "This script must be run as root."; exit 1;}


		sudo apt-get -y install docker-ce

		sudo docker run hello-world

TensorFlow installer #1

Are you sure you want to change the base?

TensorFlow installer #1

Conversation

Sergey-Pravdyukov commented Mar 6, 2017

yeputons left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sergey-Pravdyukov Mar 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeputons commented Mar 14, 2017

Sergey-Pravdyukov Mar 18, 2017 •

edited

Loading