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

Create NEST Extension Interface for NEST compatible with new registration scheme and unloading #3103

Merged
merged 32 commits into from
Mar 19, 2024

Conversation

heplesser
Copy link
Contributor

The title says it all. This is still in draft stage as I need to iron out some problems, especially segfaults on lt_dlclose() of the module. ResetKernel() works and one can install the module again afterwards, but I have not yet checked what happens if the module changes between reloads. At present, I need to provide the (DY)LD_LIBRARY_PATH to the install location of the module. The old DynamicLoader is replaced by ModuleManager and NESTExtensionInterface is the base class for extension modules.

In nest/nest-extension-module#26 you will find a version of mymodule (in the modulemgr branch) that works with this branch of NEST.

This fixes #3064.

@heplesser heplesser added T: Enhancement New functionality, model or documentation S: High Should be handled next I: External API Developers of extensions or other language bindings may need to adapt their code labels Feb 12, 2024
@heplesser heplesser marked this pull request as ready for review February 13, 2024 08:01
@heplesser
Copy link
Contributor Author

Everything works now, including loading a modified module after ResetKernel(). One still needs to make sure that the path to the extension module is explicitly set in (DY)LD_LIBRARY_PATH, even if it is the same as the installation location for the NEST libs. Got to see if one can improve on this, would need some search-path expansion in the module manager. But otherwise, this is ready for review.

Copy link
Contributor

@pnbabu pnbabu left a comment

Choose a reason for hiding this comment

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

Thank you for these changes! I tested this with modules generated from NESTML and the install works fine. I have a few comments. Please find them below.

nestkernel/module_manager.cpp Show resolved Hide resolved
nestkernel/module_manager.cpp Outdated Show resolved Hide resolved
nestkernel/module_manager.cpp Outdated Show resolved Hide resolved
nestkernel/nest_extension_interface.h Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
@heplesser
Copy link
Contributor Author

@pnbabu Thank you for your review! I have fixed most items and commented on the two remaining.

@clinssen clinssen added this to the NEST 3.7 milestone Feb 21, 2024
Copy link
Contributor

@terhorstd terhorstd left a comment

Choose a reason for hiding this comment

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

Some minor points and some questions still open.

nestkernel/nestmodule.cpp Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
cmake/ProcessOptions.cmake Show resolved Hide resolved
nestkernel/kernel_manager.h Show resolved Hide resolved
nestkernel/module_manager.h Outdated Show resolved Hide resolved
nestkernel/module_manager.h Show resolved Hide resolved
Copy link
Contributor

@pnbabu pnbabu left a comment

Choose a reason for hiding this comment

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

Looks good to me! Just some minor suggestions.

nestkernel/module_manager.h Outdated Show resolved Hide resolved
nestkernel/module_manager.h Outdated Show resolved Hide resolved
nestkernel/module_manager.h Outdated Show resolved Hide resolved
@heplesser
Copy link
Contributor Author

@terhorstd Thanks for your comments! Could you take a look at my replies?

@heplesser heplesser requested review from terhorstd and clinssen March 12, 2024 11:27
.#foo Outdated Show resolved Hide resolved
Copy link
Contributor

@terhorstd terhorstd left a comment

Choose a reason for hiding this comment

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

Looks good so far. Thanks for the complex work!

@abigailm abigailm merged commit 3ee7f98 into nest:master Mar 19, 2024
24 checks passed
@heplesser heplesser deleted the fix-3064 branch April 24, 2024 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: External API Developers of extensions or other language bindings may need to adapt their code S: High Should be handled next T: Enhancement New functionality, model or documentation
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Issues with new model registration in NEST
5 participants