TFF Aggregators placement #1
Conversation
| multiple alternatives, be sure to use sub-sections for better separation of the | ||
| idea, and list pros/cons to each approach. If there are alternatives that you | ||
| have eliminated, you should also list those here, and explain why you believe | ||
| your chosen approach is superior. |
There was a problem hiding this comment.
I think this should terminate in an asterisk, this is throwing the formatting off elsewhere?
| have eliminated, you should also list those here, and explain why you believe | ||
| your chosen approach is superior. | ||
|
|
||
| Adding the `tff.AGGREGATORS` placement for federated types involves adding a new `Placement` and `PlacementLiteral`, and then extending the compiler to recognize federated values with this placement when computing intrinsics. The compiler generally defines separate intrinsics by placement; e.g., `tff.federated_value(value, placement)` is actually interpreted by the compiler as `federated_value_at_clients(value)` or `federated_value_at_server(value)`, depending on the provided `placement`. This means we we will want to add new intrinsics that correspond to `tff.AGGREGATORS`, e.g. `federated_value_at_aggregators`. |
There was a problem hiding this comment.
It looks like (possibly because of the missing asterisk above) hard line wrapping in your IDE stopped working?
|
|
||
| Adding the `tff.AGGREGATORS` placement for federated types involves adding a new `Placement` and `PlacementLiteral`, and then extending the compiler to recognize federated values with this placement when computing intrinsics. The compiler generally defines separate intrinsics by placement; e.g., `tff.federated_value(value, placement)` is actually interpreted by the compiler as `federated_value_at_clients(value)` or `federated_value_at_server(value)`, depending on the provided `placement`. This means we we will want to add new intrinsics that correspond to `tff.AGGREGATORS`, e.g. `federated_value_at_aggregators`. | ||
|
|
||
| Existing federated computation that will need modification fall into the two categories below: |
There was a problem hiding this comment.
It is not clear to me that we need to include non-federated intrinsics in this list? sequence_sum/reduce
There was a problem hiding this comment.
I took a deeper look, and you're likely right about not needing to modify the IntrinsicDefs for these. But their factory functions will need to be adjusted to account for the new placement due to checks like this one.
There was a problem hiding this comment.
The main reason I included them is that they need to be able to handle federated values that have placement tff.AGGREGATORS in order to maintain closure (as described later in the section), so maybe this calls for an additional section for "affected code" or similar.
There was a problem hiding this comment.
I'd actually suggest that (in the code you linked) the check should be pushed into the constructor of federated_map. Which it looks like it already is... https://github.com/tensorflow/federated/blob/c671f0b8857a3be18208db7a1c460f51650a7c60/tensorflow_federated/python/core/impl/intrinsic_factory.py#L177
|
@michaelreneer it looks like you were commenting on an outdated commit, hence some of those formatting issues you mentioned. The newer commit should resolve those. |
This works for me. I was considering expanding the Design Proposal to be more comprehensive in terms of changes that might be needed, but perhaps that can wait for more iteration? Feel free to share out unless there's anything obvious that we can address first. |
| - `sequence_map` | ||
| 2. Intrinsics that will need to be parameterized by placement, but currently | ||
| aren't. | ||
| - `federated_aggregate` |
There was a problem hiding this comment.
This will be probably the interesting one. I think the accumulate / merge / report operators should be allowed to be parameterized by the tff.AGGREGATORS-placed values.
| ecosystem. | ||
|
|
||
| ## Questions and Discussion Topics | ||
|
|
There was a problem hiding this comment.
Perhaps you can add a pointer to here, which is an example of current inefficiency that would be possible to address using tff.AGGREGATORS.
TF Federated Aggregators Placement
Rendered
Objective
This document proposes adding a
tff.AGGREGATORSplacement to the Federated Core (FC) in TensorFlow Federated (TFF). This would lift the requirement that all aggregations be computed ontff.SERVERwhile still allowing users to express custom aggregation logic using FC & TF.