Refactor the hydroflow graph API to simplify the surface API #58

MingweiSamuel · 2022-01-28T20:29:58Z

We inherited the idea that subgraphs are created first, then are linked up to each other. This works okay, but requires waiting for an input/output port pair to be connected before the handoff state can be created, resulting in awkward Rc<RefCell<>> passing around to link things together.

This change swaps the order subgraphs/handoffs are created. Handoffs are created first and their state is immediately inserted into the hydroflow struct. Then it is up to the user to provide the already-created input/output ports when adding a new subgraph.

This removes a lot of Rc<RefCell<>> indirection.
This improves type inference since types are provided as arguments rather than having to be inferred as the return type.
This matches the structure of the surface API (handoffs first), so we can simplify/remove the connecting layer of the surface API implementation.
This makes code reuse (pulling graph code into separate functions) easier, potentially, since the implementation is simpler.
Allows the handoffs to modify their state while still in the graph assembly phase - useful for teeing handoffs
Better error message?

Example

Before:

type MyInputList = tt!(VecHandoff<...>, VecHandoff<...>);
type MyOutputList = tt!(VecHandoff<...>);
let (tl!(input_a, input_b), tl!(output_c)) = hydroflow.add_subgraph::<MyInputList, MyOutputList, _>(
  |ctx, tl!(recv_a, recv_b), tl!(send_c)| {
    ...
  }
);

hydroflow.connect_edge(output_c, ...);
hydroflow.connect_edge(..., input_a);
hydroflow.connect_edge(..., input_b);

After:

let (input_a, ...) = hydroflow.make_handoff::<VecHandoff<...>>();
let (input_b, ...) = hydroflow.make_handoff::<VecHandoff<...>>();
let (..., output_c) = hydroflow.make_handoff::<VecHandoff<...>>();

hydroflow.add_subgraph(
  tl!(input_a, input_b), tl!(output_c),
  |ctx, tl!(recv_a, recv_b), tl!(send_c)| {
    ...
  }
);

MingweiSamuel · 2022-02-03T19:44:30Z

Changes

Surface API `Connector` layer removed

Surface API handoff simplified

No more Rc<Cell<>>s

GraphExt functions are now created by a macro and are renamed to be more consistent:

add_subgraph_sink
add_subgraph_2sink
add_subgraph_source
add_subgraph_in_out
add_subgraph_in_2out
add_subgraph_2in_out
add_subgraph_2in_2out

Demux API is commented out for now, should be replaced with a special kind of handoff

HandoffList is not used anymore, instead there are `PortList` and `PortListSplit`

This is better for type inference in the new core API

`Input`/`OutputPort`s and `Send`/`RecvCtx`s are simplified

No more Rc<RefCell<>> junk.
Now use a common BasePort or BaseCtx to share implementation, keep code DRY.

`add_subgraph_n_in_m_out` renamed to `add_subgraph_n_m`

`A: Extend<B>, <A as Extend<B>>::Extended: Spit<A, Suffix = B>` trait bounds copied throughout

Adds a bunch of lines and is not DRY, but is necessary for better diagnostics when writing abstraction code

Improved teeing in query API

in

impl<T: Clone> Operator<T> {
    pub fn tee(self, n: usize) -> Vec<Operator<T>>

MingweiSamuel · 2022-02-03T19:52:16Z

TODOs in the future

Network APIs and add_input need to accept send/recv ports as arguments instead of returning them. This is needed if you are connecting a pair of network ports (or input) directly together (forwarding)
Rename InputPort/OutputPort to SendPort/RecvPort (not necessarily respectively?)
Rename/update BasePort/BaseCtx/BasePortList, don't use <const S: bool>, instead use a more nicely named tag struct.

MingweiSamuel · 2022-02-03T20:00:46Z

hydroflow/src/scheduled/query.rs

+        let mut sends = Vec::with_capacity(n);
+        let mut recvs = Vec::with_capacity(n);
+        for _ in 0..n {
+            let (send_port, recv_port) = df.make_handoff::<VecHandoff<T>>();
+            sends.push(send_port);
+            recvs.push(Operator {
                df: self.df.clone(),
-                output_port,
-            })
-            .collect()
+                recv_port,
+            });
+        }
+
+        df.add_subgraph_n_m(vec![self.recv_port], sends, move |_ctx, recvs, sends| {
+            let input = recvs.iter().next().unwrap().take_inner();
+            if let Some((&last_output, outputs)) = sends.split_last() {
+                for output in outputs {
+                    output.give(Iter(input.iter().cloned()));
+                }
+                last_output.give(Iter(input.into_iter()));
+            }
+        });
+
+        recvs


Improves the query API tee, only real change in the runtime code in this diff

hydroflow/tests/mux_demux.rs

justinj

Looks like a big improvement to me as far as simplicity of the code goes 👍

justinj · 2022-02-03T20:16:28Z

hydroflow/src/scheduled/graph.rs

    }

-    pub fn add_edge<H>(&mut self, output_port: OutputPort<H>, input_port: InputPort<H>)
+    pub fn make_handoff<H>(&mut self) -> (InputPort<H>, OutputPort<H>)


WDYT about renaming this to make_edge? We had some conversations get mildly derailed regarding this, it's not incorrect, and might appeal to the user's intuition a little more?

I like it. I'm always down to rename things better, up until we stabilize (which is probably not soon)

benches/benches/reachability.rs

justinj · 2022-02-03T20:23:00Z

benches/benches/reachability.rs

@@ -130,67 +129,63 @@ fn benchmark_hydroflow_scheduled(c: &mut Criterion) {
            // A dataflow that represents graph reachability.
            let mut df = Hydroflow::new();

-            let reachable_out = df.add_source(move |_ctx, send: &SendCtx<VecHandoff<usize>>| {
+            type Hoff = VecHandoff<usize>;
+            let (reachable_out, merge_lhs) = df.make_handoff::<Hoff>();


maybe we should have an alias for make_handoff<VecHandoff<...>> given that (at least for now) it's almost always what we use?

justinj · 2022-02-03T21:03:37Z

hydroflow/src/scheduled/handoff/handoff_list.rs

+impl<T: BasePortList<false>> RecvPortList for T {}
+
+#[sealed]
+pub trait BasePortList<const S: bool>: TypeList {


is there a way we can have something other than a bool here? I don't really understand what it's doing, but could it be possible to use a trait instead? Is there a reason having a trait with a bool method marked #[inline] is less expressive than a bool?

Basically we have InputPort and OutputPort which in terms of implementation are pretty much identically, but I want to treat them as different types since they're not interchangeably semantically. So here we just use one struct to represent both with a bool to differentiate (true = send, false = recv).

But I want to change this to not be a bool, and be a different tag struct instead, because the bool isn't descriptive

justinj · 2022-02-03T21:39:51Z

hydroflow/src/scheduled/mod.rs

 pub mod graph;
-pub mod graph_demux;
+pub mod port;
+// pub mod graph_demux;


stray? either delete or add a comment explaining when this could be uncommented imo

Ill add comment

Now make handoffs before subgraphs (instead of the other way around).

…nference of hf.add_subgraph()

TODO(mingwei): fix trivial subgraph on L110, needs network/input methods to take in ports instead of return them.

MingweiSamuel · 2022-02-04T17:08:35Z

Basically let (a, b) = df.add_in_out(...) becomes df.add_subgraph_in_out(a, b, ...) and df.add_edge(x, y) at the end becomes let (x, y) = df.make_handoff::<...>() in the beginning

MingweiSamuel force-pushed the api-swap branch 17 times, most recently from edf5c3d to f6b5174 Compare February 3, 2022 19:23

MingweiSamuel force-pushed the api-swap branch from f6b5174 to e2731db Compare February 3, 2022 19:53

MingweiSamuel marked this pull request as ready for review February 3, 2022 19:55

MingweiSamuel requested a review from justinj February 3, 2022 19:57

MingweiSamuel commented Feb 3, 2022

View reviewed changes

hydroflow/tests/mux_demux.rs Outdated Show resolved Hide resolved

justinj approved these changes Feb 3, 2022

View reviewed changes

MingweiSamuel force-pushed the api-swap branch 2 times, most recently from fa55982 to 499626b Compare February 4, 2022 00:14

MingweiSamuel added 3 commits February 3, 2022 16:21

Refactor hydroflow graph API

73aa231

Now make handoffs before subgraphs (instead of the other way around).

Reuse Send/RecvCtx, Input/OutputPort via bool generic, use for type i…

d4a4c74

…nference of hf.add_subgraph()

Gut HandoffList, not used directly anymore

2a47b13

MingweiSamuel force-pushed the api-swap branch from 499626b to 6c1df00 Compare February 4, 2022 00:25

MingweiSamuel added 24 commits February 3, 2022 16:27

Make GraphExt consistent with new API, use macros to generate methods

6ff9007

Fix SubgraphData succs and preds initialization

385cf45

Update some tests in test.rs

71be907

Re-add add_(channel_)input methods

ef490bf

Update rest of test.rs

9c3797c

Update relalg

1186089

Update covid_tracing to new core API

fccb2d4

Update covid_tracing_dist to new core API

fcac952

Include Context for add_subgraph_homogeneous

a1065c5

Update Surface API to new core API (1/3)

0fac7db

Update Surface API to new core API (2/3)

04c459e

Update Surface API to new core API (3/3)

7799cb4

Update reachability bench for new core API

5fb5f30

Update identity bench for new core API

d657d69

Update fork_join bench for new core API

3449352

Comment out query API benchmark

bfaa520

Fix minor formatting

cb574ef

Fix/uncomment query API tee(), uncomment tee benchmarks

ab273c0

Rename add_subgraph_homogeneous to add_subgraph_n_m

b16580f

Update networked.rs test for new core API

0dba465

TODO(mingwei): fix trivial subgraph on L110, needs network/input methods to take in ports instead of return them.

Update surface API module docs for new core API, removal of connectors

2f9c723

Remove graph_demux code, tests

b27c79c

Rename make_handoff to make_edge

c5f726c

Remove unused imports

4c69d29

MingweiSamuel force-pushed the api-swap branch from 6c1df00 to 4c69d29 Compare February 4, 2022 00:27

MingweiSamuel merged commit 7bd4f89 into hydro-project:main Feb 4, 2022

MingweiSamuel deleted the api-swap branch February 4, 2022 00:31

MingweiSamuel mentioned this pull request Feb 7, 2022

Nice way to handle input/output ports we don't care about #36

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the hydroflow graph API to simplify the surface API #58

Refactor the hydroflow graph API to simplify the surface API #58

MingweiSamuel commented Jan 28, 2022 •

edited

Loading

MingweiSamuel commented Feb 3, 2022

MingweiSamuel commented Feb 3, 2022 •

edited

Loading

MingweiSamuel Feb 3, 2022

justinj left a comment

justinj Feb 3, 2022

MingweiSamuel Feb 3, 2022

justinj Feb 3, 2022

justinj Feb 3, 2022

MingweiSamuel Feb 4, 2022

justinj Feb 3, 2022

MingweiSamuel Feb 4, 2022

MingweiSamuel commented Feb 4, 2022

Refactor the hydroflow graph API to simplify the surface API #58

Refactor the hydroflow graph API to simplify the surface API #58

Conversation

MingweiSamuel commented Jan 28, 2022 • edited Loading

Example

MingweiSamuel commented Feb 3, 2022

Changes

Surface API Connector layer removed

Surface API handoff simplified

GraphExt functions are now created by a macro and are renamed to be more consistent:

Demux API is commented out for now, should be replaced with a special kind of handoff

HandoffList is not used anymore, instead there are PortList and PortListSplit

Input/OutputPorts and Send/RecvCtxs are simplified

add_subgraph_n_in_m_out renamed to add_subgraph_n_m

A: Extend<B>, <A as Extend<B>>::Extended: Spit<A, Suffix = B> trait bounds copied throughout

Improved teeing in query API

MingweiSamuel commented Feb 3, 2022 • edited Loading

TODOs in the future

Choose a reason for hiding this comment

justinj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MingweiSamuel commented Feb 4, 2022

MingweiSamuel commented Jan 28, 2022 •

edited

Loading

Surface API `Connector` layer removed

HandoffList is not used anymore, instead there are `PortList` and `PortListSplit`

`Input`/`OutputPort`s and `Send`/`RecvCtx`s are simplified

`add_subgraph_n_in_m_out` renamed to `add_subgraph_n_m`

`A: Extend<B>, <A as Extend<B>>::Extended: Spit<A, Suffix = B>` trait bounds copied throughout

MingweiSamuel commented Feb 3, 2022 •

edited

Loading