Skip to content

BUG: test, fix problems from PR #9639 #10160

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

Merged
merged 3 commits into from
Dec 5, 2017
Merged

Conversation

mattip
Copy link
Member

@mattip mattip commented Dec 4, 2017

PR #9639 was merged, but later we uncovered some problems with it in working on PR #9998.

@ahaldane uncovered a bug where the NPY_WRITEBACKIFCOPY flag was not zeroed out in creating a ndarray from existing data.

Additionally, I realized that between the call to PyArray_ResolveWritebackIfCopy and array_dealloc, the ndarray is still, in some sense, alive, and so care must be taken to leave it in a coherent state after the call to PyArray_ResolveWritebackIfCopy.

This pull request tests and fixes both the problems above, where arr_wb.flags.writebackifcopy == True and arr_wb.base is arr:

  • test and fix that arr_wb[:] assignment, which internally creates a view to arr_wb, does not create the view with writeback semantics (error in zeroing out the view's flag)
  • test and fix that after the call to PyArray_WritebackIfCopy(arr_wb), the data pointer of arr_wb is still valid, but that the connection between arr_wb and arr has been severed, arr_wb.base is no longer arr

@ghost
Copy link

ghost commented Dec 4, 2017

I know this is probably going to be disappointing, but IMO changing C code right before a release is playing with fire. Everyone wants their PRs merged (including me!), but sometimes you don't make it on time.

@mattip
Copy link
Member Author

mattip commented Dec 4, 2017

In any case this could be merged to master

@ghost
Copy link

ghost commented Dec 4, 2017

Definitely agree that it should be merged to master ASAP.

@ahaldane
Copy link
Member

ahaldane commented Dec 4, 2017

Yeah, these are the two bugfixes we need, so looks good.

But one other thing I am concerned about: Didn't you need to sprinkle use of PyArray_WritebackIfCopy into all the uses of nditer in C in numpy, eg in numpy/core/src/umath/ufunc_object.c? Does that mean users of the C api are going to need to update their C code in a similar way in 1.14? And won't they then have to undo it in 1.15? That seems problematic.

@ghost
Copy link

ghost commented Dec 4, 2017

@ahaldane You can see all of the PRs with https://github.com/numpy/numpy/pulls?q=is%3Apr+author%3Amattip+is%3Aclosed

It should be really easy to revert all of them on the 1.14 branch once it's created.

@ghost
Copy link

ghost commented Dec 4, 2017

We also need to revert #10022 on the 1.14 branch though.

@ahaldane
Copy link
Member

ahaldane commented Dec 4, 2017

@xoviat wait I am confused. I thought your goal with this PR was not to revert any of the merged PRs, even on 1.14. I thought the idea was we would merge this PR to fix the bugs in WritebackIfCopy, merge it so we get WritebackIfCopy in 1.14, but leave the context manager stuff for 1.15.

@mattip
Copy link
Member Author

mattip commented Dec 4, 2017

@ahaldane I will review those and replace where possible with NpyIter_Close in #9998

@ghost
Copy link

ghost commented Dec 4, 2017

I thought your goal with this PR was not to revert any of the merged PRs, even on 1.14.

I think that's @mattip's goal. My view on this is that either we need to:

  1. Hold off on branching for a few more days so that we aren't rushed into resolving these issues.
  2. Revert all of the writebackifcopy stuff in the 1.14 branch.

@ahaldane
Copy link
Member

ahaldane commented Dec 4, 2017

@mattip I don't think we can release WritebackIfCopy in 1.14 if it is going to break C code using nditer. If the fix for that is only going to come in #9998, that is probably too late.

(Incidentally, I don't think we all agreed that NpyIter_Close was the right solution yet)

Py_DECREF(fa->base);
if (fa->base) {
Py_DECREF(fa->base);
}
Copy link
Member

Choose a reason for hiding this comment

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

This can be Py_XDECREF.

@ahaldane
Copy link
Member

ahaldane commented Dec 4, 2017

Otherwise the code and test in this PR look good to me, lets just let it sit a little before merging.

@mattip
Copy link
Member Author

mattip commented Dec 4, 2017

@ahaldane #9639 does not break C code, in the worst case a warning is only emitted if during PyArray_FromArray the UPDATEIFCOPY flag is requested, which can only happen directly in C code. No warning is emitted if PyArray_ResolveWritebackIfCopy is not called before NpyIter_Deallocate, no warning is emitted if an NpyIter is created using writeback semantics (or updateifcopy semantics if you prefer the old terminology).

We were very careful to make an exception for nditer in #9639, and not to force anyone to do anything at this stage. That was the purpose of #9998. The code you see in ufunc_object.c is to ensure that PyPy can work properly, not because it is required rather because it is desired.

@mattip
Copy link
Member Author

mattip commented Dec 4, 2017

Since it is more than likely #9998 will not make the 1.14 cut, I tried to document the situation

@ghost
Copy link

ghost commented Dec 4, 2017

Let's just hold off branching for a few days then and then we can put this in.

@ahaldane
Copy link
Member

ahaldane commented Dec 5, 2017

@mattip, OK I see better what is going on, and that the ufunc_object.c writeback code is not necessary at this point, except to help PyPy.

I'm going to look over this one more time and merge in a little while.

@ahaldane
Copy link
Member

ahaldane commented Dec 5, 2017

All right, let's do this. Thanks for explaining @mattip.

@ahaldane ahaldane merged commit 82deb85 into numpy:master Dec 5, 2017
@mattip mattip deleted the fix-PR9639 branch December 5, 2017 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants