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

Bring Nanobind. Fix object leak in from_numpy #2545

Merged
merged 100 commits into from
Jun 26, 2024
Merged

Bring Nanobind. Fix object leak in from_numpy #2545

merged 100 commits into from
Jun 26, 2024

Conversation

ferdonline
Copy link
Member

@ferdonline ferdonline commented Sep 26, 2023

Context

Neuron Python bindings have been implemented by using the Python C API directly. Among the major issues we identified:

  • ref-counting, which is simply too easy to get wrong, especially for routines that can exit early.
  • Identifying which references should be borrowed or stollen, which requires a careful read of the API and reference management.
  • The API verbosity. Operations like looping over a Python list are verbose, error prone and not particularly intuitive, incurring a high maintenance toll.

Such problems lead to "undead" objects i.e., objects which are no longer in use but due to its wrong ref count they are not garbage collected, keep using valuable memory.

This PR

In this PR we bring nanobind to improve our lives and apply it to solve a case where there was such leaking of undead objects.

TODO

  • Find a way to avoid git update --recursive as this is only needed for nanobind
  • Try to expose symbols on windows without using dllexport or do it in a separate header file.

@bbpbuildbot

This comment has been minimized.

@azure-pipelines
Copy link

✔️ 07d58f6 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@azure-pipelines
Copy link

✔️ bf98304 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@ferdonline ferdonline changed the title Bring Nanobind. Fix memory leak in from_numpy Bring Nanobind. Fix object leak in from_numpy Sep 28, 2023
@bbpbuildbot

This comment has been minimized.

@azure-pipelines
Copy link

✔️ 05ed787 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@alkino alkino marked this pull request as ready for review June 25, 2024 14:06
Copy link

✔️ f84830b -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@alkino
Copy link
Member

alkino commented Jun 25, 2024

And so we use NRN_EXPORT on all functions, because gcc has no flag, to change the visibility for all functions.
cmake has one but seems broken with gcc compilation for windows: https://discourse.cmake.org/t/too-restrictive-for-coff-obj-files-in-create-def/11077

@alkino
Copy link
Member

alkino commented Jun 25, 2024

For the git and recursivity I have no idea how to do it better.

Copy link
Member

@pramodk pramodk left a comment

Choose a reason for hiding this comment

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

Just one minor comment for clarifications otherwise LGTM!

@ramcdougal has already given a green signal last month.

@nrnhines: you approved this last year. In case you want to take a look again, let us know. Otherwise I think this is ready to go!

thanks a lot @alkino ! 🎉 (& @ferdonline 🍾)

.github/workflows/windows.yml Outdated Show resolved Hide resolved
cmake/ExternalProjectHelper.cmake Outdated Show resolved Hide resolved
@bbpbuildbot

This comment has been minimized.

@alkino alkino closed this Jun 26, 2024
@alkino alkino reopened this Jun 26, 2024
Copy link

✔️ 28bf8ef -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

Copy link

✔️ c218b71 -> Azure artifacts URL

@alkino alkino enabled auto-merge (squash) June 26, 2024 11:38
@alkino alkino disabled auto-merge June 26, 2024 11:40
Copy link

sonarcloud bot commented Jun 26, 2024

Copy link

✔️ ae200cc -> Azure artifacts URL

@alkino alkino merged commit 8ffa1fa into master Jun 26, 2024
35 of 36 checks passed
@alkino alkino deleted the feature/nanobind branch June 26, 2024 12:23
@bbpbuildbot

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants