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

Support for using file transfer mode via coreneuron embedded module #927

Merged
merged 5 commits into from
Jan 18, 2021

Conversation

pramodk
Copy link
Member

@pramodk pramodk commented Jan 17, 2021

  • add new option coreneuron.file_mode = True / False
  • if file_mode=True, the model is written to folder
    corenrn_data and coreneuron is launched
  • with file transfer mode, only spikes will be
    available
  • bug fix in the test test_spikes.py in MPI mode:
    mpi4py needs to be imported before importing neuron
  • add test for file_mode both in serial as well as
    mpi mode

I see two benefits with this PR:

  • Significant help a lot to test file transfer mode. Currently
    we have to write separate shell scripts to run special and
    special-core.
  • For GPU execution we can not use process per core but
    just few processes. But then model building part becomes
    slow. Based on this PR, we can build model with process
    per core and dump to the file. Then on CoreNEURON side
    only few ranks can load the data and utilise GPU using
    multiple OpenMP threads per rank.

cc: @iomaganaris @alexsavulescu

  * add new option coreneuron.file_mode = True / False
  * if file_mode=True, the model is written to folder
    corenrn_data and coreneuron is launched
  * with file transfer mode, only spikes will be
    available
  * bug fix in the test test_spikes.py in MPI mode:
    mpi4py needs to be imported before importing neuron
  * add test for file_mode both in serial as well as
    mpi mode
@pramodk pramodk requested a review from nrnhines January 17, 2021 16:30
Copy link
Member

@nrnhines nrnhines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me except it seems time to go ahead and implement the \todo item in nrnpy_hoc.cpp and have only the single get_nrncore_opt_value with a return type argument.
Perhaps the NEURON Datum union can be used to send back a int, double, or pointer to (copy?) of a c_str. (Or backcast a returned void*?)

@pramodk
Copy link
Member Author

pramodk commented Jan 17, 2021

except it seems time to go ahead and implement the \todo item in nrnpy_hoc.cpp and have only the single get_nrncore_opt_value with a return type argument.
Perhaps the NEURON Datum union can be used to send back a int, double, or pointer to (copy?) of a c_str. (Or backcast a returned void*?)

@nrnhines : I don't have complete picture about implementation aspects. Could we do this as separate PR?

@nrnhines
Copy link
Member

No problem. As soon as the checks are successful, I'll merge.

@nrnhines nrnhines merged commit 3f1f731 into master Jan 18, 2021
@nrnhines nrnhines deleted the pramodk/corenrn_file_mode branch January 18, 2021 12:33
Copy link
Member

@alexsavulescu alexsavulescu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, very nice addition. One thing to bring back though..

coreneuron.verbose = 0
h.stdinit()
corenrn_all_spike_t = h.Vector()
corenrn_all_spike_gids = h.Vector()

pc.spike_record( -1 if pc.id() == 0 else (pc.id()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By removing this we are no longer testing 2nd case when we iterate over PreSyns and match up vectors.

pramodk added a commit that referenced this pull request Jan 22, 2021
  - apply suggestion from review in #927
pramodk added a commit that referenced this pull request Jan 23, 2021
* Update test for better code coverage
  - apply suggestion from review in #927
* fix the test : gid is pc.id()+1 and not pc.id()
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.

None yet

3 participants