Update setup script for OS X #279

ghost · 2018-08-13T05:29:35Z

The script has been updated based on setup_linux.sh, using homebrew to
install Linux packages and updating the requirements for mujoco-py, gym
and baselines according to their documentation for OS X.
Since mujoco-py is installed within the setup scripts for Linux and OS
X, the script to setup mujoco has been removed.
Also, the tensorflow package was added to environment.yml so it can be
installed out of the box without the scripts. The feature to install the
GPU flavor of TensorFlow may be removed from the Linux script once
everything could be installed using environment.yml only, or another
single list of dependencies.
Finally, the default for set-envvar and the correct replacement of
string in print error and warning functions have been added to the
script to setup linux.

ghost · 2018-08-13T05:51:57Z

Reviewers, I don't have access to a laptop with OS X to test setup_osx.sh, however I checked that the functions and dependencies in the script are valid for OS X.
Namely, I checked that all the Linux functions in the script are available in Mac based on this list. Also I considered the OS X requirements for mujoco-py, gym (gym issue) and baselines.
One of the requirements to build GLFW is Xcode as mentioned here, however I'm not sure about how to install this dependency. Are the xcode command line tools sufficient?
Finally, besides your useful comments, I would appreciate if those of you using OS X could test the script to make sure everything works fine.
Thanks in advance.

CatherineSue · 2018-08-13T16:44:00Z

scripts/setup_osx.sh

-cd "$(dirname "$0")/.."
+print_help ()
+{
+  printf '%s\n' "Installer of garage for Linux."


Should this be Installer of garage for OS X?

Yes, I will change it. Thanks Chang.

ghost · 2018-08-13T16:52:44Z

I've noticed that issue #235 considers the use of Travis to test this script, so I will try that option.

ghost · 2018-08-13T17:19:33Z

On the other hand, using setup_osx in Travis may have the same problem as with setup_linux described in issue #222.

ryanjulian · 2018-08-13T17:50:59Z

Please test on 1-2 real OS X machines.

Check for the OS X versions you tested with in the script. If the user's version is different, warn the user that it might not work. Ask them to file a PR updating the list of known-good OSX versions (or a PR with fixes for their version) and include a link to our Github Issues page.

XCode command line tools is a standard requirement for OSX dev machines. Brew requires it, anyway.
We won't be making an OSX-based docker container so Setup Docker container using setup_linux.sh #222 is not a problem. Please try testing it with Travis if they have a reasonably-current array of OSX versions. Set it up as a separate build job and don't test it in the main CI build (it can be a cron job instead).

ryanjulian

Please let me know when this is tested and we can go ahead and commit it.

jonashen · 2018-08-13T17:56:10Z

scripts/setup_osx.sh

+  "The path ${_arg_mjkey_path} of the MuJoCo key is not valid." 1
+
+# Make sure that we're under the garage directory
+GARAGE_DIR="$(readlink -f $(dirname $0)/..)"


I got this error while running the script:

readlink: illegal option -- f usage: readlink [-n] [file ...]

It seems this option is not available in OS X. Even though greadlink is available, I wouldn't like to download a new tool just for this little thing. I will find a way to work this out with more primitive tools so there's no compatibility issue.

OS X is BSD but not Linux, so you can only really rely on bash primitives and POSIX commands.

This trick has worked for me before https://stackoverflow.com/questions/3572030/bash-script-absolute-path-with-osx

jonashen · 2018-08-13T17:57:15Z

scripts/setup_osx.sh

+if [[ "${_arg_set_envvar}" = on ]]; then
+  echo "export ${PYTHON_ENV_VAR}" >> "${BASH_RC}"
+else
+  echo "Remember to export the following environment variables before" \


Can you add an extra newline before this line? Or else it looks like it's part of the pre-commit install:

pre-commit installed at /private/var/folders/kj/dgb2_y1n1wj515xphc6clvzm0000gn/T/tmp.DWwDXgPH/glfw/.git/hooks/commit-msg Remember to export the following environment variables before running garage:

It looks like you are installing pre-commit in the temporary git checkout for building glfw? THat seems not useful.

Also, you should (hopefully) not need to compile glfw for Mac. Frankly I'm surprised that worked.

The pre-commit happened there because readlink failed as pointed in a comment above, so cd does not change to the garage dir.
I thought we were compiling glfw to include the newest changes since their last release is too old. Is glfw already included in Mac or installing from brew is good enough?

jonashen · 2018-08-13T17:57:57Z

scripts/setup_osx.sh

+  echo "${PATH_ENV_VAR}"
+  echo "${PYTHON_ENV_VAR}"
+  echo "You may wish to edit your .bashrc to prepend the exports of these"
+    "environment variables."


You need an extra \ here I think:

You may wish to edit your .bashrc to prepend the exports of these ./scripts/setup_osx.sh: line 243: environment variables.: command not found

OS X uses .bash_profile not .bashrc

Ok, I will.

Some OS X users use .bashrc, and .bash_profile will default to .bashrc first. I found this inside my .bash_profile:

if [ -f ~/.bashrc ]; then . ~/.bashrc; fi

You can just say "edit your .bashrc/.bash_profile" instead

I think adding the env vars to only .bashrc does not work, unless they include the snippet of code you added above in .bash_profile. I will tell them to add them into .bash_profile to avoid the confusion.

tl;dr: use bash_profile

Your code snippet is what Apple placed in OS X to avoid compatibility problems with linux tools assuming .bashrc

Many Linux distros use .bashrc for user profiles even though they shouldn't, because many people use their machines in headless (e.g. SSH) mode.

https://apple.stackexchange.com/questions/51036/what-is-the-difference-between-bash-profile-and-bashrc

CatherineSue · 2018-08-13T17:59:05Z

scripts/setup_osx.sh

+# Set up MuJoCo
+if [[ ! -d "${HOME}"/.mujoco/mjpro150 ]]; then
+  mkdir "${HOME}"/.mujoco
+  MUJOCO_ZIP="$(mktemp --suffix=_mujoco.zip)"


I don't think OS X has --suffix option for mktemp.

$ mktemp --suffix=_mujoco.zip mktemp: illegal option -- - usage: mktemp [-d] [-q] [-t prefix] [-u] template ... mktemp [-d] [-q] [-u] -t prefix

A solution could be to use the dry-run option to only output the name of the temporary file (not create it), and append the suffix.
This is a little bit unsafe if the file happens to exist, but since these file are the tmp folder and with random characters in their name, I don't think there's a chance this will happen.

Just don't add a suffix or put it in a directory instead.

jonashen · 2018-08-13T17:59:53Z

scripts/setup_osx.sh


-echo "Conda environment created! Make sure to run \`source activate garage\`" \
+echo -e "\nGarage is installed! To make the changes take effect, work under" \
+  "a new terminal. Also, make sure to run \`source activate garage\`" \
  "whenever you open a new terminal and want to run programs under garage."


Can you divide this into two or more lines? It's too long when it's being displayed in the console.

Your console doesn't wrap it?

It didn't for me, but I find it a good habit in general. It's just a personal preference.

Sure, I can split them to a constant length.

jonashen · 2018-08-13T18:04:10Z

scripts/setup_osx.sh

+  _required_args_string="'mjkey-path'"
+  error_msg="$(echo "FATAL ERROR: Not enough positional arguments - we" \
+    "require exactly 1 (namely: $_required_args_string), but got only" \
+    "${#_positionals[@]}.")"


IIRC MuJoCo recommends moving the mjkey.txt into the .mujoco folder, so maybe we can remind users that their mjkey.txt is located for example in ./scripts/setup_osx.sh ~/.mujoco/mjkey.txt. At least for me, I had to think for a while about where I last stored my mjkey.txt after seeing this error.

I don't understand this request very well. This script asks for the path where mjkey.txt is so it can be moved into ~/.mujco/mjkey.txt as shown here.
For example, if the key is downloaded in ~/Downloads/mjkey.txt, then it's copied into ~/.mujoco/mjkey.txt.
Do you mean printing a help message or something like that?

Ignore what I said previously, I didn't see that line.

I agree with Angel that it's fine.

jonashen · 2018-08-13T18:08:46Z

scripts/setup_osx.sh

+# See https://github.com/openai/gym/issues/100
+# See https://github.com/pybox2d/pybox2d/issues/82
+pip uninstall -y Box2D Box2D-kengz box2d-py
+pip install Box2D


This should be pip install box2d-py. This is the error I get from Box2D:

(garage) garage (fix_setup_osx) $ python examples/theano/trpo_cartpole.py Traceback (most recent call last): File "examples/theano/trpo_cartpole.py", line 3, in <module> from garage.envs.box2d import CartpoleEnv File "/Users/jonathon/Documents/garage/garage/garage/envs/box2d/__init__.py", line 1, in <module> from garage.envs.box2d.box2d_env import Box2DEnv File "/Users/jonathon/Documents/garage/garage/garage/envs/box2d/box2d_env.py", line 9, in <module> from garage.envs.box2d.box2d_viewer import Box2DViewer File "/Users/jonathon/Documents/garage/garage/garage/envs/box2d/box2d_viewer.py", line 1, in <module> from Box2D import b2ContactListener File "/anaconda2/envs/garage/lib/python3.6/site-packages/Box2D/__init__.py", line 20, in <module> from .Box2D import * File "/anaconda2/envs/garage/lib/python3.6/site-packages/Box2D/Box2D.py", line 435, in <module> _Box2D.RAND_LIMIT_swigconstant(_Box2D) AttributeError: module '_Box2D' has no attribute 'RAND_LIMIT_swigconstant'

I think this produced by other installation errors mentioned above, since the solution for this issue has been posted in #250. Let me fix the pointed issues, and please try later to see if this issue vanishes.

There is a dumb bug in a recent version of swig which causes this problem. You may need to download a specific version of swig from brew or build it yourself to avoid this. You need to install the new version of swig before installing Box2D the final time. You also need to force pip to re-build the package (--no-binary or --no-wheel) on install.

If box2d-py solves the problem universally then we can just use that. IIRC I had trouble getting it to work reliably with Docker and started using the latest version from the author.

I managed to fix this with

pip uninstall Box2D Box2D-kengz box2d-py pip install Box2D-kengz --no-binary Box2D-kengz

We should update the Dockerfile an setup_linux.sh so everything is consistent.

The --no-binary forces pip to compile from source using the system swig libraries, rather than using the pre-compiled distribution from PyPI (which is apparently broken).

jonashen

I have some changes that need to be addressed

ryanjulian · 2018-08-13T18:34:09Z

scripts/setup_osx.sh

+  openmpi
+
+# Build GLFW because the brew version is too old
+GLFW_DIR="$(mktemp -d)/glfw"


Is it true that the brew version is too old? Does this even work?

I'm actually using the brew version of glfw right now and it works fine for me.

Ok, then I will remove the compilation of glfw and add it to brew.

ryanjulian · 2018-08-13T18:40:47Z

The environment libraries are most likely to break across platform. Please thoroughly test lots of gym environments, our custom mujoco envs, dm_control, and our custom box2d envs before declaring this is working.

It is also work it to run nose2 and make sure that the unit tests succeed on OS X.

ryanjulian · 2018-08-15T17:22:10Z

Can our OS X users verify this works?

ryanjulian · 2018-08-16T17:54:30Z

scripts/setup_osx.sh


-echo "Conda environment created! Make sure to run \`source activate garage\`" \
-  "whenever you open a new terminal and want to run programs under garage."
+echo -e "\nGarage is installed! To make the changes take effect, work under" \


garage is lower case

ryanjulian · 2018-08-16T20:19:48Z

scripts/setup_osx.sh

+  swig \
+  openmpi
+# Required by MuJoCo
+brew reinstall gcc@7 --without-multilib


why reinstall? and why not just install it before all the brew dependencies (which might also need to be compiled using the older GCC).

Also, unless something needs a higher version, gcc6 is more broadly supported than gcc7, which is why I picked it.

mine worked with brew install gcc@6 --without-multilib before all the other brew deps

ryanjulian · 2018-08-16T20:21:06Z

scripts/setup_osx.sh

+# See https://github.com/pybox2d/pybox2d/issues/82
+pip uninstall -y Box2D Box2D-kengz box2d-py
+PYBOX2D_DIR="$(mktemp -d)/pybox2d"
+git clone https://github.com/pybox2d/pybox2d "${PYBOX2D_DIR}"


Did pip install Box2D-kengz --no-binary Box2D-kengz not work on your system?

Checking out and installing from source should be considered a last resort. Your checkout is using master, not a release tag or hash, so it could break without warning for no reason.

I didn't try installing Box2D kengz, sorry for not noticing this one before. I will try it.

ryanjulian · 2018-08-16T20:37:25Z

scripts/setup_osx.sh

+
+# Install dependencies
+echo "Installing garage dependencies"
+echo "You will probably be asked for your sudo password"


sudo does not appear in this file

ghost · 2018-08-17T03:50:10Z

Scripts are working in fresh installations of Ubuntu (16.04) and OS X (10.12).

ryanjulian · 2018-08-17T18:50:09Z

scripts/setup_osx.sh

+# Install a virtualenv for the hooks
+pre-commit
+conda deactivate
+# Set personal configuration parameters


This actually copies the template so that the user can set their own parameters

I updated the comment.

ryanjulian

@angel were you able to verify that all the environments and graphics work using the Ubuntu VM?

ryanjulian · 2018-08-17T18:51:19Z

scripts/setup_linux.sh

@@ -215,46 +202,50 @@ if [[ "${?}" -ne 0 ]]; then
 fi

 # Extras
-source activate garage
+conda activate garage
 # Prevent pip from complaining about available upgrades
 pip install --upgrade pip
 # TensorFlow is not in environment.yml because of CPU vs GPU
 # Remove any TensorFlow installations before installing the desired flavor
 pip uninstall -y tensorflow tensorflow-gpu
 if [[ "${_arg_tf_gpu}" = on ]]; then


You only need to remove the existing TFs if you are installing tensorflow-gpu

tensorflow is only removed now.

ryanjulian · 2018-08-17T18:51:51Z

scripts/setup_linux.sh

 else
-  pip install tensorflow
+  pip install "tensorflow<1.10,>=1.9.0"
 fi
 # Fix Box2D install


Can you split each of these items into blocks with a newline? It's hard to tell which steps are related.

ghost · 2018-08-19T07:06:57Z

@ryanjulian I tested the installation on my Ubuntu 16.04 VM by running the scripts found under the script section in .travis.yml, particularly the tests under nose2. Also, I separately ran examples/theano/trpo_swimmer.py and examples/tf/trpo_cartpole.py, both with run_experiment, plot=True and parallel_sampler=10, and both worked well.

ryanjulian · 2018-08-20T18:20:25Z

scripts/setup_osx.sh

+brew update
+set +e
+brew install \
+  gcc@6 --without-multilib \


We can stick with gcc@7 to stay current. You can remove --without-multilib. It has no effect (mujoco_py error hints are outdated).

ryanjulian

LGTM

Please make the small change (gcc@7) and submit.

I have a WIP for automatically testing this in Travis here. The only change necessary in the script will be to add a quiet/unattended option which does not prompt for user input.

ghost · 2018-08-20T18:49:27Z

Okay, I will create an issue to address the quiet option.

ryanjulian

You need to update Dockerfile.ci to reflect your changes to how TensorFlow is installed.

The script has been updated based on setup_linux.sh, using homebrew to install Linux packages and updating the requirements for mujoco-py, gym and baselines according to their documentation for OS X. Since mujoco-py is installed within the setup scripts for Linux and OS X, the script to setup mujoco has been removed. Also, the tensorflow package was added to environment.yml so it can be installed out of the box without the scripts. The feature to install the GPU flavor of TensorFlow may be removed from the Linux script once everything could be installed using environment.yml only, or another single list of dependencies. Finally, the default for set-envvar and the correct replacement of string in print error and warning functions have been added to the script to setup linux.

Also, bash primitives were replaced readlink, using directories instead of suffixes with mktemp and fixing some output messages. Some of the fixes for OS X were applied to the Linux scrip as well.

The command chmod and the conda installer are no longer called as root user since it's not required and it creates user access issues in the conda folder with root. Also, pybo2d is compiled from source and gcc downgraded to version 7 in order to fix the compatibility issue. In this way, garage works right after the setup script finishes successfully.

ghost self-assigned this Aug 13, 2018

ghost requested review from eric-heiden, CatherineSue and jonashen August 13, 2018 05:29

ghost self-requested a review as a code owner August 13, 2018 05:29

ghost mentioned this pull request Aug 13, 2018

Integrate MuJoCo setup script with setup_<platform>.sh #24

Closed

CatherineSue reviewed Aug 13, 2018

View reviewed changes

ghost mentioned this pull request Aug 13, 2018

Update setup_linux.sh and setup_osx.sh #235

Closed

ryanjulian reviewed Aug 13, 2018

View reviewed changes

jonashen reviewed Aug 13, 2018

View reviewed changes

CatherineSue reviewed Aug 13, 2018

View reviewed changes

jonashen reviewed Aug 13, 2018

View reviewed changes

jonashen requested changes Aug 13, 2018

View reviewed changes

ryanjulian reviewed Aug 13, 2018

View reviewed changes

ghost force-pushed the fix_setup_osx branch from c580437 to be199f7 Compare August 14, 2018 22:40

jonashen force-pushed the fix_setup_osx branch 3 times, most recently from c142aa5 to 315dbfe Compare August 14, 2018 23:22

jonashen approved these changes Aug 15, 2018

View reviewed changes

jonashen requested a review from CatherineSue August 15, 2018 17:46

ryanjulian reviewed Aug 16, 2018

View reviewed changes

ghost force-pushed the fix_setup_osx branch 3 times, most recently from d5d71d9 to c262b2c Compare August 16, 2018 19:56

ryanjulian reviewed Aug 16, 2018

View reviewed changes

ghost force-pushed the fix_setup_osx branch 3 times, most recently from 8d9decc to 166337e Compare August 17, 2018 03:47

ryanjulian reviewed Aug 17, 2018

View reviewed changes

ghost force-pushed the fix_setup_osx branch 3 times, most recently from da1e997 to e440b62 Compare August 19, 2018 05:26

ryanjulian reviewed Aug 20, 2018

View reviewed changes

ryanjulian approved these changes Aug 20, 2018

View reviewed changes

ghost force-pushed the fix_setup_osx branch from e440b62 to 380a6f5 Compare August 20, 2018 18:38

ryanjulian requested changes Aug 20, 2018

View reviewed changes

ghost force-pushed the fix_setup_osx branch from 380a6f5 to 29289d4 Compare August 20, 2018 19:20

ryanjulian approved these changes Aug 20, 2018

View reviewed changes

Angel Gonzalez and others added 5 commits August 20, 2018 14:25

Obtain GLFW from brew and add installation message

ddf2370

Also, bash primitives were replaced readlink, using directories instead of suffixes with mktemp and fixing some output messages. Some of the fixes for OS X were applied to the Linux scrip as well.

Minor message fixes

9c7a74d

Address requested changes

f51d317

ghost force-pushed the fix_setup_osx branch from 29289d4 to f51d317 Compare August 20, 2018 21:26

ghost merged commit 079d8e5 into master Aug 20, 2018

ghost mentioned this pull request Aug 20, 2018

Install tensorflow out of the box #80

Closed

This pull request was closed.

Update setup script for OS X #279

Update setup script for OS X #279

Conversation

ghost commented Aug 13, 2018

ghost commented Aug 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Aug 13, 2018 • edited by ghost

ghost commented Aug 13, 2018

ryanjulian commented Aug 13, 2018

ryanjulian 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

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

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

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

Choose a reason for hiding this comment

ghost Aug 13, 2018 • edited by ghost

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

jonashen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Aug 13, 2018 • edited by ghost

Choose a reason for hiding this comment

ryanjulian commented Aug 13, 2018

ryanjulian commented Aug 15, 2018

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

ghost commented Aug 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanjulian 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

ghost commented Aug 19, 2018

Choose a reason for hiding this comment

ryanjulian left a comment

Choose a reason for hiding this comment

ghost commented Aug 20, 2018

ryanjulian left a comment

Choose a reason for hiding this comment

ghost commented Aug 13, 2018 •

edited by ghost

ghost Aug 13, 2018 •

edited by ghost

ghost Aug 13, 2018 •

edited by ghost