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 support for custom call policies #767

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

oremanj
Copy link
Contributor

@oremanj oremanj commented Oct 24, 2024

This PR adds a new function annotation, nb::call_policy<Policy>(), which causes static methods of the user-provided Policy to be invoked before and after the bound C++ function. Unlike call_guard, the call policy methods operate on the Python object level and have access to the function arguments (precall can even modify them) and return value. This allows various esoteric user customizations that might be desirable in certain circumstances, with a minimal impact on the footprint of nanobind itself.

Motivating use case: a number of the types of interest in my application support a lightweight callback mechanism. The callback type we use is trivially copyable, so it can't manage ownership of a Python object directly. A custom call policy allows us to automatically take out a reference to the underlying Python callable when a new callback is subscribed, and drop the reference when it's unsubscribed. Allowing precall() to modify the argument list permits normalization (letting equal-but-distinct Python callables map to identical C++ callback objects, so that the C++ unsubscribe logic can find the callback that was originally subscribed).

While working on this I noticed that a function annotated with nb::keep_alive will leak a reference to its return value if the keep_alive can't be set up. I worked around the analogous issue for call policies, but didn't change the implementation of keep_alive in case this is a deliberate performance tradeoff.

@wjakob
Copy link
Owner

wjakob commented Nov 3, 2024

Hi @oremanj,

(Just got back from a 2-week vacation, hence the delay)

this looks like a neat feature and generalization of keep_alive. I am wondering if you would expect any run-time overhead on existing extensions? From a quick skim, I would say that everything should compile to basically the same code, but perhaps there was something I missed.

@oremanj
Copy link
Contributor Author

oremanj commented Nov 3, 2024

I don’t expect any overhead in terms of binary size or runtime cost for extensions that don’t use the new feature. Fixing the bug I mentioned in the last paragraph (keep_alive leaks a reference to the return value if it throws an exception) will imply a minor binary size cost for additional exception-handling landing pads, but I didn’t do that in this PR.

Copy link
Owner

@wjakob wjakob left a comment

Choose a reason for hiding this comment

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

Hi @oremanj,

I did a more throrow review, thisall looks great, just one comment regarding a testcase.

One general thing which could be improved is to point out other applications. The precall/postcall example linked in the documentation (sequence keep_alive) seems potentially a bit specific/contrived.

In contrast, the following sounds pretty nifty, but I don't really understand it from this high level description:

Motivating use case: a number of the types of interest in my application support a lightweight callback mechanism. The callback type we use is trivially copyable, so it can't manage ownership of a Python object directly. A custom call policy allows us to automatically take out a reference to the underlying Python callable when a new callback is subscribed, and drop the reference when it's unsubscribed. Allowing precall() to modify the argument list permits normalization (letting equal-but-distinct Python callables map to identical C++ callback objects, so that the C++ unsubscribe logic can find the callback that was originally subscribed).

Do you think it would be possible to explain a baby version of such a feature? It would make it more clear that powerful extensions are feasible with this escape hatch.

tests/test_functions.cpp Show resolved Hide resolved
@oremanj
Copy link
Contributor Author

oremanj commented Nov 5, 2024

Do you think it would be possible to explain a baby version of such a feature? It would make it more clear that powerful extensions are feasible with this escape hatch.

Done. It's a bit too long to include in the documentation, so I added it as a unit test.

@oremanj
Copy link
Contributor Author

oremanj commented Nov 5, 2024

I'm going to need some time to figure out why the 3.12+ Windows/macOS tests are failing. I can't reproduce the failure on my own macOS dev machine, nor on Linux with various sanitize/malloc-check options, nor can I figure out what's going on by staring at it. If you have any ideas, I'm all ears.

@wjakob
Copy link
Owner

wjakob commented Nov 5, 2024

On 3.12+, the CI uses stable ABI builds AFAIK. Can you repro the issue if you set NB_TEST_STABLE_ABI=ON?

@wjakob
Copy link
Owner

wjakob commented Nov 5, 2024

The new example looks fantastic, thank you very much for that!

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.

2 participants