Skip to content

Wheel that supports MUSIC if installed.#2096

Draft
nrnhines wants to merge 71 commits into
masterfrom
hines/music-wheel
Draft

Wheel that supports MUSIC if installed.#2096
nrnhines wants to merge 71 commits into
masterfrom
hines/music-wheel

Conversation

@nrnhines

@nrnhines nrnhines commented Nov 21, 2022

Copy link
Copy Markdown
Member

A beginning...
Extends #2092 (dynamic loading of MUSiC) which extends #1896 (reenable MUSIC support on a build machine).

How to test wheels locally for this PR

nrnhines and others added 30 commits August 26, 2022 14:46
Move around some of the tests and use find_path / find_library
also (temporarily) allow C++ MPI headers to see if things build
MUSIC won't be needing those much longer.
@alexsavulescu alexsavulescu force-pushed the hines/music-wheel branch 4 times, most recently from c43eb91 to 6bd3d7b Compare March 15, 2023 21:17
@alexsavulescu

Copy link
Copy Markdown
Member

Status - blocked

Ok so this is hitting a blocking snag.

Issue

Long story short, shipping C++ features in wheels (i.e. std::string in this case) can yield ABI issues (see relevant discussion in #1963)

Details

# On my ubuntu
(nrn_test_venv_310) savulesc@bbd-cjngk03:~/Workspace/nrn$ python test/music_tests/runtests.py
MUSIC_LIBDIR: /nrnwheel/MUSIC/lib
MUSIC located in: /nrnwheel/MUSIC/lib
MUSIC_BINPATH: /nrnwheel/MUSIC/bin:
NRN_ENABLE_MPI_DYNAMIC=ON
NRN_LIBMUSIC_PATH=/nrnwheel/MUSIC/lib/libmusic.so
PATH: /nrnwheel/MUSIC/bin:/home/savulesc/Workspace/nrn/nrn_test_venv_310/bin:/home/savulesc/.local/bin:/home/savulesc/bin:/home/savulesc/.local/bin:/home/savulesc/bin:/usr/local/Modules/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin:/snap/bin:/opt/puppetlabs/bin:/home/savulesc/.local/share/JetBrains/Toolbox/scripts:/home/savulesc/.local/share/JetBrains/Toolbox/scripts
/home/savulesc/Workspace/nrn/nrn_test_venv_310/lib/python3.10/site-packages/neuron/.data/lib/libnrnmusic.so: undefined symbol: _ZN5MUSIC4PortC2EPNS_5SetupESs
...
subprocess.CalledProcessError: Command 'mpiexec  -n  2 /nrnwheel/MUSIC/bin/music test2.music' returned non-zero exit status 1.
(nrn_test_venv_310) savulesc@bbd-cjngk03:~/Workspace/nrn$ c++filt _ZN5MUSIC4PortC2EPNS_5SetupESs
MUSIC::Port::Port(MUSIC::Setup*, std::basic_string<char, std::char_traits<char>, std::allocator<char> >)
(nrn_test_venv_310) savulesc@bbd-cjngk03:~/Workspace/nrn$ nm -A /nrnwheel/MUSIC/lib/libmusic.so | xargs c++filt | grep MUSIC::Port::Port
MUSIC::Port::Port(MUSIC::Setup*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >)
MUSIC::Port::Port(MUSIC::Setup*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >)
MUSIC::Port::Port(MUSIC::Setup*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) [clone .cold]
# On the docker image
[root@f23f375dfb64 nrn]# nm -A /opt/nrnwheel/MUSIC/lib/libmusic.so | xargs c++filt | grep MUSIC::Port::Port
MUSIC::Port::Port(MUSIC::Setup*, std::basic_string<char, std::char_traits<char>, std::allocator<char> >)
MUSIC::Port::Port(MUSIC::Setup*, std::basic_string<char, std::char_traits<char>, std::allocator<char> >)
MUSIC::Port::Port(MUSIC::Setup*, std::basic_string<char, std::char_traits<char>, std::allocator<char> >) [clone .cold]

What next?

On thing that could work is if MUSIC provided a C-API, i.e. EventOutputPort(MUSIC::Setup* s, char* id), maybe that would alleviate the wheel portability issues. Some experimentation is in order to see if that works out (I am not familiar with MUSIC code & interplay).

cc @mdjurfeldt @nrnhines

@mdjurfeldt

Copy link
Copy Markdown
Contributor

@alexsavulescu @nrnhines Hi, sorry but I saw this first now that Alex brought it to my attention.

I guess it is the std::string argument which is the problem? Would it help if I would overload the Port constructor such that it can take a C string? In that case you could cast the argument to a C string.

Also, MUSIC does provide a C api (in music-c.h). NEURON could use that instead of the C++ API, but could we maybe try out the solution I suggest above first? Do you need help from me to try it out? I don't think that I easily could reproduce this error...

@alexsavulescu

Copy link
Copy Markdown
Member

@mdjurfeldt unfortunately I haven't explored the MUSIC code yet, but it is encouraging that there is a C-API. Regarding the overload, I'm not sure, but if the MUSIC classes that we inherit from in NEURON have std::string as data members , then I believe that is not going to work out. I don't know when I'll be able to revisit the wheel integration, I can however point out how to use docker to build wheels locally and then test them on the host platform. In the meantime that will give you time to work out the licensing aspects.

@mdjurfeldt

Copy link
Copy Markdown
Contributor

Yes, please give me advice how to reproduce the error.

@alexsavulescu

alexsavulescu commented Apr 3, 2023

Copy link
Copy Markdown
Member

Yes, please give me advice how to reproduce the error.

PR description updated with how-to

@JCGoran

JCGoran commented May 16, 2025

Copy link
Copy Markdown
Collaborator

Could this potentially be revived now that #1963 has been resolved? Or is licensing an issue (I see MUSIC still seems to be under GPL v3)?

@nrnhines

Copy link
Copy Markdown
Member Author

Could this potentially be revived now that #1963 has been resolved? Or is licensing an issue (I see MUSIC still seems to be under GPL v3)?

I believe so. I don't think the license is an issue as the wheel would not contain any MUSIC software. On the user machine it just connects to MUSIC if the user wants to use it and MUSIC is installed. (like MPI and Python Dynamic) @mdjurfeldt would you like to see this completed?

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.

NRN_ENABLE_MUSIC on wheels

6 participants