Skip to content
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

[doc] Completely revamp installation instructions #15953

Merged
merged 1 commit into from
Oct 24, 2021

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Oct 22, 2021

My goal is to improve the binary installation instructions. There are still many problems with the "build from source" instructions, but I'm deferring those for the moment.

My goal is to get correct information in place and into something like a reasonable structure. I believe we could still do more to make the information more clearly explained and better organized, but here I'm only aiming for "not harmfully wrong or incomplete".

As a reminder, for local preview:

git fetch upstream pull/15953/head && git checkout FETCH_HEAD
bazel run //doc:pages -- --serve

Closes #13202.
Closes #13210.
Closes #13500.
Closes #14493.
Closes #14920.
Closes #15747.

Relates to #13539.


This change is Reviewable

@jwnimmer-tri jwnimmer-tri added status: do not merge status: do not review release notes: none This pull request should not be mentioned in the release notes labels Oct 22, 2021
@jwnimmer-tri jwnimmer-tri marked this pull request as ready for review October 22, 2021 15:51
Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 3 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @jwnimmer-tri)

a discussion (no related file):
Working

Check for broken links


a discussion (no related file):
Working

Manually test all of the commands



doc/_pages/developers.md, line 67 at r1 (raw file):

# Supported Configurations

TBD

Working

TBD

Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @jwnimmer-tri)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

Check for broken links

Done.



doc/_pages/developers.md, line 67 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

TBD

Done.

@jwnimmer-tri
Copy link
Collaborator Author

+@BetsyMcPhail do you have some time to review this today?
+@RussTedrake if you're interested, especially in the pip and virtualenv parts.

@jwnimmer-tri
Copy link
Collaborator Author

Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 unresolved discussions, LGTM missing from assignees RussTedrake(platform),BetsyMcPhail (waiting on @BetsyMcPhail, @jwnimmer-tri, and @RussTedrake)

a discussion (no related file):
Working

I think this nukes the bad instructions that #14920 is complaining about. See if we can confirm.


@jwnimmer-tri
Copy link
Collaborator Author


doc/_pages/pip.md, line 15 at r2 (raw file):

[Installation and Quickstart](/installation.html).

Drake binary releases incorporate a pre-compiled version of

Working

The pip builds do not have SNOPT yet. I'll need to comment out this section for now.

Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, LGTM missing from assignees RussTedrake(platform),BetsyMcPhail (waiting on @BetsyMcPhail, @jwnimmer-tri, and @RussTedrake)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

I think this nukes the bad instructions that #14920 is complaining about. See if we can confirm.

Done.



doc/_pages/pip.md, line 15 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

The pip builds do not have SNOPT yet. I'll need to comment out this section for now.

Done.

Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, LGTM missing from assignees RussTedrake(platform),BetsyMcPhail (waiting on @BetsyMcPhail, @jwnimmer-tri, and @RussTedrake)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

Manually test all of the commands

Done

  • pip ubuntu
  • apt ubuntu
  • targz ubuntu
  • dockerhub ubuntu

a discussion (no related file):
Working

The updated "tar.gz" instructions for python on macOS need manual testing.

http://127.0.0.1:8000/from_binary.html#use-as-a-python-library


Copy link
Contributor

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 11 files at r1, 39 of 39 files at r2, 2 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees RussTedrake(platform),BetsyMcPhail (waiting on @BetsyMcPhail, @jwnimmer-tri, and @RussTedrake)

Copy link
Contributor

@RussTedrake RussTedrake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this! It looks much better. Lots of small comments below.

Reviewed all commit messages.
Reviewable status: 9 unresolved discussions, LGTM missing from assignees RussTedrake(platform),BetsyMcPhail (waiting on @BetsyMcPhail and @jwnimmer-tri)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

The updated "tar.gz" instructions for python on macOS need manual testing.

http://127.0.0.1:8000/from_binary.html#use-as-a-python-library

tested. minor comments below.



doc/_pages/from_binary.md, line 51 at r4 (raw file):

```bash
curl -o drake.tar.gz https://github.com/RobotLocomotion/drake/releases/download/...

this just grabs the 600+kb website download website for me. wget https://github.com/RobotLocomotion/drake/releases/download/v0.35.0/drake-20211021-mac.tar.gz on the other hand actually downloads the file.


doc/_pages/from_binary.md, line 66 at r4 (raw file):

```bash
sudo env/share/drake/setup/install_prereqs

remove the sudo here. we explicitly do not run as sudo on mac.


doc/_pages/installation.md, line 73 at r4 (raw file):

All other packages support both C++ and/or Python.

|                       | Ubuntu | macOS |

per my question in the pip section, i'm not clear whether we support pip on focal. if no, then it seems that we need three columns here? if yes, then a note somewhere about how pip on focal uses 3.6/3.7 despite us stating that we only support 3.8 on focal just above?


doc/_pages/installation.md, line 76 at r4 (raw file):

|-----------------------|--------|-------|
| Using pip             | [Stable](/pip.html#stable-releases) | |
| Using apt (deb)       | [Stable](/apt.html#stable-releases) | |

Should we mention #12782 here somewhere? It's useful for folks to know it's potentially coming, and perhaps given them a place to comment / up-vote.


doc/_pages/pip.md, line 29 at r4 (raw file):

<div class="warning" markdown="1">
Drake's support for pip has only just recently been introduced (as of October
2021), so might contain bugs. Refer to

"might contain bugs" sounded a little harsh to me. perhaps "still has some known issues [cite]" is enough?


doc/_pages/pip.md, line 39 at r4 (raw file):

<div class="warning" markdown="1">
Drake's pip wheels are only published for CPython 3.6 and CPython 3.7 running
on Linux.  In the future, we intend to publish additional builds.

this seems inconsistent with the installation.md, which says we use only 3.8 on focal. Do we support pip install on focal?


doc/_pages/python_bindings.md, line 19 at r4 (raw file):

<div class="warning" markdown="1">
Drake is incompatible with the Python environment supplied by Anaconda. Please

I've heard a number of people turned off by it, and also a number of people that have managed to get it to work. perhaps it could read "Drake does not support the Python enviornment supplied by Anaconda. To use our supported workflow, please uninstall...."


doc/_pages/python_bindings.md, line 50 at r4 (raw file):

modules are available. The most up-to-date high-level demonstrations of what
can be done using ``pydrake`` are in Drake's [Tutorials](/index.html#tutorials) and
the [Underactuated Robotics Textbook](http://underactuated.mit.edu/).

We should probably add the manipulation textbook here, too.

@jwnimmer-tri jwnimmer-tri added the status: single reviewer ok https://drake.mit.edu/reviewable.html label Oct 23, 2021
Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing -@BetsyMcPhail for now, I think it's OK to merge with +(status: single reviewer ok) if @RussTedrake wants to.

Reviewable status: 8 unresolved discussions, LGTM missing from assignee RussTedrake(platform) (waiting on @ggould-tri and @RussTedrake)


doc/_pages/from_binary.md, line 51 at r4 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

this just grabs the 600+kb website download website for me. wget https://github.com/RobotLocomotion/drake/releases/download/v0.35.0/drake-20211021-mac.tar.gz on the other hand actually downloads the file.

Done. (My intent was user should replace ... with a URL from above, but that was not clear.)


doc/_pages/from_binary.md, line 66 at r4 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

remove the sudo here. we explicitly do not run as sudo on mac.

Done.


doc/_pages/installation.md, line 73 at r4 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

per my question in the pip section, i'm not clear whether we support pip on focal. if no, then it seems that we need three columns here? if yes, then a note somewhere about how pip on focal uses 3.6/3.7 despite us stating that we only support 3.8 on focal just above?

Done via footnote.


doc/_pages/installation.md, line 76 at r4 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

Should we mention #12782 here somewhere? It's useful for folks to know it's potentially coming, and perhaps given them a place to comment / up-vote.

Done (below).


doc/_pages/pip.md, line 29 at r4 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

"might contain bugs" sounded a little harsh to me. perhaps "still has some known issues [cite]" is enough?

Done.


doc/_pages/pip.md, line 39 at r4 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

this seems inconsistent with the installation.md, which says we use only 3.8 on focal. Do we support pip install on focal?

We do not yet support pip install on focal.

See #15959 and https://pypi.org/project/drake/0.35.0/#files. We only publish wheels for 3.6 and 3.7.

If a user manually installed Python 3.6 or Python 3.7 on Focal, then the current wheels would probably work correctly, but we still wouldn't support it, since we wouldn't be testing it in our CI.

Once we have a 3.8 wheel published, then we can say we support Focal (with 3.8).


doc/_pages/python_bindings.md, line 19 at r4 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

I've heard a number of people turned off by it, and also a number of people that have managed to get it to work. perhaps it could read "Drake does not support the Python enviornment supplied by Anaconda. To use our supported workflow, please uninstall...."

Done.


doc/_pages/python_bindings.md, line 50 at r4 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

We should probably add the manipulation textbook here, too.

Done.

Copy link
Contributor

@RussTedrake RussTedrake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am ok with it. I've gone through the rest of the files now. A few more small questions.

Reviewed 4 of 11 files at r1, 10 of 39 files at r2, 4 of 4 files at r5, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee RussTedrake(platform) (waiting on @jwnimmer-tri)


doc/_pages/apt.md, line 53 at r5 (raw file):

sudo apt-get update
sudo apt-get install --no-install-recommends drake-dev

btw -- i did test this. I noticed that I still got 0.34.0-1, but otherwise it worked great.


doc/_pages/docker.md, line 48 at r5 (raw file):

docker pull robotlocomotion/drake:latest

i think latest doesn't actually exist? https://hub.docker.com/r/robotlocomotion/drake/tags

and i think it's just drake:bionic or drake:focal instead of drake:bionic-latest, drake:focal-latest ?


doc/_pages/from_binary.md, line 51 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Done. (My intent was user should replace ... with a URL from above, but that was not clear.)

I understood the intent and did replace the ..., but the curl command wasn't following some redirect on the website, I think. Anyhow, this resolves it.


doc/_pages/from_source.md, line 100 at r5 (raw file):

cmake ../drake
make -j

btw -- i never use the cmake hooks for building/installing the python binaries. i remember that you had a reason to not publicize the bazel spelling, but i can't remember what it was.


doc/_pages/release_playbook.md, line 133 at r5 (raw file):

    them all.
   1. Update the github links within doc/_pages/from_binary.md to reflect the
      upcoming v0.N.0 and YYYYMMDD.

btw, is there a step we're missing for apt? does pip go automatically?


tools/install/colab/setup_drake_colab.py, line 47 at r5 (raw file):

          information on obtaining a binary from an experimental branch.

    See https://drake.mit.edu/installation.html for more information.

btw. i consider this entire file to be broken (since colab updated to 3.7) and should be removed. We could add a note somewhere recommending people can pip install drake on colab, or use the Docker images on deepnote.

Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee RussTedrake(platform) (waiting on @ggould-tri and @RussTedrake)


doc/_pages/apt.md, line 53 at r5 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

btw -- i did test this. I noticed that I still got 0.34.0-1, but otherwise it worked great.

OK Yes, the apt releases are sometimes delayed by a few days; it's a manual process. Should be up early next week.


doc/_pages/docker.md, line 48 at r5 (raw file):

I think latest doesn't actually exist?

It does exist:
https://hub.docker.com/r/robotlocomotion/drake/tags?page=1&name=latest

and i think it's just drake:bionic or drake:focal instead of drake:bionic-latest, drake:focal-latest ?

That's what I intended, but the wording was unclear. Hopefully better now.


doc/_pages/from_source.md, line 100 at r5 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

btw -- i never use the cmake hooks for building/installing the python binaries. i remember that you had a reason to not publicize the bazel spelling, but i can't remember what it was.

OK Eric wrote this section originally in #7598 (I've just moved it). Perhaps the thought is end users are most familiar with CMake.


doc/_pages/release_playbook.md, line 133 at r5 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

btw, is there a step we're missing for apt? does pip go automatically?

OK At line 157 below, it says to ping Betsy to do the apt release. That happened in #15926 (comment) but Betsy hasn't had time yet to do the apt builds.

You're right that there are no instructions in this doc related to doing the pip releases. For now, it's a Jeremy-only special. We aim to make it into a Jenkins job when we have time.


tools/install/colab/setup_drake_colab.py, line 47 at r5 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

btw. i consider this entire file to be broken (since colab updated to 3.7) and should be removed. We could add a note somewhere recommending people can pip install drake on colab, or use the Docker images on deepnote.

If I'm reading you correctly, that's OK to leave for a different PR? Rather than removing the file, we should probably replace it with a stub that prints out different directions, instead? (Or just have this file run pip install?) Could you open an issue with your thinking / request?

Copy link
Contributor

@RussTedrake RussTedrake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: awesome!

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignee RussTedrake(platform) (waiting on @jwnimmer-tri)


tools/install/colab/setup_drake_colab.py, line 47 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

If I'm reading you correctly, that's OK to leave for a different PR? Rather than removing the file, we should probably replace it with a stub that prints out different directions, instead? (Or just have this file run pip install?) Could you open an issue with your thinking / request?

Yes, ok to defer. I've updated #15463.

@jwnimmer-tri jwnimmer-tri merged commit 5cb3f68 into RobotLocomotion:master Oct 24, 2021
@jwnimmer-tri jwnimmer-tri deleted the doc-install branch October 24, 2021 14:08
kunimatsu-tri pushed a commit to kunimatsu-tri/drake that referenced this pull request Oct 25, 2021
@buoyancy99
Copy link

I think it worthwhile to add warning about anaconda incompatibility in the pip installation page.

I started a fresh installation today following the page and proceeded to use the pip inside some anaconda environment. The entire process give no warning, except that a subset of modules and classes cannot be imported. Googling these issue will not lead to the issue about anaconda. I only realized the incompatibility after looking through github issues.

We should probably warn users in the pip installation page, as many people also regard pip in conda as pip and regard conda as a kind of venv.

@jwnimmer-tri
Copy link
Collaborator Author

Thanks, that's a good point. We have it at https://drake.mit.edu/python_bindings.html but it should definitely be somewhere more central during the installation workflow. I'll make up a PR.

@jwnimmer-tri
Copy link
Collaborator Author

=> #16649 now.

I'm happy to take any comments on the PR, especially if you see anything I've missed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: high release notes: none This pull request should not be mentioned in the release notes status: single reviewer ok https://drake.mit.edu/reviewable.html
Projects
None yet
5 participants