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

Add automatic testing for MUSIC #471

Merged
merged 24 commits into from Sep 16, 2016
Merged

Conversation

@jougs
Copy link
Contributor

jougs commented Sep 1, 2016

This PR adds MUSIC support to the maximal build for Travis CI. It also brings a framework for testing MUSIC enabled SLI scripts to the testsuite. A README explains it's usage, while three basic tests serve as examples of how to write tests for the new framework.

I suggest @heplesser, @mdjurfeldt and @uahic as reviewers.

@heplesser
Copy link
Contributor

heplesser commented Sep 4, 2016

@jougs @mdjurfeldt As a first step, I have tried to build MUSIC under OSX 10.11.6 with gcc 6.2.1 and OpenMPI 2.0.1. I noticed two problems:

  • In autogen.sh, libtoolize must be glibtoolize on the Mac
  • When compiling, I get the following error (also with clang):
../../music/src/music/data_map.hh:46:13: error: 'MPI' does not name a type
     virtual MPI::Datatype type () = 0;

In NEST's mpi_manager.cpp I only find MPI_Datatype and similar, not MPI::Datatype. The only place where I spot MPI:: is in mpi_manager.h, and there it seems MUSIC-related.

Could you enlighten me?

@mdjurfeldt
Copy link
Contributor

mdjurfeldt commented Sep 4, 2016

How can one, in autogen.sh, detect that the environment is a mac?

Den 5 sep. 2016 00:57 skrev "Hans Ekkehard Plesser" <
notifications@github.com>:

@jougs https://github.com/jougs @mdjurfeldt
https://github.com/mdjurfeldt As a first step, I have tried to build
MUSIC under OSX 10.11.6 with gcc 6.2.1 and OpenMPI 2.0.1. I noticed two
problems:

  • In autogen.sh, libtoolize must be glibtoolize on the Mac
  • When compiling, I get the following error (also with clang):

../../music/src/music/data_map.hh:46:13: error: 'MPI' does not name a type
virtual MPI::Datatype type () = 0;

In NEST's mpi_manager.cpp I only find MPI_Datatype and similar, not
MPI::Datatype. The only place where I spot MPI:: is in mpi_manager.h, and
there it seems MUSIC-related.

Could you enlighten me?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#471 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADcfCcy9PM0pdZc8l_cpthBA4e000eivks5qm0y9gaJpZM4JyuGG
.

@heplesser
Copy link
Contributor

heplesser commented Sep 4, 2016

This is from nest-2.10.0's bootstrap.sh:

if [ `uname -s` = Darwin ] ; then
# libtoolize is glibtoolize on OSX
  LIBTOOLIZE=glibtoolize
else  
  LIBTOOLIZE=libtoolize
fi
@mdjurfeldt
Copy link
Contributor

mdjurfeldt commented Sep 5, 2016

i'm sorry---Tammo has already submitted a pull request to MUSIC (INCF/MUSIC#24) fixing this. The reason why it has stalled is that I need to think about a name collision for a script. I'll do this.

MPI:: is the name space for the C++ interface. I will install openmpi 2.0 myself and examine this issue. (The C++ interface has been deprecated since version 2.2. of the MPI standard.)

@jougs
Copy link
Contributor Author

jougs commented Sep 5, 2016

@heplesser, @mdjurfeldt: can you please move the discussion about problems in MUSIC to a corresponding issue on https://github.com/INCF/MUSIC and reference that here? It is rather confusing to have this discussion here. Thanks!

@gtrensch gtrensch mentioned this pull request Sep 8, 2016
@jougs jougs changed the title Add automatic testing for MUSIC Add automatic testing for MUSIC :+1: Sep 12, 2016
@jougs jougs changed the title Add automatic testing for MUSIC :+1: Add automatic testing for MUSIC Sep 12, 2016
% return value is return value of command
}
def

This comment has been minimized.

Copy link
@heplesser

heplesser Sep 14, 2016

Contributor

@jougs Why are you removing this? It may be perfectly sensible, I just would like to know.

This comment has been minimized.

Copy link
@jougs

jougs Sep 14, 2016

Author Contributor

My reasoning for the removal was like this: the check for /mpirun is not needed anymore, now that we always provide a not-commented version of /mpirun in .nestrc. The remaining code does not add anything above what /mpirun and /nest_indirect already provide.

@@ -517,6 +517,24 @@ echo "--------------------------"
if test "x$(sli -c 'statusdict/have_mpi :: =')" = xtrue ; then
junit_open 'core.phase_5'

# Check for old version of the /mpirun command, which had the NEST executable hardcoded
if test "x$(sli -c '/mpirun load cva_t Flatten length 3 eq =')" = xtrue ; then

This comment has been minimized.

Copy link
@heplesser

heplesser Sep 14, 2016

Contributor

This test needs to be done before the first test if NEST is compiled with MPI, because all serial tests are then run as mpirun -np 1 via nest_serial.

and with the total number of processes that is requested in the MUSIC
configuration file.

The test is expected to exit with an exit code of 0 if it

This comment has been minimized.

Copy link
@heplesser

heplesser Sep 14, 2016

Contributor

"Each test" or "A test"

@@ -32,7 +32,7 @@ meip << /port_name (spikes_in) /music_channel 0 >> SetStatus
meip n << /weight 750.0 >> Connect

This comment has been minimized.

Copy link
@heplesser

heplesser Sep 14, 2016

Contributor

This comment applies to the MPI-check a little further up. Could you replace the explicit quit_i code with an exit_test_gracefully (or what it is called exactly)?

@@ -32,7 +32,7 @@ sg << /spike_times [1.0 1.5 2.0 ]>> SetStatus
sg n << /weight 750.0 >> Connect

This comment has been minimized.

Copy link
@heplesser

heplesser Sep 14, 2016

Contributor

exit_gracefully also here.

@heplesser
Copy link
Contributor

heplesser commented Sep 14, 2016

@jougs This looks pretty good to me, but see my comments in the code.

Would it make sense to add at least one test in which the individual SLI processes have more than one MPI process each?

I am a little uncertain if I like the idea of files just being grouped into tests by equal/similar names. Would it make sense to have one directory per test to make the grouping explicit? I am mainly concerned with what will happen if we apply this framework to mpi-based testing in general with a large number of tests.

@jougs
Copy link
Contributor Author

jougs commented Sep 14, 2016

@heplesser: thanks for the review. I've addressed all of your comments in the recent commits. Moreover, I have made the error message for an old version of /mpirun more explicit with respect to custom versions of the function.

We should definitely add more tests for MUSIC, also including some using more than one process per application. However, this PR is merely about adding the framework and basic tests to show how things work. It was not meant to add extensive testing for MUSIC. Feel free to add an issue for any missing test case you think should be covered.

I also thought about putting the files in subdirectories, but refrained from doing so, as this would IMO limit discoverability of the existing tests. That said, I am open to re-discuss this once we extend the framework to other test phases.

%} Function def

% The command mpirun tells NEST how to assemble a string, which
% represents a valid command for calling a MPI-enabled executable in a

This comment has been minimized.

Copy link
@gtrensch

gtrensch Sep 15, 2016

Collaborator

an MPI-enabled

@gtrensch
Copy link
Collaborator

gtrensch commented Sep 15, 2016

👍 reviewed and tested. Check for old version of the /mpirun command works also fine now !

Copy link
Contributor

heplesser left a comment

@jougs All fine, and I approve!

@heplesser heplesser merged commit 38a9608 into nest:master Sep 16, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.