numpy: Require NumPy >= 0.15.0

[EricCousineau-TRI]

First checkbox in #8116

Uses commit from numpy/numpy#10898

---

This change is 

[EricCousineau-TRI]

+@jamiesnape for feature review, please, if you have time.

---

Review status: 0 of 12 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 0 of 12 files reviewed at latest revision, all discussions resolved.

---

a discussion (no related file):
Self-blocking item to upload NumPy artifacts to something other than a GitHub repo

---

Comments from Reviewable

[jwnimmer-tri]

Review status: 0 of 12 files reviewed at latest revision, 1 unresolved discussion.

---

a discussion (no related file):
I think its fine to review and iterate this proposal in its current form (running code is a better design proposal that issue discussions), but I'll renew my objection that we should only merge this to Drake master if we're citing a build of numpy master -- that is, until our numpy PR(s) merge, we should not rely on those changes for Drake.

---

Comments from Reviewable

[jamiesnape]

Review status: 0 of 12 files reviewed at latest revision, 2 unresolved discussions.

---

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I think its fine to review and iterate this proposal in its current form (running code is a better design proposal that issue discussions), but I'll renew my objection that we should only merge this to Drake master if we're citing a build of numpy master -- that is, until our numpy PR(s) merge, we should not rely on those changes for Drake.

I agree with Jeremy, we should not merge until the numpy PR has been accepted.

---

Comments from Reviewable

[jamiesnape]

Review status: 0 of 12 files reviewed at latest revision, 2 unresolved discussions.

---

a discussion (no related file):

Previously, jamiesnape (Jamie Snape) wrote…

I agree with Jeremy, we should not merge until the numpy PR has been accepted.

I just don't agree with all these "experimental" caveats and allowing other versions (as I don't agree with #8163 either) . If we are going to do this it should be the one true way and we should not clutter the language or code with the extra complications. We are writing and testing a production quality system here.

---

tools/workspace/numpy/repository.bzl, line 24 at r1 (raw file):

Arguments:
    name: A unique name for this rule.
    use_local: Use a locally installed version of NumPy. Use at your own risk!

We are only going to test one version, so I do not think we should be allowing any other version whatsoever, hence we should not have parameters like this.

---

tools/workspace/numpy/repository.bzl, line 32 at r1 (raw file):

# See `./experimental/README.md` for instructions to generate these binaries.
# PR DRAFT(eric.cousineau): Upload these to S3 or whatevs if/when they pass
# review.

Once it is accepted, I remove this comment and I will upload to S3 (https://drake-packages.csail.mit.edu/numpy/).

---

tools/workspace/numpy/repository.bzl, line 45 at r1 (raw file):

def _impl(repository_ctx):
    # Do not name this `numpy`, as Bazel will make PYTHONPATH look for love in

"look for love" is imprecise language.

---

tools/workspace/numpy/repository.bzl, line 48 at r1 (raw file):

    # all the wrong places. If `numpy` is located in the repo root,
    # `random` will be shadowed by `numpy.random`, causing everything to fail.
    # If installed elsewhere, Bazel will put its silly `__init__.py` and leak

"silly" is an opinion, so remove.

---

tools/workspace/numpy/repository.bzl, line 131 at r1 (raw file):

        use_local = False):
    if name == None:
        fail("Must supply name")

Seems strange to add a default argument and then add this check. Maybe I am missing something.

---

tools/workspace/numpy/experimental/build_direct.sh, line 1 at r1 (raw file):

#!/bin/bash

Personally, I would not put this under tools and I don't like having the "experimental" name in the path. Either it is production quality or it should not be in this repo.

---

tools/workspace/numpy/experimental/build_direct.sh, line 2 at r1 (raw file):

#!/bin/bash
set -e -x -u

BTW set -eux, ideally with an -o pipefail would be more standard.

---

tools/workspace/numpy/experimental/build_mac.sh, line 2 at r1 (raw file):

#!/bin/bash
set -e -u -x

As above.

---

tools/workspace/numpy/experimental/build_ubuntu.sh, line 2 at r1 (raw file):

#!/bin/bash
set -e -u -x

As above.

---

tools/workspace/numpy/experimental/Dockerfile, line 6 at r1 (raw file):

    && apt install --no-install-recommends -q -y \
        build-essential git \
        make libssl-dev zlib1g-dev libbz2-dev libsqlite3-dev \

build-essential installs make and since its only purpose is to install gcc, make, etc. installing make separately is unusual.

---

tools/workspace/numpy/experimental/Dockerfile, line 7 at r1 (raw file):

        build-essential git \
        make libssl-dev zlib1g-dev libbz2-dev libsqlite3-dev \
        python2.7 python2.7-dev \

Installing simple python-dev rather than these two would be more usual.

---

tools/workspace/numpy/experimental/Dockerfile, line 10 at r1 (raw file):

        python-pip python-setuptools python-wheel python-virtualenv \
        cython gfortran liblapack-dev libblas-dev \
    && rm -rf /var/lib/apt/lists/* && apt-get clean all

&& apt-get clean all is unnecessary, see https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#apt-get

---

tools/workspace/numpy/experimental/README.md, line 1 at r1 (raw file):

# Experimental NumPy

As above, either this is production or not here.

---

tools/workspace/numpy/test/numpy_test.py, line 3 at r1 (raw file):

"""
Tests that we can import NumPy, and have the version that we desire.
Note that we do not *require* the Bazel version, as sometimes it is convenient

We should require. We don't allow dev versions of anything else, and we should not this.

---

Comments from Reviewable

[jamiesnape]

Review status: 0 of 12 files reviewed at latest revision, 16 unresolved discussions.

---

tools/workspace/numpy/experimental/build_mac.sh, line 5 at r1 (raw file):

# N.B. You must ensure that you have the proper build dependencies. See
# `Dockerfile` for Ubuntu files.

List them here or add a script.

---

Comments from Reviewable

[jwnimmer-tri]

Review status: 0 of 12 files reviewed at latest revision, 17 unresolved discussions, some commit checks failed.

---

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Self-blocking item to upload NumPy artifacts to something other than a GitHub repo

Aside: @EricCousineau-TRI could you please mark yourself hard blocking here (⛔) so that the red bubble shows up in your console instead of mine?

---

a discussion (no related file):

Previously, jamiesnape (Jamie Snape) wrote…

I just don't agree with all these "experimental" caveats and allowing other versions (as I don't agree with #8163 either) . If we are going to do this it should be the one true way and we should not clutter the language or code with the extra complications. We are writing and testing a production quality system here.

Given the recent comments on numpy maintainers on numpy/numpy#10898, I think its likely that the patch will merge soonish, so it's okay to proceed reviewing this PR under the hypothetical that numpy merges to master (and hopefully even tags a release). I'd encourage @EricCousineau-TRI to push a second draft that responds to the comments so that we have time to ponder and review what a more final form of this PR would look like.

---

Comments from Reviewable

[EricCousineau-TRI]

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Given the recent comments on numpy maintainers on numpy/numpy#10898, I think its likely that the patch will merge soonish, so it's okay to proceed reviewing this PR under the hypothetical that numpy merges to master (and hopefully even tags a release). I'd encourage @EricCousineau-TRI to push a second draft that responds to the comments so that we have time to ponder and review what a more final form of this PR would look like.

OK Shall do!

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 0 of 13 files reviewed at latest revision, 17 unresolved discussions.

---

tools/workspace/numpy/repository.bzl, line 24 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

We are only going to test one version, so I do not think we should be allowing any other version whatsoever, hence we should not have parameters like this.

Done.

---

tools/workspace/numpy/repository.bzl, line 32 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Once it is accepted, I remove this comment and I will upload to S3 (https://drake-packages.csail.mit.edu/numpy/).

OK Thanks!

---

tools/workspace/numpy/repository.bzl, line 45 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

"look for love" is imprecise language.

Done.

---

tools/workspace/numpy/repository.bzl, line 48 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

"silly" is an opinion, so remove.

Done.

---

tools/workspace/numpy/repository.bzl, line 131 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Seems strange to add a default argument and then add this check. Maybe I am missing something.

Done. No longer relevant, but I had done this because numpy_repository(name = "...") would not work without this setup.

---

tools/workspace/numpy/test/numpy_test.py, line 3 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

We should require. We don't allow dev versions of anything else, and we should not this.

Done.

---

tools/workspace/numpy/experimental/build_direct.sh, line 1 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Personally, I would not put this under tools and I don't like having the "experimental" name in the path. Either it is production quality or it should not be in this repo.

Done. Renamed to packaging.

---

tools/workspace/numpy/experimental/build_direct.sh, line 2 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

BTW set -eux, ideally with an -o pipefail would be more standard.

Done.

---

tools/workspace/numpy/experimental/build_mac.sh, line 2 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

As above.

Done.

---

tools/workspace/numpy/experimental/build_mac.sh, line 5 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

List them here or add a script.

Done. Added Brewfile and referred to it in the README.

---

tools/workspace/numpy/experimental/build_ubuntu.sh, line 2 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

As above.

Done.

---

tools/workspace/numpy/experimental/Dockerfile, line 7 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Installing simple python-dev rather than these two would be more usual.

Done.

---

tools/workspace/numpy/experimental/Dockerfile, line 10 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

&& apt-get clean all is unnecessary, see https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#apt-get

Done.

---

tools/workspace/numpy/experimental/README.md, line 1 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

As above, either this is production or not here.

Done.

---

Comments from Reviewable

[jamiesnape]

Review status: 0 of 13 files reviewed at latest revision, 3 unresolved discussions.

---

tools/workspace/default.bzl, line 147 at r2 (raw file):

        nlopt_repository(name = "nlopt")
    if "numpy_py" not in excludes:
        numpy_repository(name = "numpy_py")

I think it is pretty universal that we name things xxx when they come from xxx_repository, so maybe this becomes numpy_py_repository as awkward as that sounds.

---

tools/workspace/numpy/BUILD.bazel, line 4 at r2 (raw file):

# This file exists to make our directory into a Bazel package, so that our
# neighboring *.bzl file can be loaded elsewhere.

Comment is no longer needed since the file is now more than a placeholder.

---

Comments from Reviewable

[jwnimmer-tri]

Checkpoint review of most pieces. I'll review (and test) the packaging instructions in more detail once its pointing to numpy master.

---

Reviewed 4 of 17 files at r1, 4 of 19 files at r2.
Review status: 8 of 13 files reviewed at latest revision, 5 unresolved discussions.

---

bindings/pydrake/third_party/BUILD.bazel, line 75 at r2 (raw file):

    doc_dest = "share/doc",
    deps = [
        "@numpy_py//:install",

I think repository install dependencies usually go into tools/workspace/BUILD.bazel, not injected as deps. Probably this should use the same pattern, or else comment why not.

---

tools/workspace/numpy/repository.bzl, line 96 at r2 (raw file):

)

install(

Since we are installing our own build of numpy, it seems like this target should have some install_test = [...] provided here that validates that our installed version operates correctly? I suspect the pydrake install tests kinda cover that, but proving that it works even without pydrake seems important?

---

tools/workspace/numpy/repository.bzl, line 102 at r2 (raw file):

    visibility = ["//visibility:public"],
)
    """

FYI I suggest moving this into package.BUILD.bazel for easier editing, linting, etc. Then the ctx.file changes to ctx.symlink on a Label() down below.

---

tools/workspace/pybind11/package.BUILD.bazel, line 47 at r2 (raw file):

    deps = [
        "@eigen",
        "@numpy",

FYI Confirm my understanding -- this is not required, because include/pybind11/numpy.h doesn't actually use any headers from numpy?

---

tools/workspace/numpy/packaging/README.md, line 1 at r2 (raw file):

# Packaging NumPy

Probably the first thing in the readme should explain why we are doing this.

(Something like: in order for pydrake to support numpy arrays of non-doubles, we need to use numpy upstream release of at least version foo, and because its not generally available in binary form, Drake distributes its own binary whl as a convenience.)

---

tools/workspace/numpy/packaging/README.md, line 13 at r2 (raw file):

resolve this were incorporated into `>= v1.13.1`.

## Building

Somewhere in here, the workflow for changing the numpy version we're using should be explained.

(Something like: change the tag in build_direct, and open a PR; run these commands and post a link to their output in the PR, and then ping Jamie to upload after review? Maybe there's a TODO or issue to automate the rebuild, also?)

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 2 of 16 files reviewed at latest revision, 11 unresolved discussions.

---

tools/workspace/default.bzl, line 147 at r2 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

I think it is pretty universal that we name things xxx when they come from xxx_repository, so maybe this becomes numpy_py_repository as awkward as that sounds.

Done... but definitely awkward.

---

tools/workspace/numpy/BUILD.bazel, line 4 at r2 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Comment is no longer needed since the file is now more than a placeholder.

Done.

---

tools/workspace/numpy/repository.bzl, line 96 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Since we are installing our own build of numpy, it seems like this target should have some install_test = [...] provided here that validates that our installed version operates correctly? I suspect the pydrake install tests kinda cover that, but proving that it works even without pydrake seems important?

Done.

---

tools/workspace/numpy/repository.bzl, line 102 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI I suggest moving this into package.BUILD.bazel for easier editing, linting, etc. Then the ctx.file changes to ctx.symlink on a Label() down below.

Done.

---

tools/workspace/pybind11/package.BUILD.bazel, line 47 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI Confirm my understanding -- this is not required, because include/pybind11/numpy.h doesn't actually use any headers from numpy?

OK Yup!

---

bindings/pydrake/third_party/BUILD.bazel, line 75 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I think repository install dependencies usually go into tools/workspace/BUILD.bazel, not injected as deps. Probably this should use the same pattern, or else comment why not.

Done.

---

tools/workspace/numpy/experimental/Dockerfile, line 6 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

build-essential installs make and since its only purpose is to install gcc, make, etc. installing make separately is unusual.

Done.

---

tools/workspace/numpy/packaging/README.md, line 1 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Probably the first thing in the readme should explain why we are doing this.

(Something like: in order for pydrake to support numpy arrays of non-doubles, we need to use numpy upstream release of at least version foo, and because its not generally available in binary form, Drake distributes its own binary whl as a convenience.)

Done.

---

tools/workspace/numpy/packaging/README.md, line 13 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Somewhere in here, the workflow for changing the numpy version we're using should be explained.

(Something like: change the tag in build_direct, and open a PR; run these commands and post a link to their output in the PR, and then ping Jamie to upload after review? Maybe there's a TODO or issue to automate the rebuild, also?)

Done.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 5 of 19 files at r2, 10 of 11 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

a discussion (no related file):
If Drake is using its own build of numpy as of this PR, I think the setup/** should no longer mention numpy as a dependency in Brewfile (macOS) and packages.txt (Ubuntu)?

---

a discussion (no related file):
If I am reading correctly, on master right now on macOS, we are obtaining numpy from homebrew and homebrew is at the latest current point release (1.14.2). It seems likely that homebrew would continue to track point releases. Given that, is it in fact the best choice to compile and distribute our own numpy wheel for macOS? I am not worried about macOS and Ubuntu dependency management diverging -- we already have that in other cases, and I'd rather use the best-of-breed choice on each platform, instead of forcing identical mechanisms. The downside would be a slower timeline for making numpy fixes or enhancements to reach Drake, but for a dependency as important as numpy perhaps an extra couple weeks is worth it, though I guess numpy releases are sometimes more like 1-2 month cadence. Or would it make more sense to use brew to distribute intra-release commits of numpy? Tagging @soonho-tri for discussions here also.

---

tools/workspace/numpy/BUILD.bazel, line 7 at r3 (raw file):

    "//tools/skylark:drake_py.bzl",
    "drake_py_test",
    "drake_py_unittest",

BTW Unused.

---

tools/workspace/numpy/packaging/Brewfile, line 1 at r3 (raw file):

# -*- mode: ruby -*-

I'm not sure what this file is to be used for. Nothing seems to refer to it? Does something implicitly use it? I guess for me at least some comments here or in the README would help.

---

Comments from Reviewable

[jamiesnape]

Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

If Drake is using its own build of numpy as of this PR, I think the setup/** should no longer mention numpy as a dependency in Brewfile (macOS) and packages.txt (Ubuntu)?

This is complicated. The short answer is, of course, yes, but we will still be getting numpy installed indirectly and we should not forget that drake-visualizer needs (this) numpy.

---

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

If I am reading correctly, on master right now on macOS, we are obtaining numpy from homebrew and homebrew is at the latest current point release (1.14.2). It seems likely that homebrew would continue to track point releases. Given that, is it in fact the best choice to compile and distribute our own numpy wheel for macOS? I am not worried about macOS and Ubuntu dependency management diverging -- we already have that in other cases, and I'd rather use the best-of-breed choice on each platform, instead of forcing identical mechanisms. The downside would be a slower timeline for making numpy fixes or enhancements to reach Drake, but for a dependency as important as numpy perhaps an extra couple weeks is worth it, though I guess numpy releases are sometimes more like 1-2 month cadence. Or would it make more sense to use brew to distribute intra-release commits of numpy? Tagging @soonho-tri for discussions here also.

A wheel would conflict with the Homebrew version, so it would need to be coupled with a purging of use of other formulae that use numpy and/or a move to user installs of pip dependencies.

I think slowing down the integration of fixes from numpy is actually a good thing to be honest, makes us more conscious of diverging and working around problems rather than charging off on an ever diverging fork.

---

Comments from Reviewable

[jamiesnape]

Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

tools/install/install_test.py, line 30 at r3 (raw file):

        for cmd in lines:
            cmd = cmd.strip()
            print("+ {}".format(cmd))

Remove?

---

Comments from Reviewable

[jamiesnape]

Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

tools/workspace/numpy/packaging/Brewfile, line 1 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I'm not sure what this file is to be used for. Nothing seems to refer to it? Does something implicitly use it? I guess for me at least some comments here or in the README would help.

It is mentioned in the README in this folder (brew bundle --file="./Brewfile").

---

Comments from Reviewable

[jwnimmer-tri]

tools/workspace/numpy/packaging/Brewfile, line 1 at r3 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

It is mentioned in the README in this folder (brew bundle --file="./Brewfile").

Aha, thanks! Withdrawn.

---

Comments from Reviewable

[jwnimmer-tri]

Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

tools/install/install_test.py, line 30 at r3 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Remove?

FYI I was fine leaving this output intact to help discriminate which test is being run. The output here is muted by bazel test runner unless the test fails (at which point there is already a lot of output already printed.)

---

Comments from Reviewable

[soonho-tri]

Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

a discussion (no related file):

Previously, jamiesnape (Jamie Snape) wrote…

A wheel would conflict with the Homebrew version, so it would need to be coupled with a purging of use of other formulae that use numpy and/or a move to user installs of pip dependencies.

I think slowing down the integration of fixes from numpy is actually a good thing to be honest, makes us more conscious of diverging and working around problems rather than charging off on an ever diverging fork.

FYI, homebrew is very quick at picking up a new release, so it's a matter of numpy officially releasing the new version (1.15.0). It seems that they have a plan to release another 1.14 version (1.14.3), and the next target is the 1.15.0.

Personally, +1 for waiting for the new release and use it via homebrew or pip. Another option is to use whl in the meantime, and switch to the new upstream when it's landed?

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 6 of 17 files at r13.
Reviewable status: 7 unresolved discussions, LGTM missing from assignee jamiesnape, platform LGTM from [jwnimmer-tri], labeled "do not merge"

---

tools/workspace/numpy/repository.bzl, line 30 at r13 (raw file):

wheels = {
    "ubuntu_16.04": {
        "url": "https://files.pythonhosted.org/packages/40/c5/f1ed15dd931d6667b40f1ab1c2fe1f26805fc2b6c3e25e45664f838de9d0/numpy-1.15.2-cp27-cp27mu-manylinux1_x86_64.whl",  # noqa

We will want to have a way to list more than one mirror here. In fact, mirrors.bzl already has one, that we should use. In fact, we already have pypi.bzl that we should take advantage of, or else at least cite here with a justification for not using it.

---

tools/workspace/numpy/repository.bzl, line 73 at r13 (raw file):

    )

numpy_py_repository = repository_rule(

Hmm, why did this gain an extra py? We don't have extra "_py" suffix in Drake for any of the other python externals.

Also, now the tools/workspace/DIR doesn't match the macro name.

---

tools/workspace/numpy/test/numpy_test.py, line 7 at r13 (raw file):

import numpy as np

import numpy as np

nit Duplicates the import above.

---

tools/workspace/numpy/test/numpy_test.py, line 18 at r13 (raw file):

            "WORKSPACE and usage of `numpy_py_repository` are well-formed.")
        # N.B. This should be updated each time the version is bumped.
        self.assertEqual("1.15.2", np.version.version)

nit Please be consistent in the "1.15.2" vs "np.version.version" argument order, for this numpy_test.py vs numpy_install_test.py.

[EricCousineau-TRI]

Dismissed @jamiesnape from a discussion.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee jamiesnape, platform LGTM from [jwnimmer-tri], labeled "do not merge"

a discussion (no related file):

Previously, jamiesnape (Jamie Snape) wrote…

My impression too.

Done.

---

---

tools/workspace/numpy/repository.bzl, line 30 at r13 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

We will want to have a way to list more than one mirror here. In fact, mirrors.bzl already has one, that we should use. In fact, we already have pypi.bzl that we should take advantage of, or else at least cite here with a justification for not using it.

Done.

---

tools/workspace/numpy/repository.bzl, line 73 at r13 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Hmm, why did this gain an extra py? We don't have extra "_py" suffix in Drake for any of the other python externals.

Also, now the tools/workspace/DIR doesn't match the macro name.

Done. (Hoisted statement this into Warning: in docstring.)

[EricCousineau-TRI]

@drake-jenkins-bot linux-bionic-unprovisioned-clang-bazel-experimental please.
@drake-jenkins-bot linux-xenial-unprovisioned-clang-bazel-experimental please.
@drake-jenkins-bot mac-highsierra-unprovisioned-clang-bazel-experimental please.
@drake-jenkins-bot mac-highsierra-clang-bazel-experimental please.

[EricCousineau-TRI]

This simplest PR that uses this is captured in #9997.

[jamiesnape]

Reviewable status: 6 unresolved discussions, LGTM missing from assignee jamiesnape, platform LGTM from [jwnimmer-tri], labeled "do not merge"

---

tools/workspace/mirrors.bzl, line 52 at r14 (raw file):

        "https://s3.amazonaws.com/drake-mirror/pypi/{package}/{package}-{version}.tar.gz",  # noqa
    ],
    "pypi-general": [

Do we really need another mirror type? I think you can modify pypi_archive to make it satisfy both cases and will happily duplicate any existing packages to https://drake-mirror.csail.mit.edu/pypi/source and put the numpy ones in ``https://drake-mirror.csail.mit.edu/pypi`.

---

tools/workspace/numpy_py/package.noop.BUILD.bazel, line 9 at r14 (raw file):

# No-op to permit using system library.
py_library(

I think we can have a proper target here. The files are in predictable locations.

[jamiesnape]

Reviewable status: 6 unresolved discussions, LGTM missing from assignee jamiesnape, platform LGTM from [jwnimmer-tri], labeled "do not merge"

---

tools/workspace/mirrors.bzl, line 52 at r14 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Do we really need another mirror type? I think you can modify pypi_archive to make it satisfy both cases and will happily duplicate any existing packages to https://drake-mirror.csail.mit.edu/pypi/source and put the numpy ones in ``https://drake-mirror.csail.mit.edu/pypi`.

Actually, you can use hash_path/filename for sources too, so just add an argument for wheel name to pypi_archive and I will upload the numpy archives to https://drake-mirror.csail.mit.edu/pypi/{package}/{wheel}

[jwnimmer-tri]

Small checkpoint. I'll come back to it later this week.

Reviewed 15 of 24 files at r14.
Reviewable status: 7 unresolved discussions, LGTM missing from assignee jamiesnape, platform LGTM from [jwnimmer-tri], labeled "do not merge"

---

bindings/pydrake/BUILD.bazel, line 48 at r14 (raw file):

        "//bindings/pydrake/util:compatibility_py",
        "//bindings/pydrake/util:deprecation_py",
        "@numpy_py",

nit This seems weird / unused. The __init__.py doesn't use np. Sure, the :compatibility_py uses it, but then it should be a deps of the util target, not this one? Or if we do really need an apparently-unused deps line, it needs a comment to explain why not to accidentally remove it.

---

bindings/pydrake/util/BUILD.bazel, line 62 at r14 (raw file):

    srcs = ["__init__.py"],
    imports = PACKAGE_INFO.py_imports,
    deps = ["@numpy_py"],

nit This seems weird / unused. The __init__.py doesn't use np. Sure, the :compatibility_py uses it, but then it should be a deps of the util target, not this one? Or if we do really need an apparently-unused deps line, it needs a comment to explain why not to accidentally remove it.

---

bindings/pydrake/util/compatibility.py, line 21 at r14 (raw file):

    minimum = NumpyVersion('1.15.0')
    if actual < minimum:
        raise RuntimeError(

Remind me how much of pydrake will be broken if we don't have it? I am wondering if an exception on import is too severe -- perhaps only certain modules should have a hard requirement?

---

tools/workspace/numpy/repository.bzl, line 30 at r13 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Done.

OK Indeed, as Jamie noted separately, if we're not going to use pypi.bzl we need a comment explaining why. (Really, we should just use pypi.bzl though.)

---

tools/workspace/numpy_py/BUILD.bazel, line 7 at r14 (raw file):

    "//tools/skylark:drake_py.bzl",
    "drake_py_unittest",
)

nit Unused.

[EricCousineau-TRI]

Reviewable status: 5 unresolved discussions, LGTM missing from assignee jamiesnape, platform LGTM from [jwnimmer-tri], labeled "do not merge"

---

bindings/pydrake/BUILD.bazel, line 48 at r14 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit This seems weird / unused. The __init__.py doesn't use np. Sure, the :compatibility_py uses it, but then it should be a deps of the util target, not this one? Or if we do really need an apparently-unused deps line, it needs a comment to explain why not to accidentally remove it.

Done.

---

bindings/pydrake/util/BUILD.bazel, line 62 at r14 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit This seems weird / unused. The __init__.py doesn't use np. Sure, the :compatibility_py uses it, but then it should be a deps of the util target, not this one? Or if we do really need an apparently-unused deps line, it needs a comment to explain why not to accidentally remove it.

Done.

---

bindings/pydrake/util/compatibility.py, line 21 at r14 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Remind me how much of pydrake will be broken if we don't have it? I am wondering if an exception on import is too severe -- perhaps only certain modules should have a hard requirement?

OK Given how pervasively numpy is used in pydrake, I'd prefer to fail as fast as possible.

---

tools/workspace/mirrors.bzl, line 52 at r14 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Actually, you can use hash_path/filename for sources too, so just add an argument for wheel name to pypi_archive and I will upload the numpy archives to https://drake-mirror.csail.mit.edu/pypi/{package}/{wheel}

OK Tried it out... I can't say things look all that better from a usage standpoint for pypi_archive, but now there are only two usages, and they are both distinct (macro vs. rule), in all of Drake + Anzu.

---

tools/workspace/numpy/repository.bzl, line 30 at r13 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

OK Indeed, as Jamie noted separately, if we're not going to use pypi.bzl we need a comment explaining why. (Really, we should just use pypi.bzl though.)

OK Put a TODO which could be re-done pending #10002.

---

tools/workspace/numpy_py/package.noop.BUILD.bazel, line 9 at r14 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

I think we can have a proper target here. The files are in predictable locations.

OK Would prefer not to, at least in this PR.

It's how we've been pulling in system dependencies, and would prefer that another PR focus specifically on this (esp. if we can make an issue as to why we want to do this).

[jamiesnape]

Reviewable status: 4 unresolved discussions, LGTM missing from assignee jamiesnape, platform LGTM from [jwnimmer-tri], labeled "do not merge"

---

tools/workspace/numpy_py/package.noop.BUILD.bazel, line 9 at r14 (raw file):

It's how we've been pulling in system dependencies, and would prefer that another PR focus specifically on this (esp. if we can make an issue as to why we want to do this).

That is not really true and I think you will find it is only 2-3 more lines of code. We have always had parity the content of dependencies between Mac and Linux and a no-op target is not a design pattern we should promote even temporarily, I think.

[jwnimmer-tri]

Reviewed 3 of 8 files at r15.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jamiesnape, platform LGTM from [jwnimmer-tri], labeled "do not merge"

---

bindings/pydrake/util/compatibility.py, line 21 at r14 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

OK Given how pervasively numpy is used in pydrake, I'd prefer to fail as fast as possible.

You are almost certainly correct in that assessment, but I still would like an answer my question so that I can understand (and we can document as part of this PR thread) that line of reasoning.

[jamiesnape]

Reviewable status: 3 unresolved discussions, LGTM missing from assignee jamiesnape, platform LGTM from [jwnimmer-tri], labeled "do not merge"

---

tools/workspace/mirrors.bzl, line 52 at r14 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

OK Tried it out... I can't say things look all that better from a usage standpoint for pypi_archive, but now there are only two usages, and they are both distinct (macro vs. rule), in all of Drake + Anzu.

Well, the old naming scheme (that was used for source files in our case) is changed/partially broken anyway now (technically it was not part of an API, so deprecated is not the correct word), so this would have had to be done in some form anyway.

[jamiesnape]

Reviewable status: 3 unresolved discussions, LGTM missing from assignee jamiesnape, platform LGTM from [jwnimmer-tri], labeled "do not merge"

---

tools/workspace/mirrors.bzl, line 52 at r14 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Well, the old naming scheme (that was used for source files in our case) is changed/partially broken anyway now (technically it was not part of an API, so deprecated is not the correct word), so this would have had to be done in some form anyway.

(pypi/legacy#438 (comment))

[EricCousineau-TRI]

Reviewable status: 3 unresolved discussions, LGTM missing from assignee jamiesnape, platform LGTM from [jwnimmer-tri], labeled "do not merge"

---

tools/workspace/mirrors.bzl, line 52 at r14 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

(pypi/legacy#438 (comment))

OK Gotcha, makes sense then!

---

bindings/pydrake/util/compatibility.py, line 21 at r14 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

You are almost certainly correct in that assessment, but I still would like an answer my question so that I can understand (and we can document as part of this PR thread) that line of reasoning.

Done. Added in a comment to the function in renamed file.

---

tools/workspace/numpy_py/package.noop.BUILD.bazel, line 9 at r14 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

It's how we've been pulling in system dependencies, and would prefer that another PR focus specifically on this (esp. if we can make an issue as to why we want to do this).

That is not really true and I think you will find it is only 2-3 more lines of code. We have always had parity the content of dependencies between Mac and Linux and a no-op target is not a design pattern we should promote even temporarily, I think.

Done. (Added it via repository.bzl)

[jwnimmer-tri]

I've finished a code review pass up to latest r16. I should still probably re-test locally, after the next round of fixes.

Reviewed 1 of 24 files at r14, 3 of 8 files at r15, 9 of 11 files at r16.
Reviewable status: 7 unresolved discussions, LGTM missing from assignee jamiesnape, platform LGTM from [jwnimmer-tri], labeled "do not merge"

a discussion (no related file):
It is possible that we should rethink whether the //:install rule should install numpy onto Drake's python path (for Ubuntu). Given the "Drake installed a broken meshcat, just rm -rf $drake_prefix/lib/python2.7/meshcat and then it will fall back to your pip" experience of late, it is possible that users should have to opt-in to our installed third-party python packages, via a different path, instead of always being forced to eat them when they add pydrake to their path.

---

---

tools/workspace/numpy_py/repository.bzl, line 8 at r16 (raw file):

the minimum required version.

See `check_required_numpy_version` in `pydrake/util/compatibility.py` for more

nit s/util/common/.

---

tools/workspace/numpy_py/repository.bzl, line 31 at r16 (raw file):

Arguments:
    name: A unique name for this rule.
    mirrors: Mirrors for download that must have entries for "pypi-general".

nit pypi-general is stale.

---

tools/workspace/numpy_py/repository.bzl, line 105 at r16 (raw file):

        repository_ctx.download_and_extract(
            url = urls,
            sha256 = wheel["sha256"],

nit Consider wheel["sha256"] or "0" * 64 here, so that the sha is never ever blank. A naive developer might null it out in the dict above, since that's what they can do everywhere else too.

---

tools/workspace/numpy_py/package.BUILD.bazel.in, line 27 at r16 (raw file):

)

@INSTALL_CLAUSE@

FYI Another way to do this is to load instead of textual substitution of the@INSTALL_CLAUSE@:

load(":helper.bzl", "maybe_declare_install")
maybe_declare_install()

... and then in repository.bzl symlink either helper-macos.bzl or helper-ubuntu.bzl in place depending on the OS. That makes the linter apply to the install fragments, and generally keeps load-time code and build-time code in separate files.

---

tools/workspace/numpy/package.BUILD.bazel, line 35 at r13 (raw file):

    data_dest = "lib/python2.7/site-packages",
    visibility = ["//visibility:public"],
)

As of r3 there was an install_tests = [] here, dropped as of r14. Where did this test go? It seems to me, since we're installing a custom numpy on Ubuntu, that we should test that numpy works correctly in the installed file layout.

First, fo test numpy vs drake, probably either drake/bindings/pydrake/test/common_install_test.py should call check_required_numpy_version specifically in a test case, or else all_install_test.py should do it? You could even imagine using a custom dtype after calling the version check.

(I guess there's also an argument that the import-level check is already good enough, as long as we are sure that we've run it as part of one of those two test cases, so maybe a comment somewhere is good enough. But adding a three-line test seems easy and very much clearer.)

Second, I also think its important to test numpy on its own, without importing pydrake. So I still think that this workspace install rule (on Ubuntu) should have its own install_tests = [] entry that just imports and uses numpy, without importing pydrake.

[jamiesnape]

Reviewable status: 7 unresolved discussions, LGTM missing from assignee jamiesnape, platform LGTM from [jwnimmer-tri], labeled "do not merge"

a discussion (no related file):

It is possible that we should rethink whether the //:install rule should install numpy onto Drake's python path (for Ubuntu). Given the "Drake installed a broken meshcat, just rm -rf $drake_prefix/lib/python2.7/meshcat and then it will fall back to your pip" experience of late, it is possible that users should have to opt-in to our installed third-party python packages, via a different path, instead of always being forced to eat them when they add pydrake to their path.

I think that is an overreaction, but I really have never been on board with this PR, so I guess I agree with the numpy sentiment. However, if we are not going to install it, then we should not require it, and therefore this PR has little purpose.

---

[jwnimmer-tri]

Reviewable status: 7 unresolved discussions, LGTM missing from assignee jamiesnape, platform LGTM from [jwnimmer-tri], labeled "do not merge"

a discussion (no related file):

Previously, jamiesnape (Jamie Snape) wrote…

It is possible that we should rethink whether the //:install rule should install numpy onto Drake's python path (for Ubuntu). Given the "Drake installed a broken meshcat, just rm -rf $drake_prefix/lib/python2.7/meshcat and then it will fall back to your pip" experience of late, it is possible that users should have to opt-in to our installed third-party python packages, via a different path, instead of always being forced to eat them when they add pydrake to their path.

I think that is an overreaction, but I really have never been on board with this PR, so I guess I agree with the numpy sentiment. However, if we are not going to install it, then we should not require it, and therefore this PR has little purpose.

I agree that not installing it would be poor, since its required.

The question is more like, if the user has their own (newer) numpy from pip already on their pythonpath (say, virtualenv), do we want pydrake's pythonpath to unconditionally shadow numpy? If pydrake and third-party-python were two different pythonpaths, the user could decide whether to use drake's third-party-python, or to only take pydrake (and be responsible for the dependencies). We could probably sugar up the ergonomics.

We don't have to gold plat it all yet, but I do want to be sure we agree on the rough path, before we start putting numpy on the pydrake's pythonpath if we think that path is too dangerous.

---

[EricCousineau-TRI]

Reviewable status: 7 unresolved discussions, LGTM missing from assignee jamiesnape, platform LGTM from [jwnimmer-tri], labeled "do not merge"

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I agree that not installing it would be poor, since its required.

The question is more like, if the user has their own (newer) numpy from pip already on their pythonpath (say, virtualenv), do we want pydrake's pythonpath to unconditionally shadow numpy? If pydrake and third-party-python were two different pythonpaths, the user could decide whether to use drake's third-party-python, or to only take pydrake (and be responsible for the dependencies). We could probably sugar up the ergonomics.

We don't have to gold plat it all yet, but I do want to be sure we agree on the rough path, before we start putting numpy on the pydrake's pythonpath if we think that path is too dangerous.

I am certainly all for providing a mechanism to ditch Drake's numpy when a user wants to (as I find that super useful when wanting to burrow into Python+GDB+Valgrind) , and it should be relatively easy since we already have the logic for Mac.

For now, I can just add a simple binary switch, and we can float that to some more user-friendly argument if it's an issue.

From working with pytorch a bit myself, I haven't seen any issues (at least at inference time with SSD-6D based networks) when using a later version of NumPy, since it's ABI is relatively robust.

---

[jwnimmer-tri]

Reviewable status: 7 unresolved discussions, LGTM missing from assignee jamiesnape, platform LGTM from [jwnimmer-tri], labeled "do not merge"

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

I am certainly all for providing a mechanism to ditch Drake's numpy when a user wants to (as I find that super useful when wanting to burrow into Python+GDB+Valgrind) , and it should be relatively easy since we already have the logic for Mac.

For now, I can just add a simple binary switch, and we can float that to some more user-friendly argument if it's an issue.

From working with pytorch a bit myself, I haven't seen any issues (at least at inference time with SSD-6D based networks) when using a later version of NumPy, since it's ABI is relatively robust.

A configuration option is another kind of burden. Might still need to think more / discuss.

(In any case, plenty of other discussions below to fix first.)

---

[EricCousineau-TRI]

Closing for now. Can re-open later.