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 runtime setting to suppress leak warnings #109

Merged
merged 7 commits into from Dec 29, 2022

Conversation

MatthiasKohl
Copy link
Contributor

The reasoning behind this is as follows:

In our project, we again ran into a reference-counting issue, which isn't caused by our own extension library, but most likely caused by the PyTorch extension library, see here: tingyu66/nanobind_example@8629419

We don't have the bandwidth/time to investigate all these issues in other libraries, and in general, we have no control over what other extension libraries our end users run the library with.
In order to avoid bothering users with leak reports, this PR adds a mechanism to suppress leak reports which would be used for builds distributed to end users.
Of course by default, we keep the leak reports on, in order to catch these during development, as well as in our own CI.

@wjakob
Copy link
Owner

wjakob commented Dec 8, 2022

I am sympathetic to the request but see issues with the specific solution proposed here.

Different nanobind extension modules will share the internals data structure (important for larger projects whose bindings are spread out over multiple extension modules). Only the first one to be loaded is going to install the atexit handler. This means that if another extension is imported before your project, you would still get the warnings.

My suggestion would be that you add an API like:

nb::set_leak_warnings(false);

which updates field bool leak_warnings = true; stored inside the internals data structure.

@MatthiasKohl
Copy link
Contributor Author

Thanks for the feedback !
I changed things as you suggested, please let me know if the placement in files is correct/as expected.
Also, is there some kind of automatic code formatter? I tried to keep things in line with the current style.

@wjakob
Copy link
Owner

wjakob commented Dec 21, 2022

Hi @MatthiasKohl,

sorry for taking a little while to respond. I have two more requests: all "library" symbols are declared in nb_lib.h in the detail namespace. That makes it quite easy to understand what the library exports. Could you move the declaration there?

You will then need a wrapper in the nanobind namespace that calls nanobind::detail:: set_leak_warnings. This is redundant, but it decouples the API and ABI aspects of nanobind.

Best,
Wenzel

@wjakob
Copy link
Owner

wjakob commented Dec 21, 2022

(Looks like there are some bigger issues with the code as well)

@MatthiasKohl
Copy link
Contributor Author

@wjakob that makes sense, I added the symbol to nb_lib.h.
I was quite busy before as well, and I've only now checked with my original project that everything compiles correctly and behaves as expected.

Do you need me to change anything else ?

@wjakob wjakob merged commit 04f1f9e into wjakob:master Dec 29, 2022
@wjakob
Copy link
Owner

wjakob commented Dec 29, 2022

Thanks -- merged, and amended with minor tweaks.

@wjakob wjakob changed the title Add compile-time variable to suppress leak warnings Add runtime variable to suppress leak warnings Dec 29, 2022
@wjakob wjakob changed the title Add runtime variable to suppress leak warnings Add runtime setting to suppress leak warnings Dec 29, 2022
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.

None yet

2 participants