Extract ComputationAction from OperationMessage#3132
Extract ComputationAction from OperationMessage#3132Jasopaum merged 22 commits intoOpenMined:masterfrom
Conversation
|
@Jasopaum Operation and OperationMessage now have separate (but related) msgpack serialization methods. The next step is to extract Operation's Protobuf serialization from the existing methods on OperationMessage into new methods on Operation. |
karlhigley
changed the title
karlhigley
changed the title
syft/messaging/message.py
Outdated
| return (message, self.action.return_ids) | ||
|
|
||
| @staticmethod | ||
| def simplify(worker: AbstractWorker, ptr: "OperationMessage") -> tuple: |
There was a problem hiding this comment.
Are we planning to keep "simplify" and "detail' around long term given Protobuf?
There was a problem hiding this comment.
No, but they still need to work for the time being since Protobuf serialization isn’t complete yet.
syft/messaging/message.py
Outdated
| sy.serde.protobuf.proto.set_protobuf_id(protobuf_op.return_ids.add(), return_id) | ||
|
|
||
| protobuf_op_msg.operation.CopyFrom(protobuf_op) | ||
| protobuf_op = ComputationAction.bufferize(worker, operation_message.action) |
There was a problem hiding this comment.
Is there an intentional design choice in "bufferize" being used functionally instead of as a method on operation_message.action?
There was a problem hiding this comment.
Yes, there is, but it originally comes from msgpack serialization. If I recall correctly, it’s because there’s no object to call detail/unbufferize on; the object is the output of those methods (rather than an input.) As a result, those methods have to be static, and making simplify/bufferize static too creates a parallel structure for serialization in both directions.
iamtrask
left a comment
iamtrask
left a comment
There was a problem hiding this comment.
I left some light questions but nothing blocking.
karlhigley
changed the title
Jasopaum
left a comment
Jasopaum
left a comment
There was a problem hiding this comment.
Still need to change the syft-proto requirement but otherwise, LGTM
LaRiffle
left a comment
LaRiffle
left a comment
There was a problem hiding this comment.
If I read correctly your msg from syft-core, and those actions should be more specifically ComputationAction?
Or do we allow for communication in plan.actions?
Based on your terminology, I'm 100% ok to rephrase all the attributes on plan.py when we're set on the naming.
|
@LaRiffle Yeah, the actions in I suppose we could also build in a guard clause that would prevent other types from being passed in as actions or something. 🤔 |
karlhigley
force-pushed
the
refactor/actions
branch
from
7b2c7c2 to
75de3a3
Compare
|
(Rebased this PR on top of the Plan changes in #3080.) |
4 participants
An early step toward #3008. Depends on OpenMined/syft-proto#47.