[feat] refactor data storage to allow saving on a per-window basis #862
Conversation
…to these modes
parser.add_argument('-cache_type', metavar='cache_type', type=str,
default=DEFAULT_CACHE_TYPE,
help='''specify how the received data should be synced
between memory and disk.
- JPE/OneJsonPerEnv: one json per environment
- JPW/OneJsonPerWindow: one json per window
- JPWA/OneJsonPerWindowAutoSave: one json per window,
autosave to disk when plotting''')
…to these modes
parser.add_argument('-cache_type', metavar='cache_type', type=str,
default=DEFAULT_CACHE_TYPE,
help='''specify how the received data should be synced
between memory and disk.
- JPE/OneJsonPerEnv: one json per environment
- JPW/OneJsonPerWindow: one json per window
- JPWA/OneJsonPerWindowAutoSave: one json per window,
autosave to disk when plotting''')
There was a problem hiding this comment.
Hi @shouldsee. Thanks for putting up a PR. I'll admit, this one is pretty difficult to review for a number of reasons.
- Class naming here is somewhat obtuse.
B,NIF, etc - Without explanations for these classes both inline and in some kind of overall explanation, it's hard to follow your intent and methodology.
- Overall, these additions should exist outside the
server.pyfile, which is already bloated (and part of why we're trying to refactor). - Addition of
Makefileis complicating the devX here. - Last, there are some comment-ed out changes (like in
demo.py), which we really shouldn't be maintaining in the codebase, alongside conditionals such asif 1:
I'm open to the feature change you're proposing here, but I'm definitely not ready to fully review, let alone accept, in the current state. Would love to see it all in a more final state.
| @@ -20,7 +20,7 @@ class TextPane extends React.Component { | |||
| switch (e.type) { | |||
| case 'keydown': | |||
| case 'keypress': | |||
| e.preventDefault(); | |||
| // e.preventDefault(); /// this would prevent entering text into input box | |||
There was a problem hiding this comment.
IIRC this was trying to prevent text panes from submitting on enter. Plus, the goal is to send keypress events to the backend, not necessarily to have them render up-front here.
There was a problem hiding this comment.
Ah I see. I was putting forms into visdom window, and .preventDefault would make the form inputting fairly laggy, hence commented out.
There was a problem hiding this comment.
Perhaps there's value in a new FormPane component, likely a parent class for the PropertiesPane. This would then be able to accomplish what you're attempting in #868, but in a cleaner way
| @@ -0,0 +1,21 @@ | |||
| clean: | |||
There was a problem hiding this comment.
I'm somewhat against adding yet another scripting flow to the visdom codebase. This should either be part of the package.json scripts, or we should be having a discussion about the alternate we should be using instead.
| 'Visdom failed to handle a handler for {}: {}' | ||
| ''.format(message, e) | ||
| 'Visdom failed to handle a handler for {}: {}.' | ||
| '\n traceback:{}'.format(message, e, tbmsg) |
There was a problem hiding this comment.
This is a nice-to-have addition
Description
SimpleJsonStorageSimpleWindowJsonStorageMotivation and Context
#861 . changing towards a per-window caching scheme
How Has This Been Tested?
rm -rf ./test_data/*python server.py -env_path test_datapython test_simple.pycreate plots an saveenv:testis correctly loadedScreenshots (if appropriate):
Types of changes
Checklist: