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

Exception when structural plasticity and multiple threads are used #629

Merged
merged 11 commits into from Jan 25, 2017

Conversation

@sdiazpier
Copy link
Contributor

sdiazpier commented Jan 12, 2017

This PR addresses issue #290 by throwing a kernel exception when structural plasticity is enabled and multiple threads have been set for simulation.

@sdiazpier
Copy link
Contributor Author

sdiazpier commented Jan 12, 2017

I suggest @heplesser, @jakobj and @sanjayankur31 as potential reviewers.

Copy link
Contributor

heplesser left a comment

@sdiazpier I approve this already, but you need to fix the conflict in test_all.py, which is due to the fact that I just merged #604.

@jakobj
jakobj approved these changes Jan 12, 2017
Copy link
Contributor

jakobj left a comment

Nice, apart from the my small comment, so 👍

void
nest::SPManager::enable_structural_plasticity()
{
if ( kernel().vp_manager.get_num_threads() > 1 )

This comment has been minimized.

@jakobj

jakobj Jan 12, 2017 Contributor

please add curly braces around the statement

@@ -180,6 +180,10 @@ nest::VPManager::get_status( DictionaryDatum& d )
void
nest::VPManager::set_num_threads( nest::thread n_threads )
{
if ( kernel().sp_manager.is_structural_plasticity_enabled()
&& ( n_threads > 1 ) )
throw KernelException(

This comment has been minimized.

@jakobj

jakobj Jan 12, 2017 Contributor

(see above)

with self.assertRaises(nest.NESTError):
nest.SetKernelStatus(
{
'resolution': 0.1,

This comment has been minimized.

@jakobj

jakobj Jan 12, 2017 Contributor

why do you need to change resolution here?

@heplesser
Copy link
Contributor

heplesser commented Jan 14, 2017

@sdiazpier There are some small code-formatting problems and the test for issue #578 now seems to work no longer:

 Weight Recorder Single Threaded ... ok
  ERROR: test_targets (nest.tests.test_sp.test_issue_578_sp.TestIssue578)
      raise _kernel.NESTError(encode(errorstring))
  NESTError: Structural plasticity can not be used with multiple threads in EnableStructuralPlasticity: 
  FAILED (SKIP=11, errors=1)

That test requires two threads (it is run as a single MPI process):

        nest.SetKernelStatus(
            {
                'resolution': 0.1,
                'total_num_virtual_procs': 2
            }
        )

So somehow that test and this PR seem orthogonal to each other?

…only when MPI is available.
Copy link
Contributor

heplesser left a comment

@sdiazpier Fine in principle, but some details need fixing.

call(["mpiexec", "-n", "2", "python", "test_sp/test_issue_578_sp.py"])
except:
print sys.exc_info()[0]
print "Test call with MPI ended in error"

This comment has been minimized.

@heplesser

heplesser Jan 16, 2017 Contributor

print requires () to be Python3 compatible.

And shouldn't this exception propagate so that the test suite knows that the test failed? Currently, failing MPI will be registered as a working test.

from mpi4py import MPI
except ImportError:
# Test without MPI
print "Tests without MPI"

This comment has been minimized.

@heplesser

heplesser Jan 16, 2017 Contributor

print needs () in Python3.

else:
# Test with MPI
mpi_test = 1
print "Testing with MPI"

This comment has been minimized.

@heplesser

heplesser Jan 16, 2017 Contributor

print needs () in Python3.

except ImportError:
# Test without MPI
print "Tests without MPI"
mpi_test = 0

This comment has been minimized.

@heplesser

heplesser Jan 16, 2017 Contributor

If MPI is not available, post a skipping message.

# MPI tests
if mpi_test:
try:
call(["mpiexec", "-n", "2", "python", "test_sp/test_issue_578_sp.py"])

This comment has been minimized.

@heplesser

heplesser Jan 16, 2017 Contributor

It would be best to pick up the MPI-invocation rule from .nestrc.

@sdiazpier
Copy link
Contributor Author

sdiazpier commented Jan 16, 2017

Hi dear @heplesser, I have addressed most of your comments. I am not very acquainted with the format of the .nestrc file so I read the command in the most generic way I could with the knowledge that I have from it. Maybe I am not considering certain aspects of the format so If you have a better way or suggestion to make it easier/more robust please let me know.

@sdiazpier
Copy link
Contributor Author

sdiazpier commented Jan 18, 2017

Dear @jougs, could you please take a look at this PR and let me know your opinion on handling the nestrc file to obtain the mpi execution command?

Copy link
Contributor

jougs left a comment

@sdiazpier: thanks for the PR. I've added some suggestions to make handling of the MPI command line more robust. I'll have a look again once you've addressed those.

print ("MPI test command: " + line)
nestrcf.close()
return line

This comment has been minimized.

@jougs

jougs Jan 24, 2017 Contributor

There's no need to parse and modify the line from .nestrc manually here as this is anyway a SLI function, which gives you the invocation line for a given number of processes, an executable and a script file. I suggest to replace the function signature by

def getMPITestCommand(np, script):

and the function body by something like this:

return nest.sli_func("mpirun", np, "python", script)

This is much safer, as it works for all possible definitions of mpirun in the .nestrc file.

command = getMPITestCommand()
command = command.replace(
" scriptfile", "test_sp/mpitest_issue_578_sp.py")
print ("Executing test with command: " + command)

This comment has been minimized.

@jougs

jougs Jan 24, 2017 Contributor

Given the changes to the getMPITestCommand function detailed below, you can replace lines 53-55 by just

command = getMPITestCommand(2, "test_sp/mpitest_issue_578_sp.py")
@sdiazpier
Copy link
Contributor Author

sdiazpier commented Jan 25, 2017

Hi dear @jougs I have made the changes you suggested. Could you please take a look at it and let me know if anything is missing?

… GSL is not enabled for this test and for the weight recorder test
@sdiazpier
Copy link
Contributor Author

sdiazpier commented Jan 25, 2017

Dear @jougs I implemented the changes we talked about and added the check for GSL in the test. Please let me know if anything else is needed.

@jougs
jougs approved these changes Jan 25, 2017
Copy link
Contributor

jougs left a comment

Many thanks for your changes. This looks very nice now.

@jougs jougs merged commit 9bf42d7 into nest:master Jan 25, 2017
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.