BUG,DOC: Allow attach docs twice but error if wrong #16239

seberg · 2020-05-14T18:01:23Z

This stops general try/except, and instead skips setting of
docstrings if the docstring is identical. The latter part allows
for the import to be robust when e.g. reloading numpy which may
cause add_docstring to run twice.

This commit also changes the documentation stubs for scalar
attributes and errors to instead refer to the array attribute.
These are typically inherited, and thus it is not useful if
they refer to some "virtual attribute"

Closes gh-16209 and gh-14384

@rgommers to get this fixed for 1.19 quickly, had a look at the C-side fix, which unfportunately flushed out the exact thing I thought we might miss...

seberg · 2020-05-14T18:05:15Z

numpy/core/_add_newdocs.py

+           refer_to_array_attribute('T', method=False))
+
+add_newdoc('numpy.core.numerictypes', 'generic',
+           refer_to_array_attribute('base', method=False))


For reviewers: If I had a typo in the attribute, this would crash. So the only thing is to not forget/double an attribute by accident.

seberg · 2020-05-14T18:06:42Z

numpy/core/src/multiarray/scalartypes.c.src

    {"flat",
        (getter)gentype_flat_get,
-        (setter)0,
-        "a 1-d view of scalar",


Some of these may actually have been better docstrings in a sense. OTOH, refering the ndarray attribute is nice.

seberg

And here the link to the artifact to see the actual doc changes quicker:

https://399-5652112-gh.circle-artifacts.com/0/doc/build/html/reference/generated/numpy.generic.html?highlight=generic#numpy.generic

seberg · 2020-05-15T00:25:30Z

numpy/tests/test_reloading.py

+    p = Process(target=try_full_reload)
+    p.start()
+    p.join()
+    assert p.exitcode == 0


Hmm, this fails on azure/windows with FileNotFoundError: [Errno 2] No such file or directory: 'D:\\a\\1\\s\\build\\test\\runtests.py' Just skip on windows:

https://dev.azure.com/numpy/numpy/_build/results?buildId=9919&view=logs&j=25bb66cf-4a16-533e-490c-8da4b5a3ec04&t=97446ee6-ebce-5662-24e5-f82eed312005&l=217

other option is maybe try https://docs.python.org/3/library/multiprocessing.html#contexts-and-start-methods muliprocessing.set_start_method('spawn')

not sure if this is already the default on windows for the version we are using

Maybe

txt = r""" import sys import numpy as np for k in list(sys.modules.keys()): if "numpy" in k: del sys.modules[k] import numpy as np """ p = subprocess.run([sys.executable, '-c', textwrap.dedent(txt)]) assert p.returncode == 0

Thanks matti, will just use that, seems simplest.

I changed the docs to link ndarray.attribute, did not realize the ~ would remove the ndarray as well.

rgommers · 2020-05-15T15:38:27Z

@rgommers to get this fixed for 1.19 quickly, had a look at the C-side fix, which unfportunately flushed out the exact thing I thought we might miss...

Thanks @seberg, happy for you to take this over - I'm pretty short on time on weekdays.

anirudh2290

Overall LGTM ! 👍

anirudh2290 · 2020-05-15T19:11:12Z

numpy/core/src/multiarray/compiled_base.c

@@ -1482,7 +1482,7 @@ arr_add_docstring(PyObject *NPY_UNUSED(dummy), PyObject *args)
        if (!(doc)) {                                                   \


just a nit and unrelated to this change, but the do {...} while(0) can probably be removed.
another nit and again unrelated to this change, but when i read this, i was searching for new in the code, but didnt realize that new was defined inside the macro, which made this a bit confusing.

Yeah, lets move the new in front of the macro, also removes the need for the macro cast...

I tried to clean it up, which deletes a bunch of other code since Python seems to define those types in the header now (if not, I assume CI will fail).

eric-wieser · 2020-05-15T19:47:42Z

numpy/core/src/multiarray/compiled_base.c

@@ -1507,7 +1507,8 @@ arr_add_docstring(PyObject *NPY_UNUSED(dummy), PyObject *args)
        PyObject *doc_attr;

        doc_attr = PyObject_GetAttrString(obj, "__doc__");
-        if (doc_attr != NULL && doc_attr != Py_None) {
+        if (doc_attr != NULL && doc_attr != Py_None &&
+                (PyUnicode_Compare(doc_attr, obj) != 0)) {
            PyErr_Format(PyExc_RuntimeError, "object %s", msg);


Suggested change

PyErr_Format(PyExc_RuntimeError, "object %s", msg);

if (PyErr_Occurred()) {

return NULL;

}

PyErr_Format(PyExc_RuntimeError, "object %s", msg);

Right, seems there is also a %R or so missing to tell you which object we are talking about...

Changed, the check was actually incorrect, it should have been str not obj, its fixed now (tested manually with np.add_newdoc(np.take_along_axis, "wrong docstring"))

BUT: We currently expose even np.add_newdoc as top-level... So I reverted the try/except change there, since it could be very disruptive in theory (i.e. something not importing). My tendency is to give a warning instead as soon as 1.20 is out.

numpy/tests/test_reloading.py

mattip · 2020-05-17T22:49:23Z

You should skip the failing test on PyPy, you are running into gh-10167 where the call to add_newdocs should be replaced by generated C code that is compiled in and loaded before PyType_Ready

seberg · 2020-05-17T22:58:43Z

Thanks, hadn't noticed the tests were failing...

mattip · 2020-05-18T06:22:23Z

Now the PYOPTIMIZE=2 CI run is failing the TestAddDocstring.test_different_docstring_fail tests.

seberg · 2020-05-18T15:31:05Z

Dang, should have realized, passing now...

mattip · 2020-05-18T16:07:22Z

The new tp_doc assigments seem to be working. numpy.generice.base now looks something like this:

numpy.generic.base

attribute

generic.base:

Scalar attribute identical to the corresponding array attribute.

Please see ndarray.base.

mattip · 2020-05-18T16:07:43Z

LGTM

seberg · 2020-05-19T14:23:14Z

Would be nice to finish this. @charris would you prefer if I split this up to make the backport minimal?

charris · 2020-05-19T15:09:35Z

Minimal backports are always nice :) I will probably do an rc2 next weekend as this appears a bit risky to me.

seberg · 2020-05-19T16:31:28Z

@charris I think it looks larger then it is and is only doc attaching, but still. The first commit is now the minimal fix. The TST commit is probably just as fine.

mattip · 2020-05-19T18:19:58Z

Windows is complaining "ValueError: path is on mount 'D:', start on mount 'C:'", which was supposed to be avoided. Is this rebased off master?

seberg · 2020-05-19T18:24:42Z

I did not rebase again, thought the commits apply easier then maybe, let me rebase...

This is technically not a bug, but some IDEs and IPython have autoreload magic which can mean that NumPy gets reloaded a second time. This is not safe, but when it happens ignoring that an identical docstring is already attached fixes the issue.

Previously some of these were not set, because they were already set on the C-level. Remove the duplicate message and replace it instead with a forward to the ``ndarray`` attribute/method. These docstrings are inherited by the actual scalars and thus the "virtual class" note was misleading.

seberg · 2020-05-19T18:58:46Z

The windows builds seem super flaky today... but maybe time will make that go away...

mattip · 2020-05-20T11:22:51Z

Close/Reopen to trigger CI merge with master

mattip · 2020-05-20T11:23:39Z

LGTM. Can merge if CI passes

mattip · 2020-05-20T12:35:04Z

numpy/lib/tests/test_function_base.py

+    # Test should possibly be moved, but it also fits to be close to
+    # the newdoc tests...
+    @pytest.mark.skipif(sys.flags.optimize == 2, reason="Python running -OO")
+    def test_add_same_docstring(self):


This fails on PyPy since np.ndarray.flat.__doc__ is None. Skipping would be the easiest fix I guess

Thanks... dang, I had lost the fixups for this file in the rebase and forgot about this one...

Its not quite the right file, but close to newdoc seemed sensible and we do not have a "right" file right now...

The old pointers are now provided by the C-API and the macros seemed a bit confusing since ``new`` appears from nowhere being defined in the macro.

mattip · 2020-05-20T15:16:24Z

Thanks @seberg. ba14823 is the minimal change + test to be backported to fix this in 1.19, correct?

seberg commented May 14, 2020

View reviewed changes

seberg force-pushed the reimport-do-not-set-docs branch from 9d51897 to 3032c3b Compare May 14, 2020 18:09

charris added 00 - Bug 04 - Documentation component: numpy._core labels May 14, 2020

seberg force-pushed the reimport-do-not-set-docs branch from 3032c3b to 6d460e6 Compare May 15, 2020 00:13

seberg commented May 15, 2020

View reviewed changes

seberg force-pushed the reimport-do-not-set-docs branch from 6d460e6 to 8acff73 Compare May 15, 2020 13:47

rgommers mentioned this pull request May 15, 2020

BUG: Make attaching docstrings to __array_function__ funcs more robust #16209

Closed

anirudh2290 approved these changes May 15, 2020

View reviewed changes

eric-wieser reviewed May 15, 2020

View reviewed changes

numpy/tests/test_reloading.py Outdated Show resolved Hide resolved

seberg force-pushed the reimport-do-not-set-docs branch from 6f558df to 3853a37 Compare May 15, 2020 20:52

seberg mentioned this pull request May 17, 2020

ENH: Warn when reloading numpy or using numpy in sub-interpreter #16241

Merged

seberg force-pushed the reimport-do-not-set-docs branch from 3853a37 to 397b7d8 Compare May 17, 2020 22:58

seberg force-pushed the reimport-do-not-set-docs branch from 397b7d8 to 061e42c Compare May 18, 2020 00:29

seberg force-pushed the reimport-do-not-set-docs branch from 061e42c to c79fca6 Compare May 18, 2020 13:07

charris added the 09 - Backport-Candidate PRs tagged should be backported label May 18, 2020

charris modified the milestones: 1.19.1 release, 1.19.0 release May 18, 2020

seberg force-pushed the reimport-do-not-set-docs branch 2 times, most recently from 7ff0586 to fc03476 Compare May 19, 2020 16:29

seberg force-pushed the reimport-do-not-set-docs branch from fc03476 to 1b98976 Compare May 19, 2020 17:29

seberg added 2 commits May 19, 2020 13:25

seberg force-pushed the reimport-do-not-set-docs branch from 1b98976 to 15d5619 Compare May 19, 2020 18:25

seberg closed this May 19, 2020

seberg reopened this May 19, 2020

mattip closed this May 20, 2020

mattip reopened this May 20, 2020

mattip reviewed May 20, 2020

View reviewed changes

seberg added 2 commits May 20, 2020 08:55

TST: Add a test for np.add_docstring

d758e58

Its not quite the right file, but close to newdoc seemed sensible and we do not have a "right" file right now...

MAINT: Simplify logic in add_docstring

ffdce8b

The old pointers are now provided by the C-API and the macros seemed a bit confusing since ``new`` appears from nowhere being defined in the macro.

seberg force-pushed the reimport-do-not-set-docs branch from 15d5619 to ffdce8b Compare May 20, 2020 13:55

mattip merged commit 78d7ab3 into numpy:master May 20, 2020

charris mentioned this pull request May 22, 2020

BUG: Allow attaching documentation twice in add_docstring #16344

Merged

charris removed the 09 - Backport-Candidate PRs tagged should be backported label May 22, 2020

charris removed this from the 1.19.0 release milestone May 22, 2020

jklenzing mentioned this pull request May 29, 2020

TST: numpy versions > 1.15.4 don't work in CI aburrell/ocbpy#50

Closed

seberg mentioned this pull request Jul 17, 2020

'RuntimeError: implement_array_function method already has a docstring' after matplotlib installation #15563

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG,DOC: Allow attach docs twice but error if wrong #16239

BUG,DOC: Allow attach docs twice but error if wrong #16239

seberg commented May 14, 2020

seberg May 14, 2020

seberg May 14, 2020

seberg left a comment

seberg May 15, 2020

anirudh2290 May 15, 2020

anirudh2290 May 15, 2020

mattip May 15, 2020 •

edited

seberg May 15, 2020

rgommers commented May 15, 2020

anirudh2290 left a comment

anirudh2290 May 15, 2020

seberg May 15, 2020

seberg May 15, 2020

eric-wieser May 15, 2020 •

edited by seberg

seberg May 15, 2020

seberg May 15, 2020

mattip commented May 17, 2020

seberg commented May 17, 2020

mattip commented May 18, 2020

seberg commented May 18, 2020

mattip commented May 18, 2020

mattip commented May 18, 2020

seberg commented May 19, 2020

charris commented May 19, 2020

seberg commented May 19, 2020

mattip commented May 19, 2020

seberg commented May 19, 2020

seberg commented May 19, 2020

mattip commented May 20, 2020

mattip commented May 20, 2020

mattip May 20, 2020

seberg May 20, 2020

mattip commented May 20, 2020

		@@ -1482,7 +1482,7 @@ arr_add_docstring(PyObject NPY_UNUSED(dummy), PyObject args)
		if (!(doc)) { \

BUG,DOC: Allow attach docs twice but error if wrong #16239

BUG,DOC: Allow attach docs twice but error if wrong #16239

Conversation

seberg commented May 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seberg left a 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

mattip May 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgommers commented May 15, 2020

anirudh2290 left a 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

eric-wieser May 15, 2020 • edited by seberg

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattip commented May 17, 2020

seberg commented May 17, 2020

mattip commented May 18, 2020

seberg commented May 18, 2020

mattip commented May 18, 2020

mattip commented May 18, 2020

seberg commented May 19, 2020

charris commented May 19, 2020

seberg commented May 19, 2020

mattip commented May 19, 2020

seberg commented May 19, 2020

seberg commented May 19, 2020

mattip commented May 20, 2020

mattip commented May 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattip commented May 20, 2020

mattip May 15, 2020 •

edited

eric-wieser May 15, 2020 •

edited by seberg