Spurious numpy writable warning #47271

ArtistBanda · 2020-11-03T09:04:19Z

facebook-github-bot · 2020-11-03T09:04:46Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

dr-ci · 2020-11-03T09:16:10Z

💊 CI failures summary and remediations

As of commit 187a07e (more details on the Dr. CI page):

3/3 failures possibly* introduced in this PR
- 1/3 non-CircleCI failure(s)

🕵️ 2 new failures recognized by patterns

The following CI failures do not appear to be due to upstream breakages:

binary_linux_libtorch_3_7m_cpu_devtoolset7_shared-with-deps_build (1/2)

Step: "Checkout pytorch/builder repo" (full log | diagnosis details | 🔁 rerun)

fatal: reference is not a tree: cd5a9b73c3028d2496666201588111a8c8d84878

+ sleep 2 
+ git submodule update --init --recursive 
fatal: reference is not a tree: cd5a9b73c3028d2496666201588111a8c8d84878 
Unable to checkout 'cd5a9b73c3028d2496666201588111a8c8d84878' in submodule path 'third_party/nccl/nccl' 
+ sleep 4 
+ git submodule update --init --recursive 
fatal: reference is not a tree: cd5a9b73c3028d2496666201588111a8c8d84878 
Unable to checkout 'cd5a9b73c3028d2496666201588111a8c8d84878' in submodule path 'third_party/nccl/nccl' 
+ sleep 8 
+ git submodule update --init --recursive 
fatal: reference is not a tree: cd5a9b73c3028d2496666201588111a8c8d84878 
Unable to checkout 'cd5a9b73c3028d2496666201588111a8c8d84878' in submodule path 'third_party/nccl/nccl'

pytorch_macos_10_13_py3_test (2/2)

Step: "Test" (full log | diagnosis details | 🔁 rerun)

Nov 10 10:47:28 AssertionError: False is not true : Expected zero exit code but got -15

Nov 10 10:47:28 ---------------------------------------------------------------------- 
Nov 10 10:47:28 Traceback (most recent call last): 
Nov 10 10:47:28   File "/Users/distiller/workspace/miniconda3/lib/python3.7/site-packages/torch/testing/_internal/common_distributed.py", line 278, in wrapper 
Nov 10 10:47:28     self._join_processes(fn) 
Nov 10 10:47:28   File "/Users/distiller/workspace/miniconda3/lib/python3.7/site-packages/torch/testing/_internal/common_distributed.py", line 395, in _join_processes 
Nov 10 10:47:28     self._check_return_codes(elapsed_time) 
Nov 10 10:47:28   File "/Users/distiller/workspace/miniconda3/lib/python3.7/site-packages/torch/testing/_internal/common_distributed.py", line 450, in _check_return_codes 
Nov 10 10:47:28     msg="Expected zero exit code but got {}".format(first_process.exitcode) 
Nov 10 10:47:28   File "/Users/distiller/workspace/miniconda3/lib/python3.7/site-packages/torch/testing/_internal/common_utils.py", line 1183, in assertEqual 
Nov 10 10:47:28     self.assertTrue(result, msg=msg) 
Nov 10 10:47:28 AssertionError: False is not true : Expected zero exit code but got -15 
Nov 10 10:47:28  
Nov 10 10:47:28 ---------------------------------------------------------------------- 
Nov 10 10:47:28 Ran 352 tests in 1435.985s 
Nov 10 10:47:28  
Nov 10 10:47:28 FAILED (failures=1, errors=1, skipped=12) 
Nov 10 10:47:28  
Nov 10 10:47:28 Generating XML reports... 
Nov 10 10:47:28 Generated XML report: test-reports/dist-gloo/TEST-ProcessGroupDdpComparisonTestWithSpawn-20201110102332.xml 
Nov 10 10:47:28 Generated XML report: test-reports/dist-gloo/TEST-ProcessGroupDdpUnderDistAutogradTestWithSpawn-20201110102332.xml 
Nov 10 10:47:28 Generated XML report: test-reports/dist-gloo/TEST-ProcessGroupDistAutogradTestWithSpawn-20201110102332.xml

ci.pytorch.org: 1 failed

Failed: pr/pytorch-linux-bionic-rocm3.8-py3.6

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 34 times.

codecov · 2020-11-03T13:25:41Z

Codecov Report

Merging #47271 into master will decrease coverage by 20.62%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           master   #47271       +/-   ##
===========================================
- Coverage   81.44%   60.81%   -20.63%     
===========================================
  Files        1798     2748      +950     
  Lines      188223   254027    +65804     
===========================================
+ Hits       153306   154499     +1193     
- Misses      34917    99528    +64611

walterddr · 2020-11-03T17:15:30Z

torch/csrc/utils/tensor_numpy.cpp

@@ -125,13 +125,13 @@ PyObject* tensor_to_numpy(const at::Tensor& tensor) {
  return array.release();
 }

-at::Tensor tensor_from_numpy(PyObject* obj) {
+at::Tensor tensor_from_numpy(PyObject* obj, bool copy_numpy) {


Suggested change

at::Tensor tensor_from_numpy(PyObject* obj, bool copy_numpy) {

at::Tensor tensor_from_numpy(PyObject* obj, bool copy_numpy=false) {

It throws an error if the default value is passed in the function implementation. So, false is passed during function declaration only.

37518cd added the comment for reference in implementation.

oops. sorry I meant to comment on the other macro path.

test/test_tensor_creation_ops.py

walterddr · 2020-11-04T16:27:01Z

torch/csrc/utils/tensor_numpy.cpp

@@ -7,7 +7,7 @@ namespace torch { namespace utils {
 PyObject* tensor_to_numpy(const at::Tensor& tensor) {
  throw std::runtime_error("PyTorch was compiled without NumPy support");
 }
-at::Tensor tensor_from_numpy(PyObject* obj) {
+at::Tensor tensor_from_numpy(PyObject* obj, bool copy_numpy=false) {


should we also remove defaults here?

But if the function is called without passing this argument it may fail. And by default, we can assume that a copy is not created and raise a warning for that.

mruberry · 2020-11-05T17:52:59Z

torch/csrc/utils/tensor_numpy.cpp

  if (!PyArray_Check(obj)) {
    throw TypeError("expected np.ndarray (got %s)", Py_TYPE(obj)->tp_name);
  }
  auto array = (PyArrayObject*)obj;

-  if (!PyArray_ISWRITEABLE(array)) {
+  if (!PyArray_ISWRITEABLE(array) && !copy_numpy) {


Would you add a comment here explaining why this warning isn't need when copy_numpy is true?

mruberry · 2020-11-05T17:53:57Z

torch/csrc/utils/tensor_numpy.cpp

@@ -125,13 +125,13 @@ PyObject* tensor_to_numpy(const at::Tensor& tensor) {
  return array.release();
 }

-at::Tensor tensor_from_numpy(PyObject* obj) {
+at::Tensor tensor_from_numpy(PyObject* obj, bool copy_numpy/*=false*/) {


This is a historic issue, but since this PR updates this function, anyway, would you add a brief comment explaining what this function does (and what the copy_numpy bool does)?

Also: what does /*=false*/ do in the signature?

Final nit on this signature: is copy_numpy the correct name for the argument? That would imply that it controls the behavior of the function, but actually this is only used to know whether or not to throw a warning, right?

Would a name like, "warn_if_not_writeable" be more descriptive?

Also: what does /*=false*/ do in the signature?

It's just a comment to tell the default parameter passed is false.

a410107 changes the parameter name and adds the comment for reference.

mruberry · 2020-11-05T17:58:38Z

test/test_tensor_creation_ops.py

@@ -1145,6 +1145,12 @@ def test_full_out(self, device):
        self.assertEqual(torch.full(o.shape, 1., out=o).dtype, o.dtype)
        self.assertEqual(torch.full(size, 1, out=o).dtype, o.dtype)

+    def test_tensor_from_non_writable_numpy(self, device):


Add a comment with the issue this test addresses so people can reference it

a410107 adds the comment

Did the force push remove the comment including the issue?

Sorry I'm confused. Where is the comment with the issue?

44ff10e This contains the comment

@ArtistBanda I think @mruberry wants you to add a reference to the issue in this comment. Either the issue ID or the URL to the issue (#47160).

That's right, thank you @thomasjo. I was expected a comment like

# Tests for X, see (link to issue)

Oh okay, sorry I didn't get that before. I'll add it now.

187a07e resolves this

mruberry · 2020-11-06T09:45:27Z

Would you rebase this, please?

Sure, it seems correct. Co-authored-by: Rong Rong <walterddr.walterddr@gmail.com>

ArtistBanda · 2020-11-06T10:13:08Z

Would you rebase this, please?

This is my first time contributing to a huge open source project and I am still learning every nook and corner of this, and I know force pushing is not the right way to go but I would love some feedback and like to improve and contribute more.

mruberry · 2020-11-10T06:19:34Z

torch/csrc/utils/tensor_numpy.cpp

-  if (!PyArray_ISWRITEABLE(array)) {
+  // warn_if_not_writable is true when a copy of numpy variable is created.
+  // the warning is suppressed when a copy is being created.
+  if (!PyArray_ISWRITEABLE(array) && !warn_if_not_writeable) {


!warn_if_not_writeable -> warn_if_not_writeable ?

Updated in c9a7efa

mruberry · 2020-11-10T06:20:47Z

torch/csrc/utils/tensor_new.cpp

@@ -270,7 +270,7 @@ Tensor internal_new_from_data(

  if (PyArray_Check(data)) {
    TORCH_CHECK(!pin_memory, "Can't pin tensor constructed from numpy");
-    auto tensor = tensor_from_numpy(data);
+    auto tensor = tensor_from_numpy(data, copy_numpy);


copy_numpy -> !copy_numpy?

That is, you want to warn_if_not_writeable when this does not copy, right?

Updated in c9a7efa

mruberry · 2020-11-10T06:21:45Z

Would you rebase this, please?

This is my first time contributing to a huge open source project and I am still learning every nook and corner of this, and I know force pushing is not the right way to go but I would love some feedback and like to improve and contribute more.

No worries. This is just a git issue. You can typically just fetch and pull the main branch into your branch.

ArtistBanda · 2020-11-10T09:00:08Z

@mruberry should I rebase again?

mruberry · 2020-11-10T09:58:24Z

@mruberry should I rebase again?

No need.

mruberry

Looks good! Thanks @ArtistBanda!

facebook-github-bot

@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-11-12T09:12:41Z

@mruberry merged this pull request in 2907447.

levkk · 2021-09-05T17:39:19Z

I'm still getting the warning in 1.9.0:

mruberry · 2021-09-06T03:04:38Z

@levkk Warning looks correct to me. cc @fmassa, torchvision may want to explicitly copy the NumPy array or suppress the warning.

pytorchbot added the open source label Nov 3, 2020

facebook-github-bot added the cla signed label Nov 3, 2020

walterddr reviewed Nov 3, 2020

View reviewed changes

walterddr reviewed Nov 4, 2020

View reviewed changes

ejguan added the module: numpy Related to numpy support, and also numpy compatibility of our operators label Nov 4, 2020

ejguan requested a review from mruberry November 4, 2020 21:09

ejguan added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Nov 4, 2020

ArtistBanda force-pushed the Spurious-numpy-writable-warning branch from 248b9f6 to 37518cd Compare November 5, 2020 14:33

mruberry reviewed Nov 5, 2020

View reviewed changes

ArtistBanda requested a review from mruberry November 6, 2020 09:44

ArtistBanda and others added 5 commits November 6, 2020 15:33

Suppressed iswritable warning when numpy copy

adbe212

Unit test for tensor from writable numpy

a8b68f8

Update test/test_tensor_creation_ops.py

f4b869d

Sure, it seems correct. Co-authored-by: Rong Rong <walterddr.walterddr@gmail.com>

Update tensor_numpy.cpp

574419e

Added docs and changed parameter name

44ff10e

ArtistBanda force-pushed the Spurious-numpy-writable-warning branch from a410107 to 44ff10e Compare November 6, 2020 10:05

mruberry reviewed Nov 10, 2020

View reviewed changes

ArtistBanda added 2 commits November 10, 2020 13:30

Updated warning suppressor parameter logic

c9a7efa

Added issue id for ref.

187a07e

mruberry approved these changes Nov 10, 2020

View reviewed changes

facebook-github-bot reviewed Nov 10, 2020

View reviewed changes

facebook-github-bot closed this in 2907447 Nov 12, 2020

facebook-github-bot added the Merged label Nov 12, 2020

	at::Tensor tensor_from_numpy(PyObject* obj, bool copy_numpy) {
	at::Tensor tensor_from_numpy(PyObject* obj, bool copy_numpy=false) {

Spurious numpy writable warning #47271

Spurious numpy writable warning #47271

Conversation

ArtistBanda commented Nov 3, 2020

facebook-github-bot commented Nov 3, 2020

dr-ci bot commented Nov 3, 2020 • edited

💊 CI failures summary and remediations

🕵️ 2 new failures recognized by patterns

binary_linux_libtorch_3_7m_cpu_devtoolset7_shared-with-deps_build (1/2)

pytorch_macos_10_13_py3_test (2/2)

ci.pytorch.org: 1 failed

codecov bot commented Nov 3, 2020 • edited

Codecov Report

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

mruberry commented Nov 6, 2020

ArtistBanda commented Nov 6, 2020

mruberry Nov 10, 2020 • edited

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

mruberry commented Nov 10, 2020

ArtistBanda commented Nov 10, 2020

mruberry commented Nov 10, 2020

mruberry left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 12, 2020

levkk commented Sep 5, 2021

mruberry commented Sep 6, 2021

dr-ci bot commented Nov 3, 2020 •

edited

codecov bot commented Nov 3, 2020 •

edited

mruberry Nov 10, 2020 •

edited