Serialization v1.0 #241
Serialization v1.0 #241
Conversation
arcondello
force-pushed
the
feature/serialization-v1
branch
from
420685c to
78c91f2
Compare
Specifically change Symbol._state_from_zipfile() and Symbol._state_into_zipfile() to the plural Symbol._states_from_zipfile() and Symbol._states_into_zipfile(). Also Nodes now broadcast whether or not their state is deterministic. Previously we used their status as a decision or no to decide which nodes get their states saved. Now we save the state of any node with a non-deterministic state.
arcondello
force-pushed
the
feature/serialization-v1
branch
2 times, most recently
from
f4e808e to
3de57c8
Compare
arcondello
force-pushed
the
feature/serialization-v1
branch
from
f654205 to
afc55fe
Compare
arcondello
force-pushed
the
feature/serialization-v1
branch
from
b5192e4 to
4268631
Compare
|
|
||
| The serialization version is saved in a file ``version.txt``. | ||
|
|
||
| The states have the following structure. |
There was a problem hiding this comment.
This should note that only the states of decisions and other non-deterministic symbols is saved. I will fix.
There was a problem hiding this comment.
the code below for v1.0 uses only_decision=False
There was a problem hiding this comment.
Yes, because it's using the if symbol.deterministic_state(): continue logic instead. Before we just hard-limited it to decisions which gave the same thing if you don't have non-determistic intermediate nodes (which we currently do not, but want to add).
| if symbol._deterministic_state(): | ||
| continue |
There was a problem hiding this comment.
I see advantages of skipping deterministic states on load (since source of truth), but less so on save. Sure, we'll save some space, but to read out (construct) all the states, one now has to construct the whole model.
There was a problem hiding this comment.
In many models saving all states would increase the state size by orders of magnitude. The way it's set up currently the state of intermediate variables is calculated lazily rather than eagerly.
There was a problem hiding this comment.
Inspecting states of large models is a use case I was thinking about. Perhaps you don't want to construct the huge model just to check states if debugging? So, not serializing by default is fine, but maybe leave that option open for users?
|
|
||
| The serialization version is saved in a file ``version.txt``. | ||
|
|
||
| The states have the following structure. |
There was a problem hiding this comment.
the code below for v1.0 uses only_decision=False
tests/test_states.py
Outdated
There was a problem hiding this comment.
I would add serialization tests for model and states that verify loading/failing of mismatched version (0.1 in 1.0 and 1.0 in 0.1).
There was a problem hiding this comment.
Totally agree. That's in the next PR.
Edit: added some simple ones in e6e4379. More forthcoming.
Note this PR does not increment the default serialization version. We'll do that in 0.7.0.
In a followup PR that I am already working on, I will add CI testing backwards and forwards in time checking that models serialized by package version 0.5.2 are loadable from 0.6.0+ and vice versa.
The docs need another pass or two for clarity, but better than the
# TODOthat was there before 🙂