Skip to content

Bring Nanobind. Fix object leak in from_numpy#2545

Merged
alkino merged 100 commits into
masterfrom
feature/nanobind
Jun 26, 2024
Merged

Bring Nanobind. Fix object leak in from_numpy#2545
alkino merged 100 commits into
masterfrom
feature/nanobind

Conversation

@ferdonline

@ferdonline ferdonline commented Sep 26, 2023

Copy link
Copy Markdown
Member

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
Copy Markdown

✔️ 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
Copy Markdown

✔️ 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
Copy Markdown

✔️ 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
@azure-pipelines

Copy link
Copy Markdown

✔️ f84830b -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@alkino

alkino commented Jun 25, 2024

Copy link
Copy Markdown
Member

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

alkino commented Jun 25, 2024

Copy link
Copy Markdown
Member

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

@pramodk pramodk left a comment

Copy link
Copy Markdown
Member

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 🍾)

Comment thread .github/workflows/windows.yml
Comment thread cmake/ExternalProjectHelper.cmake Outdated
@bbpbuildbot

This comment has been minimized.

@alkino alkino force-pushed the feature/nanobind branch from 1131eea to a342e8c Compare June 26, 2024 07:40
@alkino alkino closed this Jun 26, 2024
@alkino alkino reopened this Jun 26, 2024
@azure-pipelines

Copy link
Copy Markdown

✔️ 28bf8ef -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@azure-pipelines

Copy link
Copy Markdown

✔️ 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
@sonarqubecloud

Copy link
Copy Markdown

@azure-pipelines

Copy link
Copy Markdown

✔️ ae200cc -> Azure artifacts URL

@alkino alkino merged commit 8ffa1fa into master Jun 26, 2024
@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.

10 participants