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

NEST is running out of space for synapse models #1043

Closed
heplesser opened this issue Oct 9, 2018 · 10 comments · Fixed by #1088
Closed

NEST is running out of space for synapse models #1043

heplesser opened this issue Oct 9, 2018 · 10 comments · Fixed by #1088
Assignees
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Critical Needs to be addressed immediately T: Maintenance Work to keep up the quality of the code and documentation. ZC: Kernel DO NOT USE THIS LABEL ZP: Pending DO NOT USE THIS LABEL

Comments

@heplesser
Copy link
Contributor

NEST currently supports only 63 different synapse models. The current master has already 55 synapse models, so that only very few additional synapse models can be created with CopyModel. Adding more synapse models makes the situation even worse, see #865.

The number of synapse models available by default is as large as 55, because many synapse models exist in multiple versions (plain, _lbl, _hpc).

I believe we need to discuss increasing the space for synapse models to at least 255 and reconsider if all model variants should be created even if they are never used: _lbl is used by PyNN, which then does not use the plain version, and models using _hpc variants do not use the plain variant simultaneously.

@heplesser heplesser added ZC: Kernel DO NOT USE THIS LABEL ZP: Pending DO NOT USE THIS LABEL S: High Should be handled next T: Maintenance Work to keep up the quality of the code and documentation. I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Oct 9, 2018
@heplesser
Copy link
Contributor Author

@suku248 @jakobj Could you look at and comment on this issue?

aserenko added a commit to aserenko/nest-simulator that referenced this issue Oct 10, 2018
A work-around for Issue nest#1043, as suggested by
@heplesser's comment to PR nest#865.
@jakobj
Copy link
Contributor

jakobj commented Oct 15, 2018

yes, the fact that one can only create few additional synapse models is certainly an issue. here are my thoughts:

  1. we should explicitly discourage people from use copy model for identical models with different parameters settings as this not only exhausts the maximal number of synapse types but also can incur runtime penalties
  2. we should reduce the default synapse model count by only registering the "_lbl" and "_hpc" synapses when explicitly requested during compile time
  3. we should add a cmake flag that allows users to increase the number of possible synapses at the expense of maximal mpi ranks/threads/number of neurons. the choice of concrete numbers will have to be discussed (if we increase the number of possible synapse models without introducing additional constraints, we will increase the memory usage of NEST)

@jougs
Copy link
Contributor

jougs commented Oct 22, 2018

@jakobj: I was assigned to this mainly for mental support, not for the coding ;-)

First, let me reply to your comment:

  • Discouraging the use of CopyModel probably won't help much if NEST already brings 55 types and the user anyway only has 8 types left for customization.
  • Not registering the special types probably leads to quite some user complaints as PyNN needs the _lbl types, some examples and existing code use the _hpc types and people generally don't expect a recompilation to be necessary for such things to work.

The current split of the Target union into the different fields allows 1,048,576 ranks (20 bit), 1,024 threads per rank (10 bit), 64 synapse ids (6 bit), 134,217,728 local synapses (27 bit) and a flag for knowing if the synapse has been processed. These numbers seem to be optimized for the super-large scale, while creating problems for normal users in interactive sessions, where 8 custom synapse types may quickly be used up during exploration (again, NEST already populates most of the space and NESTML will make the creation of new synapse types much easier soon...)

I thus propose to alter the defaults towards a more John-Doe-friendly split and make it configurable from cmake for experts and HPC users (something like --Dtarget-union-split=20-10-6-27-1, a small shell snippet and a bit of code generation/injection for nestkernel/target.h should do the trick).

A default of 19 (instead of 20) bits for the rank (=524,288) and 9 (instead of 10) bits for the thread id (=512) in favor of 8 bit for the synapse type would probably already solve the problem, while also keeping most HPC users happy.

There should also be a clear explanation of the split in the NEST documentation along the lines of the Target table in Fig. 4 of Jordan et al. 2018, probably also mentioning the actual number of elements (i.e. 2x for x in the table)

@jakobj
Copy link
Contributor

jakobj commented Nov 16, 2018

thanks for your support @jougs :)

I agree that the current split is suboptimal for the normal (non-HPC) use case and that my previously suggested solution has significant drawbacks. However, giving users full flexibility with respect to the desired split as in your example makes me a bit uneasy as this complicates reproduction of results among users ("which split did you use?", "hmm, I don't remember, let me check my log files, ..., ughh, can find them right now, but it was something like XXX -- maybe?") without providing significant usability advantages. How about hardcoding two splits instead, one for the normal use case and one for HPC? It's not that I'm not intrigued by the technical side of your suggestion. ;)

@Silmathoron
Copy link
Member

As mentioned in the full-day NEST dev visio, I would advise for a cmake option (one for HPC, one for laptop users), in agreement with @jakobj 's suggestion.

@heplesser
Copy link
Contributor Author

Discussed in VC 26 Nov. Combine user-friendly cmake options as suggested by @jakobj and @Silmathoron and @jougs' suggestion for full control. To be implemented asap and included in NEST 2.16.1.

@heplesser heplesser added S: Critical Needs to be addressed immediately and removed S: High Should be handled next labels Nov 26, 2018
@jakobj
Copy link
Contributor

jakobj commented Dec 3, 2018

i've now implemented the cmake option to set the bit sizes of various target members, see https://github.com/jakobj/nest-simulator/tree/feature/cmake-target-customization. However, this does not seem to solve the problem, since in various other places, we also assume maximal/minimal sizes of rank, thread id, syn id, lcid, see for example https://github.com/jakobj/nest-simulator/blob/feature/cmake-target-customization/nestkernel/target_data.h#L41. If a user chooses more than 8bits for syn id, this code will break. I will work on cleaning up this mess.

@jougs
Copy link
Contributor

jougs commented Dec 5, 2018

Many thanks for working on this and good luck with the cleansing ;-)

@heplesser
Copy link
Contributor Author

@jakobj Will #1088 fix the limitation on the number of synapse models?

@jakobj
Copy link
Contributor

jakobj commented Apr 9, 2019

yes, the standard setting for target-bits-split now supports 512 synapse models. the limitation remains however for the hpc setting.

aserenko added a commit to aserenko/nest-simulator that referenced this issue Jun 6, 2019
This reverts commit 54045ac.
Now that Issue nest#1043 is closed, there is no need for
such workaround. So, I restore the original version of
test_common_props_setting.sli, for it was cleaner.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Critical Needs to be addressed immediately T: Maintenance Work to keep up the quality of the code and documentation. ZC: Kernel DO NOT USE THIS LABEL ZP: Pending DO NOT USE THIS LABEL
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants