feat(hydroflow_plus)!: mark non-deterministic operators as unsafe and introduce timestamped streams #1584
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Deploying hydroflow with
|
| Latest commit: |
e5d39ac
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://66f5947c.hydroflow.pages.dev |
| Branch Preview URL: | https://pr1584.hydroflow.pages.dev |
This was referenced Nov 21, 2024
shadaj
force-pushed
the
pr1584
branch
10 times, most recently
from
92a7be4
to
7974894
Compare
shadaj
force-pushed
the
pr1584
branch
2 times, most recently
from
da4c9b5
to
edb2843
Compare
shadaj
changed the title
shadaj
force-pushed
the
pr1584
branch
2 times, most recently
from
323b4cd
to
95e6ac1
Compare
shadaj
force-pushed
the
pr1584
branch
2 times, most recently
from
fdb1430
to
843df89
Compare
… introduce timestamped streams Big PR. First big change is we introduce a `Timestamped` location. This is a bit of a hybrid between top-level locations and `Tick` locations. The idea is that you choose where timestamps are generated, and then have a guarantee that everything after that will be atomically computed (useful for making sure we add payloads to the log before ack-ing). The contract is that an operator or module that takes a `Timestamped` input must still be deterministic regardless of the stamps on messages (which are hidden unless you `tick_batch`). But unlike a top-level stream (which has the same constraints), you have the atomicity guarantee. Right now the guarantee is trivial since we have one global tick for everything. But in the future when we want to apply @davidchuyaya's optimizations this will be helpful to know when there are causal dependencies on when data can be sent to others. Second change is we mark every non-deterministic operator (modulo explicit annotations such as `NoOrder`) with Rust's `unsafe` keyword. This makes it super clear where non-determinism is taking place. I've used this to put `unsafe` blocks throughout our example code and add `SAFETY` annotations that argue why the non-determinism is safe (or point out that we've explicitly documented / expect non-determinism). I also added `#![warn(unsafe_op_in_unsafe_fn)]` to the examples and the template, since this forces good hygiene of annotating sources of non-determinism even inside a module that is intentionally non-deterministic. Paxos changes are mostly refactors, and I verified that the performance is the same as before.
|
Yeah, went back and forth a bunch on using |
MingweiSamuel
approved these changes
Nov 27, 2024
Comment on lines
+99
to
+110
| let out = self | ||
| .l | ||
| .spin() | ||
| .flat_map(q!(move |_| 0..batch_size)) | ||
| .flat_map_ordered(q!(move |_| 0..batch_size)) | ||
| .map(q!(|_| ())) | ||
| .tick_batch(self) | ||
| .timestamped(self); | ||
|
|
||
| unsafe { | ||
| // SAFETY: at runtime, `spin` produces a single value per tick, | ||
| // so each batch is guaranteed to be the same size. | ||
| out.tick_batch() | ||
| } |
There was a problem hiding this comment.
For now can just leave the rest of the unsafe blocks as is, but would be good as a convention to only wrap the unsafe method calls in unsafe, otherwise the whole call chain goes into unsafe and it doesn't distinguish anything (have to check each method called/memorize to see if that is the unsafe one)
There was a problem hiding this comment.
Yeah, I think I will try to write up a style guide soon on what to wrap in unsafe. Sometimes it's nice to include additional API calls if it is a "temporary"/"local" unsafety rather than one which propagates out the function call.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Big PR.
First big change is we introduce a
Timestampedlocation. This is a bit of a hybrid between top-level locations andTicklocations. The idea is that you choose where timestamps are generated, and then have a guarantee that everything after that will be atomically computed (useful for making sure we add payloads to the log before ack-ing).The contract is that an operator or module that takes a
Timestampedinput must still be deterministic regardless of the stamps on messages (which are hidden unless youtick_batch). But unlike a top-level stream (which has the same constraints), you have the atomicity guarantee. Right now the guarantee is trivial since we have one global tick for everything. But in the future when we want to apply @davidchuyaya's optimizations this will be helpful to know when there are causal dependencies on when data can be sent to others.Second change is we mark every non-deterministic operator (modulo explicit annotations such as
NoOrder) with Rust'sunsafekeyword. This makes it super clear where non-determinism is taking place.I've used this to put
unsafeblocks throughout our example code and addSAFETYannotations that argue why the non-determinism is safe (or point out that we've explicitly documented / expect non-determinism). I also added#![warn(unsafe_op_in_unsafe_fn)]to the examples and the template, since this forces good hygiene of annotating sources of non-determinism even inside a module that is intentionally non-deterministic.Paxos changes are mostly refactors, and I verified that the performance is the same as before.