pybind / pybind11

Seamless operability between C++11 and Python
https://pybind11.readthedocs.io/
Other
15.39k stars 2.07k forks source link

fix(types): remove newlines from docstring signature #4735

Closed JeanElsner closed 1 year ago

JeanElsner commented 1 year ago

Description

Addresses issue https://github.com/pybind/pybind11/issues/4734 by removing all newlines from function signature docstring.

Suggested changelog entry:

Enforce single line docstring signatures.
rwgk commented 1 year ago

Everything works here now. I'll try to run this through global testing asap.

rwgk commented 1 year ago

Good news: the global testing is fine (no surprises).

I think with added tests this PR will be good.

(For myself, in case I need to backtrack later, this is the internal ID for the global testing: OCL:546641582:BASE:546634977:1688902402537:16f03b9e)

rwgk commented 1 year ago

@antonkesy approved these changes 7 hours ago

We're waiting for unit tests.

rwgk commented 1 year ago

I marked this as Draft. Please mark as Ready for review after adding unit tests.

rwgk commented 1 year ago

Did you find out already how to deal with this?

ModuleNotFoundError: No module named 'numpy'

See e.g. test_stl_binders.py:

def test_vector_buffer_numpy():
    np = pytest.importorskip("numpy")
JeanElsner commented 1 year ago

Did you find out already how to deal with this?

ModuleNotFoundError: No module named 'numpy'

I don't quite understand why some of the checks are failing now. I'm only using Eigen types in test_eigen_matrix which already imports numpy at module level. My only other modifications are in test_kwargs_and_defaults and there I didn't use any Eigen/numpy types. Any support would be really appreciated.

rwgk commented 1 year ago

I looked at the changes and indeed it's not immediately obvious why there is the numpy import error. Possibly it's only a secondary failure. Wild guess: 1. there is a bug in the new function, 2. the new function runs when pytest collects the tests to be run, 3. it just so happens that the numpy import stumbles over some deeper problem.

My usual approach to get certainty is to 0. not think to much, 1. experiment a lot (requires a little thinking what to try next).

The first thing I'd try: #ifdef out the entire body of the new function, and simply return text as a std::string. Result expected:

True or false? I'd go from there.

Debugging via GitHub actions is slow and cumbersome. I'd try a little to reproduce the problem locally, maybe in a docker container.

JeanElsner commented 1 year ago

I could reproduce the error in Docker and it's a bit convoluted. It turns out that if I set the default value to an Eigen type, numpy is imported by the pybind11_tests CPython module (so before pytest.importorskip is executed in the Python test). If I remove the Eigen defaults or simply install numpy the tests run successfully.

Is there a way to check for numpy in C++ similar to pytest.importorskip? Otherwise I would remove those tests and only test with custom types (none of the existing numpy/Eigen tests cases test defaults either).

rwgk commented 1 year ago

if I set the default value to an Eigen type, numpy is imported by the pybind11_tests

Oh...

I see scipy imports in the eigen code, but not numpy.

This may not be the most important question, but to get a better understanding for best judgement, do you know where the numpy import comes from?

JeanElsner commented 1 year ago

I think it's simply scipy.sparse imported in the Eigen code that in turn imports numpy.

rwgk commented 1 year ago

I think it's simply scipy.sparse imported in the Eigen code that in turn imports numpy.

That would be really odd: AFAIK scipy cannot be installed without installing numpy first. At least that seems highly likely for GitHub Actions jobs.

Jumping ahead:

I wonder if we could try importing numpy from C++ and .def() the new functions only if that succeeds.

I wouldn't be surprised if that runs into weird difficulties, but I'd try.

Another idea would be to add a -DPYBIND11_TESTS_HAVE_NUMPY via ci.yml somehow and test for that from C++. We don't need to cover a lot of platforms, just one would be a good start.

Giving up is another option, but similar to before, I'd try at least a little bit more.

JeanElsner commented 1 year ago

You are right about the scipy dependency of course. Actually, the tests succeed without scipy but only numpy installed. I will investigate further and try your suggestions, thanks!

rwgk commented 1 year ago

The two CI failures are unrelated: one is PR #4767, the other one a frequent flake (test_iostream).

rwgk commented 1 year ago

If you get a chance, could you please git merge master again?

rwgk commented 1 year ago

(FYI: waiting a bit for feedback from other maintainers)

rwgk commented 1 year ago

There was a failure I've not seen before:

docs/readthedocs.org:pybind11 — Read the Docs build failed!

https://readthedocs.org/projects/pybind11/builds/21602133/

Probably transient, because it's working for #4786. I'll close-open this PR to re-trigger GHA.

rwgk commented 1 year ago

No luck: docs/readthedocs.org:pybind11 — Read the Docs build failed!

I cancelled the other GHA to not burn CPU time unnecessarily.

I'll wait a couple hours then try again, but @JeanElsner if you have any clues or suspicions please let me know.

rwgk commented 1 year ago

I took a super quick look, this PR doesn't touch any rst or config files, there is no obvious reason why readthedocs would fail AFAICT.

rwgk commented 1 year ago

Trying GHA again without any changes in this PR.

rwgk commented 1 year ago

Failing on master already.

Strangely, we didn't have that error under #4786: https://readthedocs.org/projects/pybind11/builds/21599337/

Maybe it broke shortly after?

rwgk commented 1 year ago

Hi @JeanElsner I fixed the readthedocs problem (#4789). Could you please git merge master, git push again?

rwgk commented 1 year ago

No feedback from any other maintainer for 7 days (I sent two requests for feedback). Merging.