{"action":"created","issue":{"url":"https://api.github.com/repos/RobotLocomotion/drake/issues/8608","repository_url":"https://api.github.com/repos/RobotLocomotion/drake","labels_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/8608/labels{/name}","comments_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/8608/comments","events_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/8608/events","html_url":"https://github.com/RobotLocomotion/drake/pull/8608","id":314865958,"number":8608,"title":"Use packaged NumPy build in Bazel","user":{"login":"EricCousineau-TRI","id":26719449,"avatar_url":"https://avatars1.githubusercontent.com/u/26719449?v=4","gravatar_id":"","url":"https://api.github.com/users/EricCousineau-TRI","html_url":"https://github.com/EricCousineau-TRI","followers_url":"https://api.github.com/users/EricCousineau-TRI/followers","following_url":"https://api.github.com/users/EricCousineau-TRI/following{/other_user}","gists_url":"https://api.github.com/users/EricCousineau-TRI/gists{/gist_id}","starred_url":"https://api.github.com/users/EricCousineau-TRI/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/EricCousineau-TRI/subscriptions","organizations_url":"https://api.github.com/users/EricCousineau-TRI/orgs","repos_url":"https://api.github.com/users/EricCousineau-TRI/repos","events_url":"https://api.github.com/users/EricCousineau-TRI/events{/privacy}","received_events_url":"https://api.github.com/users/EricCousineau-TRI/received_events","type":"User","site_admin":false},"labels":[{"id":353617587,"url":"https://api.github.com/repos/RobotLocomotion/drake/labels/status:%20do%20not%20merge","name":"status: do not merge","color":"e67e22","default":false}],"state":"open","locked":false,"assignee":{"login":"jamiesnape","id":3630149,"avatar_url":"https://avatars3.githubusercontent.com/u/3630149?v=4","gravatar_id":"","url":"https://api.github.com/users/jamiesnape","html_url":"https://github.com/jamiesnape","followers_url":"https://api.github.com/users/jamiesnape/followers","following_url":"https://api.github.com/users/jamiesnape/following{/other_user}","gists_url":"https://api.github.com/users/jamiesnape/gists{/gist_id}","starred_url":"https://api.github.com/users/jamiesnape/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jamiesnape/subscriptions","organizations_url":"https://api.github.com/users/jamiesnape/orgs","repos_url":"https://api.github.com/users/jamiesnape/repos","events_url":"https://api.github.com/users/jamiesnape/events{/privacy}","received_events_url":"https://api.github.com/users/jamiesnape/received_events","type":"User","site_admin":false},"assignees":[{"login":"jamiesnape","id":3630149,"avatar_url":"https://avatars3.githubusercontent.com/u/3630149?v=4","gravatar_id":"","url":"https://api.github.com/users/jamiesnape","html_url":"https://github.com/jamiesnape","followers_url":"https://api.github.com/users/jamiesnape/followers","following_url":"https://api.github.com/users/jamiesnape/following{/other_user}","gists_url":"https://api.github.com/users/jamiesnape/gists{/gist_id}","starred_url":"https://api.github.com/users/jamiesnape/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jamiesnape/subscriptions","organizations_url":"https://api.github.com/users/jamiesnape/orgs","repos_url":"https://api.github.com/users/jamiesnape/repos","events_url":"https://api.github.com/users/jamiesnape/events{/privacy}","received_events_url":"https://api.github.com/users/jamiesnape/received_events","type":"User","site_admin":false},{"login":"jwnimmer-tri","id":17596505,"avatar_url":"https://avatars3.githubusercontent.com/u/17596505?v=4","gravatar_id":"","url":"https://api.github.com/users/jwnimmer-tri","html_url":"https://github.com/jwnimmer-tri","followers_url":"https://api.github.com/users/jwnimmer-tri/followers","following_url":"https://api.github.com/users/jwnimmer-tri/following{/other_user}","gists_url":"https://api.github.com/users/jwnimmer-tri/gists{/gist_id}","starred_url":"https://api.github.com/users/jwnimmer-tri/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jwnimmer-tri/subscriptions","organizations_url":"https://api.github.com/users/jwnimmer-tri/orgs","repos_url":"https://api.github.com/users/jwnimmer-tri/repos","events_url":"https://api.github.com/users/jwnimmer-tri/events{/privacy}","received_events_url":"https://api.github.com/users/jwnimmer-tri/received_events","type":"User","site_admin":false}],"milestone":null,"comments":10,"created_at":"2018-04-17T00:45:27Z","updated_at":"2018-04-23T23:40:19Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/RobotLocomotion/drake/pulls/8608","html_url":"https://github.com/RobotLocomotion/drake/pull/8608","diff_url":"https://github.com/RobotLocomotion/drake/pull/8608.diff","patch_url":"https://github.com/RobotLocomotion/drake/pull/8608.patch"},"body":"Uses commit from numpy/numpy#10898\n\n<!-- Reviewable:start -->\n---\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"34\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/robotlocomotion/drake/8608)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/RobotLocomotion/drake/issues/comments/383756270","html_url":"https://github.com/RobotLocomotion/drake/pull/8608#issuecomment-383756270","issue_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/8608","id":383756270,"user":{"login":"jwnimmer-tri","id":17596505,"avatar_url":"https://avatars3.githubusercontent.com/u/17596505?v=4","gravatar_id":"","url":"https://api.github.com/users/jwnimmer-tri","html_url":"https://github.com/jwnimmer-tri","followers_url":"https://api.github.com/users/jwnimmer-tri/followers","following_url":"https://api.github.com/users/jwnimmer-tri/following{/other_user}","gists_url":"https://api.github.com/users/jwnimmer-tri/gists{/gist_id}","starred_url":"https://api.github.com/users/jwnimmer-tri/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jwnimmer-tri/subscriptions","organizations_url":"https://api.github.com/users/jwnimmer-tri/orgs","repos_url":"https://api.github.com/users/jwnimmer-tri/repos","events_url":"https://api.github.com/users/jwnimmer-tri/events{/privacy}","received_events_url":"https://api.github.com/users/jwnimmer-tri/received_events","type":"User","site_admin":false},"created_at":"2018-04-23T23:40:19Z","updated_at":"2018-04-23T23:40:19Z","author_association":"COLLABORATOR","body":"Checkpoint review of most pieces.  I'll review (and test) the packaging instructions in more detail once its pointing to numpy master.\n\n---\n\nReviewed 4 of 17 files at r1, 4 of 19 files at r2.\nReview status: 8 of 13 files reviewed at latest revision, 5 unresolved discussions.\n\n---\n\n*[bindings/pydrake/third_party/BUILD.bazel, line 75 at r2](https://beta.reviewable.io/reviews/robotlocomotion/drake/8608#-LAomKDo389RNx1VUA_9:-LAomKDo389RNx1VUA_A:bhybzu) ([raw file](https://github.com/robotlocomotion/drake/blob/e533cd26238beedcb895a8a1ff937ef9e20d21d1/bindings/pydrake/third_party/BUILD.bazel#L75)):*\n> ```Python\n>     doc_dest = \"share/doc\",\n>     deps = [\n>         \"@numpy_py//:install\",\n> ```\n\nI 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.\n\n---\n\n*[tools/workspace/numpy/repository.bzl, line 96 at r2](https://beta.reviewable.io/reviews/robotlocomotion/drake/8608#-LAooxpR-zkgBnIzOhvs:-LAooxpR-zkgBnIzOhvt:boiyz5z) ([raw file](https://github.com/robotlocomotion/drake/blob/e533cd26238beedcb895a8a1ff937ef9e20d21d1/tools/workspace/numpy/repository.bzl#L96)):*\n> ```Python\n> )\n> \n> install(\n> ```\n\nSince 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?\n\n---\n\n*[tools/workspace/numpy/repository.bzl, line 102 at r2](https://beta.reviewable.io/reviews/robotlocomotion/drake/8608#-LAoqFF1D7KQdQKR3tIa:-LAoqFF3E-50dZTqEqS3:b58zdzo) ([raw file](https://github.com/robotlocomotion/drake/blob/e533cd26238beedcb895a8a1ff937ef9e20d21d1/tools/workspace/numpy/repository.bzl#L102)):*\n> ```Python\n>     visibility = [\"//visibility:public\"],\n> )\n>     \"\"\"\n> ```\n\nFYI 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.\n\n---\n\n*[tools/workspace/pybind11/package.BUILD.bazel, line 47 at r2](https://beta.reviewable.io/reviews/robotlocomotion/drake/8608#-LAoo5wXAr_kYOMqVBOa:-LAoo5wXAr_kYOMqVBOb:b-4y1g5q) ([raw file](https://github.com/robotlocomotion/drake/blob/68c65489cba2c69fbbe49f922e663169de51f1b4/tools/workspace/pybind11/package.BUILD.bazel#L47)):*\n> ```\n>     deps = [\n>         \"@eigen\",\n>         \"@numpy\",\n> ```\n\nFYI Confirm my understanding -- this is not required, because `include/pybind11/numpy.h` doesn't actually use any headers from numpy?\n\n---\n\n*[tools/workspace/numpy/packaging/README.md, line 1 at r2](https://beta.reviewable.io/reviews/robotlocomotion/drake/8608#-LAorIOs-F7Ly4nhpS4L:-LAorIOs-F7Ly4nhpS4M:b-4klatb) ([raw file](https://github.com/robotlocomotion/drake/blob/e533cd26238beedcb895a8a1ff937ef9e20d21d1/tools/workspace/numpy/packaging/README.md#L1)):*\n> ```Markdown\n> # Packaging NumPy\n> ```\n\nProbably the first thing in the readme should explain _why_ we are doing this.\n\n(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.)\n\n---\n\n*[tools/workspace/numpy/packaging/README.md, line 13 at r2](https://beta.reviewable.io/reviews/robotlocomotion/drake/8608#-LAorhh-Cqai-8oeUM0W:-LAorhh-Cqai-8oeUM0X:b-eue7jx) ([raw file](https://github.com/robotlocomotion/drake/blob/e533cd26238beedcb895a8a1ff937ef9e20d21d1/tools/workspace/numpy/packaging/README.md#L13)):*\n> ```Markdown\n> resolve this were incorporated into `>= v1.13.1`.\n> \n> ## Building\n> ```\n\nSomewhere in here, the workflow for changing the numpy version we're using should be explained.\n\n(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?)\n\n---\n\n\n*Comments from [Reviewable](https://beta.reviewable.io/reviews/robotlocomotion/drake/8608#-:-LAos8ek7SW8PgAnj-2z:b8rz89d)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"jwnimmer-tri"}}