MUSIC with MPI_DYNAMIC and PYTHON_DYNAMIC + coverage by nrnhines · Pull Request #2092 · neuronsimulator/nrn

nrnhines · 2022-11-17T11:44:21Z

@alexsavulescu I see you changed the base branch to master and back again to enable-music. A master base seems like a good idea as I thought enable-music was merged. But I do see extensive differences between master and enable-music. The situation seems to be, or rather I am, confused. master has a number of mentions of MUSIC, most recently Dec 2, 2022

nrnhines · 2023-02-07T20:15:17Z

I think hines/music-dyamic is fairly complete. But there are a few commits in hines/music-wheel that make sense to put into this PR. In particular, the changes to src/neuronmusic/nrnmusic_dynam.cpp and test/music_tests/runtests.py in regard to the change from MUSIC_LIB_NRN_PATH to NRN_LIBMUSIC_PATH

Then I thing this PR is ready to merge. hines/music-wheel needs a great deal of work. Mostly in regard to whether CI will robustly succeed in creating a wheel with MUSIC or whether we are setting ourselves up for regular failure.

alexsavulescu · 2023-02-08T08:49:40Z

I see you changed the base branch to master and back again to enable-music. A master base seems like a good idea as I thought enable-music was merged.

Yes, there were some merge conflicts so I switched back. It looks like enable-music is indeed merged.

Then I thing this PR is ready to merge.

