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

[BUG] Clearer commitment to public / stable API? (most importantly, type_caster) #2763

Open
EricCousineau-TRI opened this issue Dec 31, 2020 · 5 comments
Labels
casters Related to casters, and to be taken into account when analyzing/reworking casters

Comments

@EricCousineau-TRI
Copy link
Collaborator

EricCousineau-TRI commented Dec 31, 2020

At present, type_caster is something that exists in the py::detail namespace, but may be overloaded in downstream applications, either via a macro, or more directly.

A naive downstream example (that I wrote 😬):
https://github.com/RobotLocomotion/drake/blob/v0.25.0/bindings/pydrake/common/wrap_pybind.h#L54-L114
https://github.com/RobotLocomotion/drake/blob/v0.25.0/bindings/pydrake/common/cpp_param_pybind.h#L149-L158

At present, my reading of py::detail is that it's generally internal implementation details, i.e. it's internal, not public API, and should not be expected to be stable.

It seems like the base contract of py::detail::type_caster may more-or-less be leaked into a public details. We may want to consider making this more public, and having some level of public stability for this. In this way, we could hoist it outside of py::detail, so we could keep that reserved for truly internal details.

Possibly related:

\cc @rwgk @YannickJadoul @rhaschke @wjakob

@EricCousineau-TRI EricCousineau-TRI changed the title [BUG] Clearer commitment to public / stabile API? (most importantly, type_caster) [BUG] Clearer commitment to public / stable API? (most importantly, type_caster) Dec 31, 2020
@YannickJadoul
Copy link
Collaborator

The docs acknowledge this, though, and explicitly say it is allowed: https://pybind11.readthedocs.io/en/stable/advanced/cast/custom.html

I would first focus on fixing a few things, then use what wa learned (e.g., what things would be nice to allow to be customized from the outside) to fuel a redesign? There's already a casters label to aggregate things towards this purpose ;-)

@YannickJadoul YannickJadoul added the casters Related to casters, and to be taken into account when analyzing/reworking casters label Dec 31, 2020
@rwgk
Copy link
Collaborator

rwgk commented Jan 1, 2021

The docs acknowledge this, though, and explicitly say it is allowed: https://pybind11.readthedocs.io/en/stable/advanced/cast/custom.html

Quoting from there: "In very rare cases, ..."

This premise seems far removed from reality (to avoid the word "wrong").
I'm seeing a good number of type casters in our code universe (combination of closed-source and OSS code).
My feeling is that pybind11::detail::type_caster counts as one of the externally perceived core identities of pybind11, so much so that changing it would make people think it's not pybind11 anymore.

Interesting experiment to get scientific numbers (and possibly also a way to clean house): rename pybind11::detail to pybind11::private_detail, then create aliases in pybind11:detail as needed until all our tests (millions) work again. That's probably a very good approximation for what the word relies on to be in pybind11:detail.

@YannickJadoul
Copy link
Collaborator

Oh, I definitely agree, there. And as far as I'm concerned, custom type_casters form an important customization point in pybind11 (mainly for power-users, though, in a "I once wrote this, now XYZ works" fashion).

This premise seems far removed from reality (to avoid the word "wrong").

I'm not convinced about that, though. These should be for rare, specialized cases where you need full control. Often, you can (should?) just wrap things with a lambda.

My main point in linking to that part of the docs that is that type_casters are explicitly in a grey zone, saying "this is detail so we reserve the right to break stuff" but since it's documented, it's also providing some guarantees (it's been ages since breaking, and the upgrade guide has always been clear on these!). Which I think is reasonably acceptable, since 1) the docs urge to do this sporadically, 2) these are power-user tools, and 3) it's not a repeated pattern (one caster applies to everything). So the risks are not too high and clearly marked on the box?

That's probably a very good approximation for what the word relies on to be in pybind11:detail.

To be fair, apart from casters (which are explicitly allowed, caveats about semi-unstable API included), nothing should depend on pybind11::detail. If things do, pybind11 is not to blame (i.e., `.

Moreover, ríght now, as we're planning to fix a bunch of holder and caster stuff after bumping into the limits of the current design, would be (IMO) the very worst time to commit to a fully public API.
My suggestion: let's 1) keep the type_caster API as stable as possible (as pybind11 has been doing), 2) keep discouraging custom casters as last-resort, low-level fix, after all other options have been exhausted, 3) fix things regarding holder and casters, and 4) learn from that to spur a redesign that can include a public caster API.

Summary: this is not me dismissing that this would be very nice to have on the longer term; it would definitely be and be an important step towards a maturing pybind11! But committing to the current API would be a grave mistake, I believe.

@rwgk
Copy link
Collaborator

rwgk commented Jan 1, 2021

This premise seems far removed from reality (to avoid the word "wrong").

I'm not convinced about that, though. These should be for rare,

I think I'll do the suggested experiment, to get actual numbers and see where we really stand.

Maybe I'm missing something (a sincere question)?
Given some C++ type that already has a native or otherwise existing Python counterpart (e.g. datetime for native, numpy for otherwise), is there a way to implement conversions of my C++ type from/to Python without using detail::type_caster?

@YannickJadoul
Copy link
Collaborator

I think I'll do the suggested experiment, to get actual numbers and see where we really stand.

That could definitely be interesting!

Given some C++ type that already has a native or otherwise existing Python counterpart (e.g. datetime for native, numpy for otherwise), is there a way to implement conversions of my C++ type from/to Python without using detail::type_caster?

Apart from py::class_, if you need a special conversion, in principle, yes, you could accept py::object in a lambda and try cast, there. In practice, I can see that's not always a perfect solution. But then it's hard to judge without particular use cases. The basic type_caster interface (even though it's in detail) has not really been broken, I believe, though, and would practically require pybind11 3.0 anyway to make a full backwards-compatible break?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
casters Related to casters, and to be taken into account when analyzing/reworking casters
Projects
None yet
Development

No branches or pull requests

3 participants