Copy #984

jgosmann · 2016-03-16T20:49:14Z

Addresses #977.

Nengo objects should be copyable and pickable now. There might be some weird effects for objects referencing other objects (e.g., connections). I might have to think about those and whether that works as expected (though, technically it works in some way).

Still todo:

Networks
FrozenObjects: Process, LearningRule, Synapse (also check for potential code duplication)
FIXMEs

hunse · 2016-03-16T21:11:21Z

FrozenObjects already have __getstate__ and __setstate__, so hopefully getting them working is as easy as just calling copy.copy in our own copy function.

jgosmann · 2016-03-17T19:16:43Z

I'm slightly surprised by the two failing test runs ... as the other test envs pass it is probably something going wrong with the warnings filtering?

jgosmann · 2016-03-17T19:49:11Z

😠 The debug commit made the tests pass ... what the hell?

jgosmann · 2016-03-17T20:04:46Z

I got a bit closer to the issue: Network.context is empty ... despite being in a with network block. Just not sure why it is empty.

jgosmann · 2016-03-18T15:50:23Z

@tbekolay Is py.test running tests in parallel on Travis-CI?

tbekolay · 2016-03-18T15:52:12Z

Yes, it runs with -n 2
On Mar 18, 2016 11:50, "Jan Gosmann" notifications@github.com wrote:

@tbekolay https://github.com/tbekolay Is py.test running tests in
parallel on Travis-CI?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#984 (comment)

jgosmann · 2016-08-29T20:19:35Z

I added a commit 41a0bba that makes all the tests pass, see the commit message for details and #1152 for related discussion.

I set the label back to "needs review" because these changes are more far reaching than just a simple fixup.

hunse · 2016-08-29T20:33:53Z

Those changes look okay to me.

Seanny123 · 2016-09-01T00:09:47Z

Found a bug that won't let connections with slices be pickled.

Given the following code:

import nengo
import cPickle as pickle

with nengo.Network() as model:
    out_nd = nengo.Node(size_in=1)

    osc = nengo.networks.Oscillator(0.1, 1, 300)


    nengo.Connection(osc.ensemble[0], out_nd)

with open("model", "w") as ff:
    pickle.dump(model, ff)

I get the error:

Traceback (most recent call last):
  File "pickle_test.py", line 14, in <module>
    pickle.dump(model, ff)
  File "/usr/lib/python2.7/copy_reg.py", line 84, in _reduce_ex
    dict = getstate()
  File "/home/saubin/git/nengo-copy/nengo/base.py", line 175, in __getstate__
    raise NotImplementedError("Nengo objects do not support pickling")
NotImplementedError: Nengo objects do not support pickling

jgosmann · 2016-09-01T00:17:47Z

I just looked over the code; it might work if you just remove the __getstate__ and __setstate__ methods on ObjView.

Seanny123 · 2016-09-01T00:22:11Z

Yep, just removing the code solved the problem! Thanks Jan!

jgosmann · 2016-09-01T03:13:44Z

One note is that I did change the public interface for NengoObject in this commit: 1b5290a

However, I don't think anyone is using this right now (it didn't require any changes to unit tests or examples) so I'm inclined to do this without deprecation and requiring a new major version...

nengo_gui is using it: nengo/nengo-gui#819

I suppose the options are:

Revert those changes.
Fix nengo_gui breaking compatibility to older Nengo versions.
Fix nengo_gui with keeping compatibility to previous Nengo versions.

The last option might lead to a lot of conditionals. So I would probably prefer option 1 or 2. The new interface is nice, but I'm fine with 1 if people think that the compatibility to older Nengo versions is important.

hunse · 2016-09-02T15:32:57Z

I'd be in favour of partially reverting the changes, and making params back into a property.

I could also go for option 2. The situation I worry about is someone upgrading Nengo, and not not realizing they also have to upgrade nengo_gui. How quickly does it fail?

I don't think option 3 really needs that many conditionals. We should just be able to do params = self.config._clsparams.params; params = params if is_iterable(params) else params() or something like that.

EDIT: I should clarify, I think params is fine as a property because calling it takes very little computation. In fact, I'm not sure if any real computation happens because it's just making an iterator, which I don't think should be evaluated much at all until someone starts iterating over it. I think the only real computation that happens right when params is called is an inspect.isclass(obj) and a dir(obj), both of which should be really fast.

EDIT2: So I should also clarify, I am most in favour of just making params back into a property, but I think any of the solutions are workable. If @tbekolay is okay with making params a property, then let's go with that.

Seanny123 · 2016-09-02T15:45:12Z

I could also go for option 2. The situation I worry about is someone upgrading Nengo, and not not realizing they also have to upgrade nengo_gui. How quickly does it fail?

It fails immediately by stopping you from running nengo_gui in any way.

jgosmann · 2016-09-02T19:23:15Z

I don't think option 3 really needs that many conditionals.

Yeah, it seems to be only that one place in the GUI.

jgosmann · 2016-09-07T16:18:35Z

I made the necessary changes to allow copies of object views and added some tests for that. Still waiting on @tbekolay to give his ok to revert params back into a property.

jgosmann · 2016-09-21T20:51:01Z

I added a commit to turn params back into a property. I haven't tested whether this fixes nengo/nengo-gui#819. Right now the property returns an iterator, but to me it seems that it is uncommon for properties to do that. So maybe we want to turn the interator into a tuple or list. This should not affect any existing code.

tbekolay · 2016-11-03T18:24:33Z

After a bit of head scratching, I decided the history of this branch is not too important and squashed this all into one commit with co-authors. I added a fixup (4790cbb) to change the properties that were returning iterators to instead return lists (or tuples where appropriate), which I think is the right call. I'll test this with the snippet in nengo/nengo-gui#819 to see if it works, but assuming it does, I'll merge this tomorrow morning (in case anyone wants to take a final look at this branch today).

jgosmann · 2016-11-03T18:51:43Z

nengo/tests/test_copy.py

-    catcher.__enter__()
-    warnings.simplefilter(
-        'error', category=NotAddedToNetworkWarning)
-    request.addfinalizer(lambda: catcher.__exit__(None, None, None))


Why did you remove this test?

It makes all the with warns(NotAddedToNetworkWarning) tests fail (since they error when the warning is raised). It seemed more complicated than it was worth to disable this check in the cases in which we wanted to check the warning. Any ideas on how to do it simply?

Ah right, this is a fixture not a test. Removing this LGTM.

Seanny123 · 2016-11-04T08:45:31Z

LGTM. Excited to see this merged!

This commit makes it possible to copy and pickle NengoObjects, and objects related to NengoObjects like Neuron and LearningRule. In order to make Neurons and LearningRules copyable, some changes were made to those classes: * When accessing special connection points like Ensemble.neurons or Connection.learning_rule we now recreate the corresponding object. Those objects are basically just indicators from or to where a connection is; they neither implement any behaviour on their own nor do they store a lot of data (except the back-reference to their parent). That resolves cyclic references (always good) and the performance and memory impact should be negligible. * The weak references in those classes are turned into strong references. Thanks to the broken reference cycle we don't have to worry about these being garbage collected anymore (and even with strong reference and a reference cycle they would be garbage collected sooner or later). The main benefit is that we don't have to manually handle those references when copying or pickling objects. * Methods for hashing and equality testing have been implemented on those classes to keep the builder working that uses them as keys in a dictionary. Unrelated to that, this seems to be the proper thing to do as two Neurons objects referring to the same ensemble should be treated as equal. None of these changes should affect user models. Some other miscellaneous cleanup is done in this commit as well, including removing some within function imports, and making some properties return lists instead of iterables. See the diff for more details. Co-authored by: Eric Hunsberger <erichuns@gmail.com> Co-authored by: Trevor Bekolay <tbekolay@gmail.com>

jgosmann added the work in progress label Mar 16, 2016

jgosmann self-assigned this Mar 16, 2016

jgosmann added this to the 2.1.1 release milestone Mar 16, 2016

jgosmann force-pushed the copy branch 4 times, most recently from c37f092 to 6c96a6a Compare March 17, 2016 18:32

jgosmann added needs review and removed work in progress labels Mar 17, 2016

jgosmann assigned jgosmann and unassigned jgosmann Mar 17, 2016

jgosmann added work in progress and removed needs review labels Mar 17, 2016

jgosmann force-pushed the copy branch from 6c96a6a to 0a1850d Compare March 17, 2016 18:41

jgosmann added needs review and removed work in progress labels Mar 17, 2016

jgosmann removed their assignment Mar 17, 2016

jgosmann force-pushed the copy branch 2 times, most recently from cdb2ee4 to 448eb56 Compare March 17, 2016 18:57

jgosmann added needs changes and removed needs review labels Mar 17, 2016

jgosmann force-pushed the copy branch from 2b1549d to 91cf4a6 Compare March 17, 2016 19:57

jgosmann force-pushed the copy branch 2 times, most recently from 4b7540b to 6f45862 Compare March 18, 2016 15:35

jgosmann removed the reviewed label Aug 29, 2016

Seanny123 added needs second review and removed needs review labels Aug 29, 2016

jgosmann added the needs changes label Sep 1, 2016

jgosmann removed their assignment Sep 7, 2016

jgosmann removed the needs changes label Sep 21, 2016

jgosmann mentioned this pull request Sep 21, 2016

Should be able to load pickled networks into GUI nengo/nengo-gui#819

Closed

tbekolay force-pushed the copy branch from a6d8697 to 4790cbb Compare November 3, 2016 18:22

jgosmann commented Nov 3, 2016

View reviewed changes

Seanny123 added reviewed and removed needs second review labels Nov 4, 2016

tbekolay force-pushed the copy branch from a66d201 to a3165d6 Compare November 4, 2016 13:53

tbekolay merged commit a3165d6 into master Nov 4, 2016

tbekolay deleted the copy branch November 4, 2016 14:10

tbekolay mentioned this pull request Dec 5, 2016

Serialize Nengo networks #680

Closed

jgosmann mentioned this pull request Nov 17, 2018

Allow FrozenObjects to control param init order #1493

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Copy #984

Copy #984

jgosmann commented Mar 16, 2016

hunse commented Mar 16, 2016

jgosmann commented Mar 17, 2016

jgosmann commented Mar 17, 2016

jgosmann commented Mar 17, 2016

jgosmann commented Mar 18, 2016

tbekolay commented Mar 18, 2016

jgosmann commented Aug 29, 2016

hunse commented Aug 29, 2016

Seanny123 commented Sep 1, 2016

jgosmann commented Sep 1, 2016

Seanny123 commented Sep 1, 2016

jgosmann commented Sep 1, 2016

hunse commented Sep 2, 2016 •

edited

Loading

Seanny123 commented Sep 2, 2016

jgosmann commented Sep 2, 2016

jgosmann commented Sep 7, 2016

jgosmann commented Sep 21, 2016

tbekolay commented Nov 3, 2016

jgosmann Nov 3, 2016

tbekolay Nov 3, 2016

jgosmann Nov 3, 2016

Seanny123 commented Nov 4, 2016

Copy #984

Copy #984

Conversation

jgosmann commented Mar 16, 2016

hunse commented Mar 16, 2016

jgosmann commented Mar 17, 2016

jgosmann commented Mar 17, 2016

jgosmann commented Mar 17, 2016

jgosmann commented Mar 18, 2016

tbekolay commented Mar 18, 2016

jgosmann commented Aug 29, 2016

hunse commented Aug 29, 2016

Seanny123 commented Sep 1, 2016

jgosmann commented Sep 1, 2016

Seanny123 commented Sep 1, 2016

jgosmann commented Sep 1, 2016

hunse commented Sep 2, 2016 • edited Loading

Seanny123 commented Sep 2, 2016

jgosmann commented Sep 2, 2016

jgosmann commented Sep 7, 2016

jgosmann commented Sep 21, 2016

tbekolay commented Nov 3, 2016

jgosmann Nov 3, 2016

Choose a reason for hiding this comment

tbekolay Nov 3, 2016

Choose a reason for hiding this comment

jgosmann Nov 3, 2016

Choose a reason for hiding this comment

Seanny123 commented Nov 4, 2016

hunse commented Sep 2, 2016 •

edited

Loading