Well I was having issues building master with MUSIC so I opened a PR to fix it and add coverage (#2092). But that setup is using NRN_ENABLE_PYTHON_DYNAMIC=ON which exhibits linking issues:

[ 34%] Linking CXX executable ../../bin/nrniv
/usr/bin/ld: ../../lib/libnrniv.so: undefined reference to `nrnpy_po2ho(_object*)'
/usr/bin/ld: ../../lib/libnrniv.so: undefined reference to `nrnpy_ho2po(Object*)'
collect2: error: ld returned 1 exit status
make[2]: *** [src/nrniv/CMakeFiles/nrniv.dir/build.make:135: bin/nrniv] Error 1
make[1]: *** [CMakeFiles/Makefile2:1702: src/nrniv/CMakeFiles/nrniv.dir/all] Error 2
make: *** [Makefile:146: all] Error 2

and that is related to

# NRN_ENABLE_MODULE_INSTALL will create a separate nrnpython lib
if(NRN_ENABLE_PYTHON AND NOT NRN_ENABLE_PYTHON_DYNAMIC)
  # Include nrnpython in nrniv - useful for single lib neuron and wheels
  list(APPEND NRN_NRNIV_LIB_SRC_FILES ${NRN_NRNPYTHON_SRC_FILES})
endif()

hines/music-wheel needs a great deal of work. Mostly in regard to whether CI will robustly succeed in creating a wheel with MUSIC or whether we are setting ourselves up for regular failure

That will have to wait for a stable MUSIC release. As you can see in https://github.com/neuronsimulator/nrn/pull/2209/files I was thinking of testing agains MUSIC based on the MPI branch. But that is not robust enough for CI purposes (better than nothing since I'm also working on dropping distutils #2193 where I've also touched MUSIC setup).

I will rebase this PR on top of master and get incorporate #2209 into it.

@nrnhines

Improve the FindPackage module for MUSIC Move around some of the tests and use find_path / find_library Fix cmake formatting Various minor fixes addressing @nrnhines' comments CMake module file name should correspond to find_package name. factor nrnmusic.cpp into neuronmusic folder nrnmusicapi.h valid for build time linkage or runtime dynamic loading Enabling music with dyamic mpi loads music dynamically. WIP. Dynamic MPI? Build a libnrnmusic for each mpi. tentative dynamic loading of nrnmusic neuronmusic.so for DYNAMIC needs linking against libnrnmusic...dylib Need to nrnmpi_load if -mpi or -music Temporary hack for music launching nrniv with music args. libnrnmusic.so does not depend on MPI version. Require MUSIC_LIB_NRN_PATH for dynamic loading of libmusic.so. Only dynamically load nrnmusic if a future nrnmusic_init will set nrnmusic=1 music_test sets PATH and MUSIC_LIB_NRN_PATH if needed. nrnmusic only exists if NRN_MUSIC defined revert nrnmpi_dynam.cpp to master.

alexsavulescu · 2023-02-08T09:08:51Z

I will rebase this PR on top of master

I've resolved the merge conflicts and squashed all commits. Notice there is now a back-up branch: hines/music-dynamic-old

add coverage setup

azure-pipelines · 2023-02-08T15:13:33Z

✔️ f4e2885 -> Azure artifacts URL

bbpbuildbot · 2023-02-08T16:00:47Z

bbpbuildbot · 2023-02-09T04:00:39Z

nrnhines · 2023-02-09T11:39:46Z

The code coverage failure is a puzzle

107: subprocess.CalledProcessError: Command 'mpirun -np 2 music test2.music' returned non-zero exit status 127.

As I was not able to reproduce on my ubuntu 22.04.1 desktop with

cmake .. -DNRN_ENABLE_MPI=ON -DNRN_ENABLE_INTERVIEWS=ON -DNRN_ENABLE_PYTHON=ON -DNRN_ENABLE_PYTHON_DYNAMIC=ON -DNRN_PYTHON_DYNAMIC=/home/hines/.pyenv/shims/python3.10 -DNRN_ENABLE_CORENEURON=ON -DNRN_ENABLE_PROFILING=ON -DNRN_ENABLE_BACKTRACE=ON -DNRN_ENABLE_MUSIC=ON -DCMAKE_C_COMPILER=gcc -DCMAKE_CXX_COMPILER=g++ -DNRN_ENABLE_TESTS=ON -DCMAKE_C_FLAGS="--coverage -O0 -fno-inline -g" -DCMAKE_CXX_FLAGS="--coverage -O0 -fno-inline -g" -DCORENRN_ENABLE_UNIT_TESTS=ON -DCMAKE_PREFIX_PATH=/home/hines/neuron/nrnmusic/external/MUSIC/instdir

$ ctest -R music_tests
Test project /home/hines/neuron/temp/build
    Start 101: nrnmusic::music_tests
1/1 Test #101: nrnmusic::music_tests ............   Passed    5.08 sec

As an aside, it is not clear to me why test numbering is different.

bbpbuildbot · 2023-02-10T12:00:42Z

codecov · 2023-02-10T12:50:57Z

Codecov Report

Merging #2092 (d3831a9) into master (9ee7c40) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2092      +/-   ##
==========================================
+ Coverage   56.10%   56.15%   +0.04%     
==========================================
  Files         618      620       +2     
  Lines      124114   124286     +172     
==========================================
+ Hits        69629    69787     +158     
- Misses      54485    54499      +14

Impacted Files	Coverage Δ
src/ivoc/nrnmain.cpp	`100.00% <ø> (ø)`
src/nrncvode/netcvode.cpp	`88.59% <ø> (+<0.01%)`	⬆️
src/nrniv/nrnpy.cpp	`63.80% <ø> (ø)`
src/nrnmpi/nrnmpi.cpp	`73.57% <ø> (+0.77%)`	⬆️
src/neuronmusic/nrnmusic.cpp	`87.58% <100.00%> (ø)`
src/nrniv/netpar.cpp	`97.87% <100.00%> (+0.02%)`	⬆️
src/nrnpython/nrnpy_p2h.cpp	`74.66% <100.00%> (+0.06%)`	⬆️
src/neuronmusic/nrnmusic.h	`100.00% <0.00%> (ø)`
... and 4 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

alexsavulescu · 2023-02-10T12:57:47Z

As I was not able to reproduce on my ubuntu 22.04.1 desktop with

neither was I, but have realized MUSIC was installed in /usr/local and we weren't providing MUSIC_ROOT. Maybe we'd need to tweak FindMUSIC a little bit.

As an aside, it is not clear to me why test numbering is different.

Numbering is relative to the number of test selected for run (-R music gives us just one)

bbpbuildbot · 2023-02-10T13:17:11Z

bbpbuildbot · 2023-02-13T10:10:03Z

bbpbuildbot · 2023-02-13T13:32:39Z

azure-pipelines · 2023-02-13T13:38:47Z

✔️ 8626c2b -> Azure artifacts URL

alexsavulescu

LGTM

azure-pipelines · 2023-02-13T20:26:55Z

✔️ d3831a9 -> Azure artifacts URL

bbpbuildbot · 2023-02-13T20:30:59Z

nrnhines marked this pull request as draft November 17, 2022 11:44

nrnhines changed the base branch from master to hines/enable-music November 17, 2022 11:46

nrnhines mentioned this pull request Nov 17, 2022

NRN_ENABLE_MUSIC on wheels #2091

Open

nrnhines mentioned this pull request Nov 17, 2022

Build neuronmusic #1894

Closed

This was referenced Nov 24, 2022

Re-enable MUSIC support in NEURON #1896

Merged

Wheel that supports MUSIC if installed. #2096

Draft

pramodk reviewed Nov 25, 2022

View reviewed changes

Comment thread src/neuronmusic/nrnmusic_dynam.cpp Outdated

alexsavulescu changed the base branch from hines/enable-music to master February 7, 2023 14:32

alexsavulescu changed the base branch from master to hines/enable-music February 7, 2023 14:38

alexsavulescu added a commit that referenced this pull request Feb 7, 2023

CMake MUSIC setup from #2092

1bb277e

alexsavulescu added a commit that referenced this pull request Feb 8, 2023

CMake MUSIC setup from #2092

d5df041

alexsavulescu force-pushed the hines/music-dynamic branch from e303b33 to 8539b70 Compare February 8, 2023 09:06

alexsavulescu changed the base branch from hines/enable-music to master February 8, 2023 09:07

fix MUSIC setup.py for multiple MPI include dirs

502aea0

add coverage setup

This was referenced Feb 8, 2023

fix MUSIC setup.py and add coverage #2209

Closed

remove distutils usage #2193

Merged

nrnhines added 2 commits February 8, 2023 20:58

nrnmusic.cpp link errors if python dynamically loaded

ce910db

format

137317d

alexsavulescu force-pushed the hines/music-dynamic branch 2 times, most recently from ba147c3 to 1fa1d63 Compare February 10, 2023 10:27

alexsavulescu force-pushed the hines/music-dynamic branch from 1fa1d63 to cd41673 Compare February 10, 2023 12:22

update music tests

aa0c02b

alexsavulescu force-pushed the hines/music-dynamic branch from cd41673 to aa0c02b Compare February 10, 2023 12:22

alexsavulescu requested a review from pramodk February 10, 2023 12:59

alexsavulescu requested a review from ohm314 February 10, 2023 13:24

alexsavulescu reviewed Feb 10, 2023

View reviewed changes

Comment thread .github/workflows/coverage.yml Outdated

MUSIC 1.2.0

dccc2c3

alexsavulescu force-pushed the hines/music-dynamic branch from f92a52e to dccc2c3 Compare February 13, 2023 09:19

Merge branch 'master' into hines/music-dynamic

8626c2b

alexsavulescu approved these changes Feb 13, 2023

View reviewed changes

Merge branch 'master' into hines/music-dynamic

d3831a9

alexsavulescu merged commit 7815d1e into master Feb 13, 2023

alexsavulescu deleted the hines/music-dynamic branch February 13, 2023 20:31

Uh oh!

Conversation

nrnhines commented Nov 17, 2022 • edited by alexsavulescu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

related

Uh oh!

bbpbuildbot commented Nov 17, 2022

Uh oh!

bbpbuildbot commented Nov 17, 2022

Uh oh!

bbpbuildbot commented Nov 17, 2022

Uh oh!

bbpbuildbot commented Nov 18, 2022

Uh oh!

bbpbuildbot commented Nov 19, 2022

Uh oh!

bbpbuildbot commented Nov 20, 2022

Uh oh!

bbpbuildbot commented Nov 20, 2022

Uh oh!

azure-pipelines Bot commented Nov 20, 2022

Uh oh!

pramodk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

azure-pipelines Bot commented Feb 7, 2023

Uh oh!

nrnhines commented Feb 7, 2023

Uh oh!

nrnhines commented Feb 7, 2023

Uh oh!

alexsavulescu commented Feb 8, 2023

Uh oh!

alexsavulescu commented Feb 8, 2023

Uh oh!

azure-pipelines Bot commented Feb 8, 2023

Uh oh!

bbpbuildbot commented Feb 8, 2023

Uh oh!

bbpbuildbot commented Feb 9, 2023

Uh oh!

nrnhines commented Feb 9, 2023

Uh oh!

bbpbuildbot commented Feb 10, 2023

Uh oh!

codecov Bot commented Feb 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alexsavulescu commented Feb 10, 2023

Uh oh!

bbpbuildbot commented Feb 10, 2023

Uh oh!

Uh oh!

bbpbuildbot commented Feb 13, 2023

Uh oh!

bbpbuildbot commented Feb 13, 2023

Uh oh!

azure-pipelines Bot commented Feb 13, 2023

Uh oh!

alexsavulescu left a comment

Choose a reason for hiding this comment

Uh oh!

azure-pipelines Bot commented Feb 13, 2023

Uh oh!

bbpbuildbot commented Feb 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nrnhines commented Nov 17, 2022 •

edited by alexsavulescu

Loading

codecov Bot commented Feb 10, 2023 •

edited

Loading