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

Add py::object casting example to embedding docs #2466

Merged
merged 5 commits into from
Sep 9, 2020

Conversation

kohr-h
Copy link
Contributor

@kohr-h kohr-h commented Sep 6, 2020

Some examples for auto-casting from py::object to subtypes. These were the simplest I could come up with; happy to take suggestions.

Closes: #1310

@@ -105,6 +105,25 @@ The two approaches can also be combined:
std::cout << message;
}

Typically, a call to a function in a `py::module` returns a generic `py::object`,
which can often be autmatically cast to a specialized pybind11 type:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Calling py::module::import returns a py::module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's why I wrote "typically" 😉
But maybe that's not true either. Is it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is true, at least from my reading:

static module import(const char *name) {

I think the "typically" can be dropped, because in C++, it is py::module (the C++ type itself).

@YannickJadoul
Copy link
Collaborator

Note: #2349 will probably turn this into an error if it's nót the type you say it is (but doing that now is already wrong and potentially dangerous). But maybe we should make this more clear?

Also, I'm not sure this should be added to the embedding-specific part. This can also be useful for extension modules, so I'd suggest adding it to https://pybind11.readthedocs.io/en/stable/advanced/pycpp/object.html ? (Not sure if that's the perfect place either, though.)

@kohr-h
Copy link
Contributor Author

kohr-h commented Sep 6, 2020

Note: #2349 will probably turn this into an error if it's nót the type you say it is (but doing that now is already wrong and potentially dangerous). But maybe we should make this more clear?

What do you mean by "this" (... into an error)? The code in the examples?
What's your suggestion for this PR, adding a note on using the correct type on the LHS, or auto if we don't care?

Also, I'm not sure this should be added to the embedding-specific part. This can also be useful for extension modules, so I'd suggest adding it to https://pybind11.readthedocs.io/en/stable/advanced/pycpp/object.html ? (Not sure if that's the perfect place either, though.)

Okay sure, would that be under "Casting back and forth", as implicit cast without calling obj.cast()?

@YannickJadoul
Copy link
Collaborator

What do you mean by "this" (... into an error)? The code in the examples?
What's your suggestion for this PR, adding a note on using the correct type on the LHS, or auto if we don't care?

No, those should be fine, actually. But this kind of approach will fail if the type is not the expected type (i.e., if an attribute is a list and you're assigning it to a dict). Currently, it might not fail immediately but maybe later (e.g. when setting with d[...] = ...;), but after #2349, it should fail immediately (if it gets merged, ofc).

For this PR, we might want to make clear that for e.g. py::str curdir_abs = path.attr("abspath")(path.attr("curdir"));, you need to be sure that curdir_abs is a str? Or do you think that's implicit?

Okay sure, would that be under "Casting back and forth", as implicit cast without calling obj.cast()?

Hmmm, maybe, but it would make that (now very succinct) section very long. What do you think about adding another section to the bottom?
I'm also fine with putting it somewhere else, if you can find something better. But I think this is more widely applicable than just for embedding :-)

@kohr-h
Copy link
Contributor Author

kohr-h commented Sep 6, 2020

What do you mean by "this" (... into an error)? The code in the examples?
What's your suggestion for this PR, adding a note on using the correct type on the LHS, or auto if we don't care?

No, those should be fine, actually. But this kind of approach will fail if the type is not the expected type (i.e., if an attribute is a list and you're assigning it to a dict). Currently, it might not fail immediately but maybe later (e.g. when setting with d[...] = ...;), but after #2349, it should fail immediately (if it gets merged, ofc).

For this PR, we might want to make clear that for e.g. py::str curdir_abs = path.attr("abspath")(path.attr("curdir"));, you need to be sure that curdir_abs is a str? Or do you think that's implicit?

Alright, got ya. I was actually assuming that the cast would fail early, but if that's not the case at the moment, a warning makes sense. If PR #2349 is likely to land, I will add a note that casting to the wrong type will lead to downstream errors, and later on to immediate cast errors.

Hmmm, maybe, but it would make that (now very succinct) section very long. What do you think about adding another section to the bottom?

Okay, maybe "Implicit casting" or something? I would put it right below the "Casting back and forth".

I'm also fine with putting it somewhere else, if you can find something better. But I think this is more widely applicable than just for embedding :-)

Yes I agree, embedding is only where I stumbled on it :-). I'll see where it fits best, but I suspect it will be somewhere under "Python types".

@kohr-h
Copy link
Contributor Author

kohr-h commented Sep 6, 2020

I've moved the stuff over to object.rst. Please see if the formulations make sense, I'm not too firm with C++ terminology.

Also a question: how should I use things like py::object in the documentation to make sure that a link to the API doc is created? I went for :class:`object` now, but it would be nicer to write something like `py::object` where the link is correctly generated nonetheless. Any ideas?
In other places of the doc, this isn't handled consistently either. Some links are dead, some are just code snippets (double backtick).

Copy link
Collaborator

@YannickJadoul YannickJadoul left a comment

Choose a reason for hiding this comment

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

Starts looking good to me! :-)

Also a question: how should I use things like py::object in the documentation to make sure that a link to the API doc is created? I went for :class:object now, but it would be nicer to write something like py::object where the link is correctly generated nonetheless. Any ideas?
In other places of the doc, this isn't handled consistently either. Some links are dead, some are just code snippets (double backtick).

As you say: it's not handled consistently. But it's nice to have this link there, so whichever works best with respect to that; :class:`object` then, probably? (Just `object` doesn't work?)

Also, you could report an issue on this, if there's broken links. It won't be at the top of the list of things to do, but it's useful to know!

docs/advanced/pycpp/object.rst Outdated Show resolved Hide resolved
docs/advanced/pycpp/object.rst Outdated Show resolved Hide resolved
docs/advanced/pycpp/object.rst Outdated Show resolved Hide resolved
docs/advanced/pycpp/object.rst Outdated Show resolved Hide resolved
@kohr-h
Copy link
Contributor Author

kohr-h commented Sep 7, 2020

As you say: it's not handled consistently. But it's nice to have this link there, so whichever works best with respect to that; :class:object then, probably? (Just object doesn't work?)

I suppose it would. Sphinx would probably do a best guess, starting from the primary cpp domain. May depend on whether :any: domain is activated in the config, though.

Also, you could report an issue on this, if there's broken links. It won't be at the top of the list of things to do, but it's useful to know!

It seems like only a few links actually work, so an issue would cover the majority of documentation 😅

equivalent to the Python code ``env = list(os.environ)`` that produces a
list of the dict keys.

.. TODO: Adapt text once PR #2349 has landed
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great, thanks! :-)

docs/advanced/pycpp/object.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@YannickJadoul YannickJadoul 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, @kohr-h! Looks good to me :-)

@YannickJadoul
Copy link
Collaborator

@bstaletic, @henryiii, @EricCousineau-TRI: I think you might all be interested in this? If so, a quick look would be appreciated, and then this could go in soon :-)

Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Looks great!

FWIW I typically try to minimize how much Python logic I write using pybind11s interface API.

If I find myself writing more than a few if statements, more than say 15 lines, or with more complex semantics, I try to pass it directly to Python code.

Example from our codebase:
https://github.com/RobotLocomotion/drake/blob/8581223802b7790504529b9d66f1f1aab6eaaee9/bindings/pydrake/pydrake_pybind.h#L134-L139

@henryiii henryiii merged commit fbc7563 into pybind:master Sep 9, 2020
@henryiii
Copy link
Collaborator

henryiii commented Sep 9, 2020

Thanks!

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.

Convert py::object Numpy array to py::array
5 participants