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

Wheel that supports MUSIC if installed. #2096

Draft
wants to merge 71 commits into
base: master
Choose a base branch
from
Draft

Conversation

nrnhines
Copy link
Member

@nrnhines nrnhines commented Nov 21, 2022

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.
@azure-pipelines
Copy link

✔️ c9dd9fb -> Azure artifacts URL

@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
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
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
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
Contributor

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

@alexsavulescu
Copy link
Member

alexsavulescu commented Apr 3, 2023

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

PR description updated with how-to

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
5 participants