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

Simulator should have close() method #857

Closed
jgosmann opened this issue Sep 29, 2015 · 12 comments
Closed

Simulator should have close() method #857

jgosmann opened this issue Sep 29, 2015 · 12 comments
Milestone

Comments

@jgosmann
Copy link
Collaborator

I think the Simulator class should have a close method because some backends require closing (e.g. SpiNNaker). This would make it easier to write code independent of the backend (which comes up in my spaopt branch).

In addition to that I would vote to allow a the simulator in a with statement like so:

with nengo.Simulator(model) as sim:
    sim.run(1.)

(I think we had an unresolved discussion on this in some other issue already.)

@jgosmann jgosmann added this to the 2.1.0 release milestone Sep 29, 2015
@hunse
Copy link
Collaborator

hunse commented Sep 29, 2015

Another useful thing I found recently is the atexit module. It allows you to register functions to run when the program is exiting. We could use this, combined with a global weakref list of simulators (or something), to make sure that all simulators are closed on exit. This saves users having to use the with statement (though we can still have that functionality for users who want their simulator to close earlier).

@jgosmann
Copy link
Collaborator Author

Doesn't solve my problem at hand because I want to create another simulator after closing the first one in the same script. But maybe it makes sense to implement this in general. (I believe nengo_spinnaker might already be doing this?)

@hunse
Copy link
Collaborator

hunse commented Sep 29, 2015

As I said, we'd still have the option to manually close. The atexit is just to make sure that if users don't close, the board still gets cleaned up.

@e2crawfo
Copy link
Contributor

See #739, I (roughly) followed the recommendations there in nengo_mpi. https://github.com/e2crawfo/nengo_mpi/blob/operational/nengo_mpi/simulator.py

@hunse
Copy link
Collaborator

hunse commented Sep 29, 2015

Yes, it seems that we just repeated that conversation, just with different people 😄 Good that we're all on the same page now.

@tcstewar
Copy link
Contributor

:)

One thing that comes to mind that didn't come up in the previous conversation: should .close() on the reference Simulator be a no-op? Or should it set a flag so that subsequent calls to .run() raise an Exception?

And can you .close() more than once?

@hunse
Copy link
Collaborator

hunse commented Sep 29, 2015

I would say a) .close() on the reference simulator should stop subsequent calls to .run and b) you can close more than once (the subsequent calls will just do nothing).

@jgosmann
Copy link
Collaborator Author

That seems sensible to me too.

@tbekolay
Copy link
Member

I was the main dissenting voice on the .close() stuff, but yeah, the consensus is pretty overwhelming so this all sounds good to me.

Makes sense for consistency that close() in the reference sim should stop subsequent calls to .run. I think in the future we will have some reasons to use close(); mostly I'm thinking about when we have the ability to write to files.

Closing more than once seems fine to me. I just tried this with file pointers in Python; you can call close multiple time, so there's precedent for that.

One question that comes to mind: should nengo manage open simulators for all backends? Or should we leave this up to them? Both SpiNNaker and MPI have their own (mostly identical) code for managing existing simulators and closing them safely with atexit. Should we include this in nengo, with something like a nengo.open_simulators list? And some functions like nengo.close_simulators(sim_type=None)?

jgosmann added a commit that referenced this issue Sep 30, 2015
jgosmann added a commit that referenced this issue Sep 30, 2015
jgosmann added a commit that referenced this issue Sep 30, 2015
jgosmann added a commit that referenced this issue Sep 30, 2015
jgosmann added a commit that referenced this issue Sep 30, 2015
@tcstewar
Copy link
Contributor

One question that comes to mind: should nengo manage open simulators for all backends? Or should we leave this up to them? Both SpiNNaker and MPI have their own (mostly identical) code for managing existing simulators and closing them safely with atexit. Should we include this in nengo, with something like a nengo.open_simulators list? And some functions like nengo.close_simulators(sim_type=None)?

I don't think we should have core nengo manage this. It might make sense in the future if we find that a lot of backends are using similar code to do this management, but at the moment things are so different that I'd prefer to let the backends handle this however they want to independently.

@hunse
Copy link
Collaborator

hunse commented Sep 30, 2015

I don't think we should have core nengo manage this.

I was thinking the opposite. It would be nice to have core Nengo do the atexit stuff to make sure close is called, or at least have helper functions or classes in there for backends to use if they want it. An open_simulators list sounds good, as does a close_simulators function. The close method itself is always going to be different, of course.

@tbekolay
Copy link
Member

My opinion is that core Nengo should handle this too. If a backend needs something done other than just calling close it can do that, but all backends will need close called, so it doesn't seem like we're handcuffing backends at all by making this possible.

tbekolay pushed a commit that referenced this issue Jan 14, 2016
tbekolay pushed a commit that referenced this issue Jan 14, 2016
tbekolay pushed a commit that referenced this issue Jan 15, 2016
tbekolay pushed a commit that referenced this issue Jan 15, 2016
Seanny123 pushed a commit that referenced this issue Feb 12, 2016
Seanny123 pushed a commit that referenced this issue Feb 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants