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

Force config save before load #773

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Force config save before load #773

wants to merge 1 commit into from

Conversation

Seanny123
Copy link
Collaborator

Resolves #763

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @tcstewar to be a potential reviewer

@tbekolay
Copy link
Member

Quick question: how many hours of work was this one-line fix? Just curious 😜 🐙 🌈

@Seanny123
Copy link
Collaborator Author

8 hours. 1 hour to locate the bug. 6 hours to get confused between the division between the client updating code and the server updating code. 1 hour to just talk to Terry for a bit and fix the actual problem.

@tbekolay
Copy link
Member

@jgosmann jgosmann self-assigned this Jun 13, 2016
@jgosmann
Copy link
Collaborator

I would like to take a look at this before merging.

@jgosmann
Copy link
Collaborator

This doesn't fix the issue for me. Steps used to test (not necessarily minimal):

  1. Open nengo with a new blank file.
  2. Type:
import nengo

with nengo.Network() as model:
    ens = nengo.Ensemble(10, 1)
  1. Move ens
  2. Type another line: b = nengo.Ensemble(10, 1)
  3. Insert a newline and type xi, wait for compilation error
  4. Remove xi to have it compile fine again
  5. Move b

Console output:

Error processing: "{"act":"pos","uid":"b","x":0.2776996968904669,"y":0.7480387765804274}"
Traceback (most recent call last):
  File "/home/jgosmann/Documents/projects/nengo_gui/nengo_gui/server.py", line 136, in ws_viz_component
    component.message(msg)
  File "/home/jgosmann/Documents/projects/nengo_gui/nengo_gui/components/netgraph.py", line 413, in message
    act = nengo_gui.user_action.create_action(action, self, **info)
  File "/home/jgosmann/Documents/projects/nengo_gui/nengo_gui/user_action.py", line 16, in create_action
    return Pos(net_graph, **kwargs)
  File "/home/jgosmann/Documents/projects/nengo_gui/nengo_gui/user_action.py", line 215, in __init__
    self.x, self.y = self.obj_config.pos
TypeError: 'NoneType' object is not iterable

Concerns apart from not fixing the issue:

  • The name load_config is misleading if it also saves the config.
  • A function returning a value should have no side effects (like saving). (Sometimes this is necessary, but people tend to assume that function either return something or have a side effect, but not both.)
  • Are there any downsides to saving the config that often? (I think fixing the problem is more important, but we might want to keep that in mind for later refactoring?)

Unrelated to this PR ... but why is there a force and a lazy argument in the save_config function signature? That is very confusing.

@Seanny123
Copy link
Collaborator Author

Seanny123 commented Jun 14, 2016

This doesn't fix the issue for me.

Back to the drawing board then.

Downsides to saving the config that often?

High disk usage which kills the battery power of laptops and the lifetime of hard drives?

@jgosmann
Copy link
Collaborator

High disk usage which kills the battery power of laptops and the lifetime of hard drives?

Though, it's probably buffered anyways ...

@jgosmann jgosmann removed their assignment Jun 14, 2016
@tcstewar
Copy link
Collaborator

Downsides to saving the config that often?

The downside I had in mind when I wrote this originally was the time taken to iterate through the entire graph of objects, read everything, and create the config. But this might be yet another example of premature optimization.....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants