-
Notifications
You must be signed in to change notification settings - Fork 33
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
Adds modular SERDE approach #175
Conversation
fbfc9e1
to
3f98ce6
Compare
Update: just need to test more, but I think I have a good design (and update libraries to latest locally...) core.serde has functions. People can then register their own for their application -- I haven't tested overriding but I assume it'll work. Tracker models should be dumb -- so instead I push serialization calling into hooks. Once I merge and rebase off of #215 can add persister tests that will also then check for this... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Largely looks good, I'm confused about the serde kwargs TBH.
Obviously needs docs but I imagine that'll come later.
6f96150
to
5def700
Compare
burr/core/serde.py
Outdated
from functools import singledispatch | ||
from typing import Any, Union | ||
|
||
KEY = "__bur_serde__" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
burr_serde
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, docs + small items
It was lost somehow / bad before.
f19d3e5
to
a815aae
Compare
This code introduces a core module for SERDE. State relies on this to know how to serialize and deserialize itself. A design decision made was that we will use singledispatch style registration based on types for serialization, and a string key value for deserialization. Serialization always outputs a dictionary that can then be JSON serialized somewhere. A key is added to that to know how to deserialize things. Field level SERDE specification is still possible, but hasn’t been implemented yet. Otherwise people interacting with State object should use the methods defined on it. The core.serde module can be used in other contexts easily too — so if trackers and persisters want to utilize it they can. One constraint is that all SERDE functions take in **kwargs. That way people could wire through some interesting args to do more interesting serialization, e.g. like for pickle, or pandas. The kwargs are really a dict of kwargs, where each serde function/module should namespace its kwargs to avoid clashes. If in the future we need to change what is registered as SERDE, that’s also possible to adjust. The only limitations this design imposes is a global way to do SERDE. Field level will come, and make it even more customizeable, otherwise we think this approach should cover most use cases. This change also fixed a bug in the tracker when looking for sequence ids, because it hadn’t anticipated spans… Squashed commits: [6cbc9ee] Flips print to debug [b9b7dda] Fixes broken tests due to deps [9daf325] Updates code and adds tests This adds tests and removes code. [20579ac] Fixes bugs with incomplete file log state File logs could be incomplete. So this checks to find the last entry that is valid for that particular context. [47d3288] WIP: Adds more docs and adds pickle This commit will be cleaned up. But adds more docs, and a way to register classes to be pickled! [1a1efb2] Refactors serde to proper place and adds POC This is WIP -- but it seems to work via unit tests. Uses **kwargs for future proofing, but also allowing one to pass in values to serde -- idea is that each would have some namespace of kwargs. Just now need to test it for the tracker. TODO: allow one to register field level state serde. [3762aa9] Adds prototype module to handle serde Singledispatch for type serialization. For non-primitive types, make them return a dict with a serde key. Then use a string value based mechanism to deserialize. This way we can be "field agnostic" during deserialization. Otherwise deserialization without knowing what it is, is a challenge. Thus the idea to include a key in a dict for it. Otherwise the only other way for deserialization is to field registries... TODO: - determine module naming and location - actually wire in - proper tests - how to override these - think about any other edge/error cases
a815aae
to
7e8d1d6
Compare
This PR introduces a modular SERDE approach.
of a "string dispatcher function". i.e. like single dispatch, but you register a string value for it. This approach
means that most people will get around the need for field level deserialization / having to tell Burr about state types explicitly.
This skips field level serde registration for now. But leaves room for it.
Otherwise this custom serde for:
TODOs:
wire changes through to UI reading local tracker logsNot required.Changes
How I tested this
Notes
Checklist