Skip to content
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

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

Merged
merged 30 commits into from
Aug 15, 2023

Conversation

JeanElsner
Copy link
Contributor

Description

Addresses issue #4734 by removing all newlines from function signature docstring.

Suggested changelog entry:

Enforce single line docstring signatures.

include/pybind11/pybind11.h Outdated Show resolved Hide resolved
@rwgk
Copy link
Collaborator

rwgk commented Jul 9, 2023

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

@rwgk
Copy link
Collaborator

rwgk commented Jul 9, 2023

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
Copy link
Collaborator

rwgk commented Jul 13, 2023

@antonkesy approved these changes 7 hours ago

We're waiting for unit tests.

@rwgk rwgk marked this pull request as draft July 13, 2023 16:21
@rwgk
Copy link
Collaborator

rwgk commented Jul 13, 2023

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

@rwgk
Copy link
Collaborator

rwgk commented Jul 24, 2023

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
Copy link
Contributor Author

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
Copy link
Collaborator

rwgk commented Jul 25, 2023

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:

  • no more import errors, i.e. pytest should actually run all tests
  • only the new test fails

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
Copy link
Contributor Author

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
Copy link
Collaborator

rwgk commented Jul 25, 2023

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
Copy link
Contributor Author

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

@rwgk
Copy link
Collaborator

rwgk commented Jul 25, 2023

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
Copy link
Contributor Author

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!

Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the making this a high quality PR!


// Replace characters in whitespaces array with spaces and squash consecutive spaces
while (*text != '\0') {
if (strchr(whitespaces, *text)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please make this std::strchr?

Elsewhere we have std::strcmp consistently, and we already have #include <cstring>.

@@ -52,6 +52,37 @@ PYBIND11_WARNING_DISABLE_MSVC(4127)

PYBIND11_NAMESPACE_BEGIN(detail)

inline std::string replace_newlines_and_squash(const char *text) {
const char *whitespaces = " \t\n\r\f\v";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please sprinkle \r, \f, \v into the tests below?

What we want is that a test fails if any of the characters here is accidentally lost. (Imagine a silly typo slipping in while refactoring.)

@rwgk
Copy link
Collaborator

rwgk commented Aug 8, 2023

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

@rwgk rwgk marked this pull request as ready for review August 8, 2023 14:51
@rwgk
Copy link
Collaborator

rwgk commented Aug 8, 2023

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

@rwgk
Copy link
Collaborator

rwgk commented Aug 14, 2023

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 rwgk closed this Aug 14, 2023
@rwgk rwgk reopened this Aug 14, 2023
@rwgk
Copy link
Collaborator

rwgk commented Aug 14, 2023

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
Copy link
Collaborator

rwgk commented Aug 14, 2023

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
Copy link
Collaborator

rwgk commented Aug 15, 2023

Trying GHA again without any changes in this PR.

@rwgk rwgk closed this Aug 15, 2023
@rwgk rwgk reopened this Aug 15, 2023
@rwgk
Copy link
Collaborator

rwgk commented Aug 15, 2023

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
Copy link
Collaborator

rwgk commented Aug 15, 2023

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

@rwgk
Copy link
Collaborator

rwgk commented Aug 15, 2023

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

@rwgk rwgk merged commit b9359ce into pybind:master Aug 15, 2023
82 checks passed
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Aug 15, 2023
@JeanElsner JeanElsner deleted the jean/patch/argval branch August 15, 2023 18:22
rwgk pushed a commit that referenced this pull request Sep 14, 2023
* Bug fix: `result[0]` called if `result.empty()`

* Add unit test that fails without the fix.
@henryiii henryiii changed the title Remove newlines from docstring signature fix(types): remove newlines from docstring signature Nov 15, 2023
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Mar 27, 2024
rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Jun 12, 2024
* fix: Use lowercase builtin collection names (pybind#4833)

* Update render for buffer sequence and handle  (pybind#4831)

* fix: Add capitalize render name of `py::buffer` and `py::sequence`

* fix: Render `py::handle` same way as `py::object`

* tests: Fix tests `handle` -> `object`

* tests: Test capitaliation of `py::sequence` and `py::buffer`

* style: pre-commit fixes

* fix: Render `py::object` as `Any`

* Revert "fix: Render `py::object` as `Any`"

This reverts commit 7861dcf.

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Ralf W. Grosse-Kunstleve <[email protected]>

* fix: Missing typed variants of `iterator` and `iterable` (pybind#4832)

* Fix small bug introduced with PR pybind#4735 (pybind#4845)

* Bug fix: `result[0]` called if `result.empty()`

* Add unit test that fails without the fix.

* fix(cmake): correctly detect FindPython policy and better warning (pybind#4806)

* fix(cmake): support DEBUG_POSTFIX correctly (pybind#4761)

* cmake: split extension

Into suffix and debug postfix. Pybind11 is currently treating both as
suffix, which is problematic when something else defines the
DEBUG_POSTFIX because they will be concatenated.

pybind11_extension sets SUFFIX to _d.something and if DEBUG_POSTFIX is
set to _d.

    _d + _d.something = _d_d.something

The issue has been reported at:

pybind#4699

* style: pre-commit fixes

* fix(cmake): support postfix for old FindPythonInterp mode too

Signed-off-by: Henry Schreiner <[email protected]>

---------

Signed-off-by: Henry Schreiner <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Henry Schreiner <[email protected]>

* Avoid copy in iteration by using const auto & (pybind#4861)

This change is fixing a Coverity AUTO_CAUSES_COPY issues.

* Add 2 missing `throw error_already_set();` (pybind#4863)

Fixes oversights in PR pybind#4570.

* MAINT: Include `numpy._core` imports (pybind#4857)

* MAINT: Include numpy._core imports

* style: pre-commit fixes

* Apply review comments

* style: pre-commit fixes

* Add no-inline attribute

* Select submodule name based on numpy version

* style: pre-commit fixes

* Update pre-commit check

* Add error_already_set and simplify if statement

* Update .pre-commit-config.yaml

Co-authored-by: Ralf W. Grosse-Kunstleve <[email protected]>

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Ralf W. Grosse-Kunstleve <[email protected]>

* MAINT: Remove np.int_ (pybind#4867)

* chore(deps): update pre-commit hooks (pybind#4868)

* chore(deps): update pre-commit hooks

updates:
- [github.com/psf/black-pre-commit-mirror: 23.7.0 → 23.9.1](psf/black-pre-commit-mirror@23.7.0...23.9.1)
- [github.com/astral-sh/ruff-pre-commit: v0.0.287 → v0.0.292](astral-sh/ruff-pre-commit@v0.0.287...v0.0.292)
- [github.com/codespell-project/codespell: v2.2.5 → v2.2.6](codespell-project/codespell@v2.2.5...v2.2.6)
- [github.com/shellcheck-py/shellcheck-py: v0.9.0.5 → v0.9.0.6](shellcheck-py/shellcheck-py@v0.9.0.5...v0.9.0.6)
- [github.com/PyCQA/pylint: v3.0.0a7 → v3.0.0](pylint-dev/pylint@v3.0.0a7...v3.0.0)

* Update .pre-commit-config.yaml

* style: pre-commit fixes

* Update .pre-commit-config.yaml

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Henry Schreiner <[email protected]>

---------

Signed-off-by: Henry Schreiner <[email protected]>
Co-authored-by: Sergei Izmailov <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Henry Schreiner <[email protected]>
Co-authored-by: László Papp <[email protected]>
Co-authored-by: Oleksandr Pavlyk <[email protected]>
Co-authored-by: Mateusz Sokół <[email protected]>
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.

4 participants