Expand std::string_view support to str, bytes, memoryview by jagerman · Pull Request #3521 · pybind/pybind11

jagerman · 2021-12-01T15:22:24Z

Description

Allows constructing a str or bytes implicitly from a string_view; this is essentially a small shortcut allowing a caller to write py::bytes{sv} rather than py::bytes{sv.data(), sv.size()}.

For py::str this also allows std::u8string_view, but not for py::bytes because that didn't seem entirely appropriate to me.
Allows implicit conversion to std::string_view from py::bytes—this plugs a current hole where there's no simple way to get such a view of the bytes without copying it (or resorting to Python API calls).

(This is not done for str because when the str contains unicode we have to allocate to a temporary and so there might not be some string data we can properly view without owning.)
Allows memoryview::from_memory to accept a string_view. As with the other from_memory calls, it's entirely your responsibility to keep it alive.

This also required moving the string_view availability detection into detail/common.h because this PR needs it in pytypes.h, which is higher up the include chain than cast.h where it was being detected currently.

Suggested changelog entry:

* Make str/bytes/memoryview more interoperable with ``std::string_view``.

rwgk

Nice!

rwgk · 2021-12-01T16:00:06Z

The 2.7 ubuntu failure probably just needs a leading u (u"i'm a string", a trick we use in many places).

I've never seen the ITERATOR_DEBUG_LEVEL failure before.

jagerman · 2021-12-02T13:35:25Z

Squashed it.

Skylion007 · 2021-12-02T15:29:12Z

@rwgk Does this pass the Google Test suite?

jagerman · 2021-12-02T17:29:42Z

I don't understand why the 3.6 - windows-latest build here started failing. Is that a known flakey build or something?

rwgk · 2021-12-02T18:10:05Z

I don't understand why the 3.6 - windows-latest build here started failing. Is that a known flakey build or something?

Yes, that's our most common known flake, safe to ignore. It's a lot better than it used to be before PR #2995, but it's still not stable.

There are other common flakes: 1. print from destructor; 2. various transient issues installing or downloading dependencies.

rwgk · 2021-12-02T18:11:17Z

@rwgk Does this pass the Google Test suite?

I'll initiate this now (will take several hours).

henryiii

Happy when Google's happy. :)

rwgk · 2021-12-02T23:14:18Z

Happy when Google's happy. :)

I missed the 12:00 global testing opportunity, but got this PR into the 16:00 batch. Results are expected about 4-5 hours later.

I did comprehensive manual testing before sending this PR for global testing, including testing with ASAN. No issues.

rwgk · 2021-12-03T00:05:35Z

This PR didn't make it through the basic "smoke check" :-(
Below is one breakage. I still have to try to understand myself.
IIRC we recently had some trouble with tensorflow::tstring already, I forget what exactly ...

EDIT: What I had in mind was a PyCLIF issue, not pybind11: google/clif@ef5fa11
(Not sure what we can learn from it. Just mentioning for completeness.)

EDIT: This is the tensorflow::tstring code that causes the issue: https://github.com/tensorflow/tensorflow/blob/289850292c2b9761055415b91c2a6c0f924780f0/tensorflow/core/platform/tstring.h#L142

Error message:

third_party/tensorflow/python/lib/io/file_io_wrapper.cc:316:21: error: ambiguous conversion for functional-style cast from 'tensorflow::tstring' to 'py::bytes'
             return py::bytes(result);
                    ^~~~~~~~~~~~~~~~
./third_party/pybind11/include/pybind11/detail/../pytypes.h:1174:5: note: candidate constructor
    bytes(const std::string &s) : bytes(s.data(), s.size()) { }
    ^
./third_party/pybind11/include/pybind11/detail/../pytypes.h:1189:5: note: candidate constructor
    bytes(std::string_view s) : bytes(s.data(), s.size()) { }
    ^
1 error generated.

While compiling:

      .def("read",
           [](BufferedInputStream* self, int64_t bytes_to_read) {
             py::gil_scoped_release release;
             tensorflow::tstring result;
             const auto status = self->ReadNBytes(bytes_to_read, &result);
             if (!status.ok() && !tensorflow::errors::IsOutOfRange(status)) {
               result.clear();
               tensorflow::MaybeRaiseRegisteredFromStatusWithGIL(status);
             }
             py::gil_scoped_acquire acquire;
             return py::bytes(result);
           })

1. Allows constructing a str or bytes implicitly from a string_view; this is essentially a small shortcut allowing a caller to write `py::bytes{sv}` rather than `py::bytes{sv.data(), sv.size()}`. 2. Allows implicit conversion *to* string_view from py::bytes -- this saves a fair bit more as currently there is no simple way to get such a view of the bytes without copying it (or resorting to Python API calls). (This is not done for `str` because when the str contains unicode we have to allocate to a temporary and so there might not be some string data we can properly view without owning.) 3. Allows `memoryview::from_memory` to accept a string_view. As with the other from_memory calls, it's entirely your responsibility to keep it alive. This also required moving the string_view availability detection into detail/common.h because this PR needs it in pytypes.h, which is higher up the include chain than cast.h where it was being detected currently.

This change is known to fix the `tensorflow::tstring` issue reported under pybind#3521 (comment) TODO: Minimal reproducer for the `tensorflow::tstring` issue.

rwgk · 2021-12-03T01:22:32Z

Trying a fix. I don't know if this is the best approach, but at least it fixes the failures in the wild (at least some; I still have to try the global testing again).

The force push was needed because I also rebased on master.

Error without the enable_if trick: ``` /usr/local/google/home/rwgk/forked/pybind11/tests/test_builtin_casters.cpp:169:16: error: ambiguous conversion for functional-style cast from 'TypeWithBothOperatorStringAndStringView' to 'py::bytes' return py::bytes(TypeWithBothOperatorStringAndStringView()); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/detail/../pytypes.h:1174:5: note: candidate constructor bytes(const std::string &s) : bytes(s.data(), s.size()) { } ^ /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/detail/../pytypes.h:1191:5: note: candidate constructor bytes(std::string_view s) : bytes(s.data(), s.size()) { } ^ ```

rwgk · 2021-12-03T07:26:03Z

I think this will work now, pending full global testing; just initiated. Results when I wake up (hopefully).

I already verified that all failing targets from the previous sample run pass with the latest version of this PR.

jagerman · 2021-12-03T15:23:38Z

I think this will work now, pending full global testing; just initiated. Results when I wake up (hopefully).

str() will have the same issue; I've pushed an update to apply the workaround there, too.

jagerman · 2021-12-03T15:25:45Z

I'd kind of prefer that, in the case of ambiguous conversions, we were preferring string_view to string (because it likely avoids an extra allocation), but I don't really see a nice way to do that without introducing potential breakage for existing code.

rwgk · 2021-12-03T16:55:25Z

I'd kind of prefer that, in the case of ambiguous conversions, we were preferring string_view to string (because it likely avoids an extra allocation), but I don't really see a nice way to do that without introducing potential breakage for existing code.

Initially I tried #ifdefing out the std::string constructor, which also fixed the tensorflow::tstring issue in the wild, but broke existing unit tests. With a pushing-and-shoving mentality I tried to rescue it, but quickly got into a mess and gave up.

rwgk · 2021-12-03T17:01:33Z

The one CI failure is a flake (Run jwlawson/actions-setup-cmake@v1.11 Error: connect ETIMEDOUT 140.82.114.6:443).

Google-global testing was successful (it did not run into the str issue fixed here by 5709ccf in the meantime).

Definitely good to merge Jason!

jagerman force-pushed the more-string-view branch from fd737d5 to d7c5d27 Compare December 1, 2021 15:45

Skylion007 reviewed Dec 1, 2021

View reviewed changes

Comment thread include/pybind11/pytypes.h Outdated

rwgk approved these changes Dec 1, 2021

View reviewed changes

Comment thread include/pybind11/pytypes.h

rwgk reviewed Dec 1, 2021

View reviewed changes

Comment thread include/pybind11/pytypes.h

Skylion007 reviewed Dec 1, 2021

View reviewed changes

Comment thread tests/test_builtin_casters.py Outdated

Skylion007 reviewed Dec 1, 2021

View reviewed changes

Comment thread include/pybind11/detail/common.h Outdated

rwgk approved these changes Dec 1, 2021

View reviewed changes

Comment thread include/pybind11/pytypes.h

Comment thread include/pybind11/pytypes.h

jagerman force-pushed the more-string-view branch 2 times, most recently from 0bf48b2 to 0c7d446 Compare December 2, 2021 13:35

Skylion007 reviewed Dec 2, 2021

View reviewed changes

Comment thread include/pybind11/detail/common.h Outdated

Skylion007 approved these changes Dec 2, 2021

View reviewed changes

Skylion007 requested a review from henryiii December 2, 2021 15:28

henryiii approved these changes Dec 2, 2021

View reviewed changes

jagerman and others added 3 commits December 2, 2021 17:19

Move string_view include to pytypes.h

f56dc86

CI-testing a fix for the "ambiguous conversion" issue.

add6628

This change is known to fix the `tensorflow::tstring` issue reported under pybind#3521 (comment) TODO: Minimal reproducer for the `tensorflow::tstring` issue.

rwgk force-pushed the more-string-view branch from c207063 to add6628 Compare December 3, 2021 01:19

rwgk added 3 commits December 2, 2021 17:28

Make clang-tidy happy (hopefully).

7f7ddaa

Adding missing NOLINTNEXTLINE

3428195

Also apply ambiguous conversion workaround to str()

5709ccf

jagerman merged commit b4939fc into pybind:master Dec 3, 2021

github-actions Bot added the needs changelog Possibly needs a changelog entry label Dec 3, 2021

henryiii removed the needs changelog Possibly needs a changelog entry label Dec 21, 2021

VemundH mentioned this pull request Oct 23, 2022

[BUG] char8_t/u8string support broken since 2.9 #4277

Closed

rwgk mentioned this pull request Feb 10, 2023

FWD pybind11 google/pybind11clif#3521

Closed

Conversation

jagerman commented Dec 1, 2021 • edited by henryiii Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Suggested changelog entry:

Uh oh!

Uh oh!

rwgk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rwgk commented Dec 1, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jagerman commented Dec 2, 2021

Uh oh!

Uh oh!

Skylion007 commented Dec 2, 2021

Uh oh!

jagerman commented Dec 2, 2021

Uh oh!

rwgk commented Dec 2, 2021

Uh oh!

rwgk commented Dec 2, 2021

Uh oh!

henryiii left a comment

Choose a reason for hiding this comment

Uh oh!

rwgk commented Dec 2, 2021

Uh oh!

rwgk commented Dec 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwgk commented Dec 3, 2021

Uh oh!

rwgk commented Dec 3, 2021

Uh oh!

jagerman commented Dec 3, 2021

Uh oh!

jagerman commented Dec 3, 2021

Uh oh!

rwgk commented Dec 3, 2021

Uh oh!

rwgk commented Dec 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jagerman commented Dec 1, 2021 •

edited by henryiii

Loading

rwgk commented Dec 3, 2021 •

edited

Loading