Guard NumPy usage using USE_NUMPY#11798
Conversation
Maybe this'll work.
caffe2/python/pybind_state.cc
Outdated
| const auto it = numpy_type_map.find(meta.id()); | ||
| return it == numpy_type_map.end() ? -1 : it->second; | ||
| #else | ||
| return -1; |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
caffe2/python/pybind_state.cc
Outdated
| return it == caffe_type_map.end() ? unknown_type : it->second; | ||
| #else | ||
| static TypeMeta unknown_type; | ||
| return unknown_type; |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
So, regarding the test failures you encounter, it is because the -DUSE_NUMPY is only passed to cmake but not passed down to the compilers. To do so, you should add #cmakedefine USE_NUMPY in caffe2/core/macros.h.in and you should be able to properly get the with-numpy version. Thanks so much for the rest of the PR! It looks great. |
|
By the way, when importing this into Facebook internal code, we will need to add the USE_NUMPY macro in our internal build as well, so I'll self-assign this PR to myself to import. |
|
Thank you @Yangqing for the solution. I think I've done the necessary as advised, the tests should pass now I suppose. |
1f7bae7 to
722e14f
Compare
|
The test failures persist, originating from the same place as earlier. |
|
All tests pass now, the test failures are related to DDP. |
|
@apaszke could I get a review on this? |
|
@Yangqing could I get a review on this? |
|
@pytorchbot retest this please |
|
Now all the builds have passed. I think the fix is right. |
facebook-github-bot
left a comment
There was a problem hiding this comment.
SsnL has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
apaszke
left a comment
There was a problem hiding this comment.
Can we please revert the PyArrayObject -> PyObject change?
caffe2/python/pybind_state.h
Outdated
| void FeedTensor( | ||
| const DeviceOption& option, | ||
| PyArrayObject* original_array, | ||
| PyObject* original_array, |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
@Yangqing is this good to go? |
facebook-github-bot
left a comment
There was a problem hiding this comment.
SsnL is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
There was a problem hiding this comment.
SsnL has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
extra places to be guarded: pytorch/caffe2/python/pybind_state_mkl.cc Line 33 in a2ebbcc pytorch/caffe2/python/pybind_state_mkl.cc Line 72 in a2ebbcc |
facebook-github-bot
left a comment
There was a problem hiding this comment.
SsnL has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@ssnl Will this comment help?
|
|
@vishwakftw I talked with yangqing and I'm patching internal build right now. |
|
Thank you @ssnl for working on this. |
Summary: All usages of the `ndarray` construct have now been guarded with `USE_NUMPY`. This eliminates the requirement of NumPy while building PyTorch from source. Fixes #11757 Reviewed By: Yangqing Differential Revision: D10031862 Pulled By: SsnL fbshipit-source-id: 32d84fd770a7714d544e2ca1895a3d7c75b3d712
All usages of the
ndarrayconstruct have now been guarded withUSE_NUMPY. This eliminates the requirement of NumPy while building PyTorch from source.Fixes #11757
cc: @apaszke