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

Subscope rework #194

Merged
merged 7 commits into from Oct 10, 2018

Conversation

Projects
None yet
1 participant
@frankmcsherry
Owner

frankmcsherry commented Oct 6, 2018

This PR reworks subgraphs and subscopes a bit, with the aim of making things more flexible for subgraphs whose timestamps are not product-order structured. Potential examples include having the same timestamp as the containing scope (for creating regions for abstraction) and lexicographic-order structured extensions.

There is a new trait Refines<T> which is implementable by many timestamps, and which indicates how to convert into and from the refining timestamp, and how the refined path summaries can be abstracted.

    pub trait Refines<T: Timestamp> : Timestamp {
        /// Converts the outer timestamp to an inner timestamp.
        fn to_inner(other: T) -> Self;
        /// Converts the inner timestamp to an outer timestamp.
        fn to_outer(self) -> T;
        /// Summarizes an inner path summary as an outer path summary.
        ///
        /// It is crucial for correctness that the result of this summarization's `results_in`
        /// method is equivalent to `|time| path.results_in(time.to_inner()).to_outer()`, or
        /// at least produces times less or equal to that result.
        fn summarize(path: <Self as Timestamp>::Summary) -> <T as Timestamp>::Summary;
    }

This trait is not symmetric with T and Self because information is lost when we go from Self to T, and so we cannot expect that a path summary on T can be faithfully restored to a path summary on Self.


On possibly delightful consequence is that RootTimestamp is pretty useless now. Everything would refine it, and no one really wants to use Product<RootTimestamp,T> when they could just use T, so I got rid of it.

There is a little bit of fall-out with type inference. For some reason (more generics, I guess) inputs don't always drive type inference as well as they used to, and in some examples I had to explicitly annotate e.g.

    worker.dataflow::<usize,_,_>( ... )

The reasons aren't especially clear, and they happen in some weird cases (e.g. not in examples/hello.rs, but yes in examples/distinct.rs apparently due to a hashmap).

Another bit of pedagogical fall out is that if you want to be able to have a feedback edge you still need to have Product<_,_> structured timestamps, because we don't trust you enough to supply a non-trivial summary to loop_variable. So if doing e.g. examples/pagerank.rs you need to use a timestamp type Product<(), usize> rather than just usize. Perhaps that should be thought through a bit better before this lands. We could open up loop variable and have you express a summary, check that it is not the default summary (zero action), and just go with that.


A related change is essentially removing the complicated PathSummary implementation for Product. There was a bunch of Outer and Local stuff, and .. unfortunately I think it only makes sense if the contained scope is product ordered. That is, if you are an iterative scope, you want your parent scope to have this sort of summary.

Fortunately, it turns out that the weird structure hasn't been needed for a while, because nested scopes need not track the flow of their own internal capabilities out and back around to their inputs (the parent scope learns about them, and then presents them back as its input capabilities).

@frankmcsherry frankmcsherry merged commit 3f6f84d into master Oct 10, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment