Implement Serialize/Deserialize for ActionStage #1186
Conversation
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable, and 1 discussions need to be resolved
nativelink-util/src/action_messages.rs line 769 at r1 (raw file):
} impl Serialize for ActionStage {
I fought this for a few hours over the weekend and today, but while doing Serialize is fairly simple without a wrapper type is fairly simple, I couldn't figure out how to do this for Deserialize without implementing more complex things like Visitors. If @jhpratt or @caass have more experience with this please let me know if there's a better way.
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Cargo Dev / macos-13, Installation / macos-13, Installation / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, and 2 discussions need to be resolved
nativelink-util/src/action_messages.rs line 769 at r1 (raw file):
Previously, zbirenbaum (Zach Birenbaum) wrote…
I fought this for a few hours over the weekend and today, but while doing Serialize is fairly simple without a wrapper type is fairly simple, I couldn't figure out how to do this for
Deserializewithout implementing more complex things likeVisitors. If @jhpratt or @caass have more experience with this please let me know if there's a better way.
I have implemented deserializers with visitors. My recommendation is to avoid it if reasonably possible — it's not fun and is extremely error prone.
nativelink-util/src/action_messages.rs line 825 at r1 (raw file):
} impl<'de> Deserialize<'de> for ActionStage {
Unless I'm mistaken, there's a helper attribute from serde that lets you derive Deserialize by way of another type, essentially avoiding this manual impl.
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained, and 2 discussions need to be resolved
nativelink-util/src/action_messages.rs line 825 at r1 (raw file):
Previously, jhpratt (Jacob Pratt) wrote…
Unless I'm mistaken, there's a helper attribute from
serdethat lets you deriveDeserializeby way of another type, essentially avoiding this manual impl.
You're referring to deserialize_with right? I tried it out but enums make it a little more complicated unless I'm missing something
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1.
Reviewable status: 1 of 1 LGTMs obtained, and 2 discussions need to be resolved
nativelink-util/src/action_messages.rs line 825 at r1 (raw file):
Previously, zbirenbaum (Zach Birenbaum) wrote…
You're referring to
deserialize_withright? I tried it out but enums make it a little more complicated unless I'm missing something
Yes, that's the attribute. If it's simpler to do it this way, then by all means do that. It's not as if it's significant overhead.
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and 2 discussions need to be resolved
nativelink-util/src/action_messages.rs line 769 at r1 (raw file):
Previously, jhpratt (Jacob Pratt) wrote…
I have implemented deserializers with visitors. My recommendation is to avoid it if reasonably possible — it's not fun and is extremely error prone.
Is this to work around CompletedFromCache(ProtoActionResult) ? I thought there was some patch that allowed for the protobuf generator to sprinkle Serialize / Deserialize on those protos to make them compatible with serialization traits
There was a problem hiding this comment.
Reviewable status: 1 of 1 LGTMs obtained, and 2 discussions need to be resolved
nativelink-util/src/action_messages.rs line 769 at r1 (raw file):
Previously, adam-singer (Adam Singer) wrote…
Is this to work around
CompletedFromCache(ProtoActionResult)? I thought there was some patch that allowed for the protobuf generator to sprinkleSerialize/Deserializeon those protos to make them compatible with serialization traits
Unfortunately while we can put those derive flags on all the structs we generate, they are missing on the underlying Prost types. There’s definitely a way to get it working, but it’s likely a larger undertaking than it seems at first glance.
I do think there’s a way to do this without cloning so that’s not to say this PR can’t be improved
zbirenbaum
force-pushed
the
action-stage-serialize
branch
2 times, most recently
from
393837d to
434b95b
Compare
Implements Serialize/Deserialize for ActionStage to allow for storage and retrieval from stores such as Redis. towards: TraceMachina#1182
zbirenbaum
force-pushed
the
action-stage-serialize
branch
from
434b95b to
29653ff
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Installation / ubuntu-22.04, Remote / large-ubuntu-22.04, and 2 discussions need to be resolved
There was a problem hiding this comment.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: Remote / large-ubuntu-22.04, and 1 discussions need to be resolved
nativelink-util/src/action_messages.rs line 769 at r1 (raw file):
Previously, zbirenbaum (Zach Birenbaum) wrote…
Unfortunately while we can put those derive flags on all the structs we generate, they are missing on the underlying Prost types. There’s definitely a way to get it working, but it’s likely a larger undertaking than it seems at first glance.
I do think there’s a way to do this without cloning so that’s not to say this PR can’t be improved
Improved this a lot and think everyone is happy with where it's at.
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: Remote / large-ubuntu-22.04
Implements Serialize/Deserialize for ActionStage to allow for storage and retrieval from stores such as Redis. towards: TraceMachina#1182
Description
Implements Serialize/Deserialize for ActionStage to allow for storage
and retrieval from stores such as Redis.
towards: #1182
Type of change
Checklist
bazel test //...passes locallygit amendsee some docsThis change is