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

A standard way for Simulators to free resources #739

Closed
tcstewar opened this issue Jun 14, 2015 · 7 comments
Closed

A standard way for Simulators to free resources #739

tcstewar opened this issue Jun 14, 2015 · 7 comments
Milestone

Comments

@tcstewar
Copy link
Contributor

Some backends need ways to indicate that they are finished and their resources are no longer needed. For example, nengo_spinnaker needs to put the SpiNNaker machine back into a clean state so that new programs can be loaded on (without rebooting the board).

Right now, the nengo_spinnaker implementation does this with a sim.close() call. You can call sim.run(time) multiple times, and then do a sim.close() when you are finished with the simulator. You cannot call sim.run(time) after calling sim.close().

It also supports using the Simulator as a context manager, with an empty __enter__ and a __exit__ that just calls self.close(). Using the context manager is strictly optional.

Right now, you must call close after running a Simulation. Otherwise, the board gets left in a state that means it needs to be physically reset before loading a new program. It might be possible to add an atexit handler to do this, at the cost of a global list of un-closed Simulators (and this would be in addition to the close system, since we need to support running one Simulator and then another within the same program, such as nengo_gui).

As was discussed a bit in #737 , it's not clear whether we should add a sim.close() to all Simulators (and have it do nothing if the Simulator doesn't need to be closed), or whether this should be something that each backend that needs it can implement in their own way. I guess the first question is, which backends could use something like this? Would it be useful in nengo_ocl? What about nengo_mpi or nengo_bluegene?

The second question is, if we do make a close() method standard, should we also include the context manager version?

On the first question, I think this is something that will be needed with hardware backends if they are to run in nengo_gui. I'd like to add an atexit handler to nengo_spinnaker so that if people forget the close when writing scripts it will still get closed, but something explicit like this is still needed if people want to run multiple simulations in the same script. I can foresee a similar thing happening with nengo_brainstorm and nengo_mpi. So I think it'd be nice to just add a standard .close() method to all Simulators. Of course, we can always just do a if hasattr(sim, 'close'): sim.close() instead.

On the second question, I'd vote that if we do support a .close() method we should also support being a context manager. It's only 4 lines of code, and this is exactly the sort of situation context managers are for.

@mundya
Copy link
Contributor

mundya commented Jun 14, 2015

On the second question, I'd vote that if we do support a .close() method we should also support being a context manager. It's only 4 lines of code, and this is exactly the sort of situation context managers are for.

Moreover it fits that close is the same method name as for sockets/files/... so the least surprising thing for us to do is have a context manager like them. I hadn't come across atexit before, that's pretty cool.

@jgosmann
Copy link
Collaborator

+1 atexit
+1 .close()
+1 context manager

I think, it would be good to have a standard that all backends adhere to. It would be annoying if each backend has it's own way of closing.

@jgosmann
Copy link
Collaborator

I would propose the following:

  1. Each backend should leave the hardware in a clean state after the script execution ends (with atexit).
  2. Each simulator should have a close function which:
    1. May do nothing
    2. Guarantees the hardware to be in a clean state and usable by another simulator.
    3. After calling close further calls to run are allowed to raise an error or fail in other ways. But they don't have to. (So the default simulator can implement close as a noop.)
  3. Each simulator with a close function can be used as context manager.

As long as no more than one simulator is created in a script, everything in those scripts can be kept as is. If more than one simulator in one script is created, one can either manage simulators with explicitly calling close or use a context manager is simple cases.

Note that the following works with a context manager:

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

data = sim.data[probe]  # The sim object still exists to read out data, but run() calls may fail

@tcstewar
Copy link
Contributor Author

That proposal sounds right to me (and nice clarification on the interaction between run and close in 2.iii.).

I think I also agree with allowing sim.data after sim.close.... @mundya , does nengo_spinnaker support that? Or does it completely shut things down on close?

@mundya
Copy link
Contributor

mundya commented Jun 15, 2015

nengo_spinnaker supports that.

@e2crawfo
Copy link
Contributor

nengo_mpi would definitely benefit from this, and +1 to what's been proposed so far.

@tbekolay tbekolay added this to the 2.1.0 release milestone 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
jgosmann added a commit that referenced this issue Sep 30, 2015
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
@jgosmann
Copy link
Collaborator

This issue should be resolved with PR #859.

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