feat(dfir_rs, hydro_lang): add resolve_futures and resolve_futures_ordered APIs #1741

RyanAlameddine · 2025-02-23T21:30:47Z

No description provided.

shadaj · 2025-02-23T22:00:15Z

hydro_lang/src/stream.rs

+    /// # use dfir_rs::futures::StreamExt;
+    /// # use hydro_lang::*;
+    /// # tokio_test::block_on(test_util::stream_transform_test(
+    ///     |process| {


Ah, like the other doctests, you should move this line above, and also # out the assertion and only display a comment instead.

shadaj

Couple tweaks needed to the docs, implementation looks great!

shadaj · 2025-02-23T23:32:48Z

hydro_lang/src/stream.rs

+    /// # use dfir_rs::futures::StreamExt;
+    /// # use hydro_lang::*;
+    /// # tokio_test::block_on(test_util::stream_transform_test(|process| {
+    ///         let tick = process.tick();


You'll want to dedent these (you can hover over the function name in VSCode to see the rendered docs.

shadaj · 2025-02-23T23:33:07Z

hydro_lang/src/stream.rs

+    ///                 x
+    ///             }))
+    ///             .poll_futures()
+    ///         // 1, 2, 3, 4, 5, 6, 7, 8, 9


Just to mirror the other docs, this should be next to the assertion.

shadaj · 2025-02-23T23:33:17Z

hydro_lang/src/stream.rs

+    /// #   |mut stream| async move {
+    /// #       assert_eq!(
+    /// #           HashSet::<i32>::from_iter(1..10),
+    /// #           HashSet::from_iter(vec![stream.next().await.unwrap(), stream.next().await.unwrap(), stream.next().await.unwrap(), stream.next().await.unwrap(), stream.next().await.unwrap(), stream.next().await.unwrap(), stream.next().await.unwrap(), stream.next().await.unwrap(), stream.next().await.unwrap()])


Can we use a for loop? :)

shadaj · 2025-02-23T23:33:27Z

hydro_lang/src/stream.rs

+    ///                 x
+    ///             }))
+    ///             .poll_futures_ordered()
+    ///             // 2, 3, 1, 9, 6, 5, 4, 7, 8


Same comments as above

shadaj · 2025-02-23T23:34:43Z

hydro_test_local/src/local/futures.rs

+    process
+        .source_iter(q!([2, 3, 1, 9, 6, 5, 4, 7, 8]))
+        .map(q!(|x| async move {
+            // tokio::time::sleep works, import then just sleep does not, unsure why


Very strange... this would indicate that something is wrong with the stageleft logic... But irrelevant to this PR so okay for now.

shadaj · 2025-02-24T18:21:12Z

Another open question; should we name this poll_futures or something like resolve_futures? It feels like "polling" is more of an implementation detail...

shadaj

Last set of changes!

shadaj · 2025-02-25T23:16:58Z

dfir_lang/CHANGELOG.md

@@ -356,7 +356,7 @@ Pipeline flags: Needs checking of bounded vs unbounded for batch()vs all_once()N

 ### Reverted

- - <csr-id-256779abece03bee662b351430d27141d10bd5ef/> "feat(hydroflow): Added poll_futures and poll_futures_async operators.", fix #1183
+ - <csr-id-256779abece03bee662b351430d27141d10bd5ef/> "feat(hydroflow): Added resolve_futures and resolve_futures_async operators.", fix #1183


Ah, looks like a buggy global find-and-replace, we should restore the original file.

shadaj · 2025-02-25T23:17:09Z

dfir_rs/CHANGELOG.md

@@ -413,7 +413,7 @@ Pipeline flags: Needs checking of bounded vs unbounded for batch()vs all_once()N

 ### Reverted

- - <csr-id-256779abece03bee662b351430d27141d10bd5ef/> "feat(hydroflow): Added poll_futures and poll_futures_async operators.", fix #1183
+ - <csr-id-256779abece03bee662b351430d27141d10bd5ef/> "feat(hydroflow): Added resolve_futures and resolve_futures_async operators.", fix #1183


shadaj · 2025-02-25T23:17:17Z

hydro_lang/CHANGELOG.md

@@ -1142,7 +1142,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

 ### Reverted

- - <csr-id-256779abece03bee662b351430d27141d10bd5ef/> "feat(hydroflow): Added poll_futures and poll_futures_async operators.", fix #1183
+ - <csr-id-256779abece03bee662b351430d27141d10bd5ef/> "feat(hydroflow): Added resolve_futures and resolve_futures_async operators.", fix #1183


shadaj · 2025-02-25T23:18:07Z

hydro_lang/src/stream.rs

+    ///     .resolve_futures()
+    /// #   },
+    /// #   |mut stream| async move {
+    /// // 1, 2, 3, 4, 5, 6, 7, 8, 9


Can we add a small (in arbitrary order) note and scramble the order just so developers don't make any assumptions about that?

shadaj · 2025-02-25T23:18:20Z

hydro_lang/src/stream.rs

+        )
+    }
+
+    // fn test() {


shadaj · 2025-02-25T23:19:00Z

hydro_lang/src/stream.rs

+    /// let tick = process.tick();
+    /// process.source_iter(q!([2, 3, 1, 9, 6, 5, 4, 7, 8]))
+    ///     .map(q!(|x| async move {
+    ///         // tokio::time::sleep works, import then just sleep does not, unsure why


Let's not put this inside the test. Also, the reason this happens is that for doctests specifically, we don't have infrastructure to extract the imports used outside the quoted code.

shadaj · 2025-02-25T23:19:09Z

hydro_lang/src/stream.rs

+    /// let tick = process.tick();
+    /// process.source_iter(q!([2, 3, 1, 9, 6, 5, 4, 7, 8]))
+    ///     .map(q!(|x| async move {
+    ///         // tokio::time::sleep works, import then just sleep does not, unsure why


Same here, let's drop this line.

RyanAlameddine requested a review from shadaj February 23, 2025 21:30

RyanAlameddine force-pushed the main branch 2 times, most recently from e64adca to d9bc803 Compare February 23, 2025 21:52

shadaj reviewed Feb 23, 2025

View reviewed changes

RyanAlameddine force-pushed the main branch 3 times, most recently from 7fa6e13 to 6f604f7 Compare February 23, 2025 22:27

feat(hydroflow): Re-introduced poll_futures.

0312da2

RyanAlameddine force-pushed the main branch from 6f604f7 to 0312da2 Compare February 23, 2025 23:12

shadaj requested changes Feb 23, 2025

View reviewed changes

shadaj changed the title ~~feat(hydroflow): Added poll_futures and poll_futures_ordered to hydroflow plus.~~ feat(dfir_rs,hydro_lang): add poll_futures and poll_futures_ordered APIs Feb 24, 2025

shadaj changed the title ~~feat(dfir_rs,hydro_lang): add poll_futures and poll_futures_ordered APIs~~ feat(dfir_rs, hydro_lang): add poll_futures and poll_futures_ordered APIs Feb 24, 2025

RyanAlameddine force-pushed the main branch from 07af650 to 62985b9 Compare February 25, 2025 00:03

feat(hydroflow): Re-introduced poll_futures as resolve_futures.

48f2be7

RyanAlameddine force-pushed the main branch from 62985b9 to 48f2be7 Compare February 25, 2025 00:18

shadaj changed the title ~~feat(dfir_rs, hydro_lang): add poll_futures and poll_futures_ordered APIs~~ feat(dfir_rs, hydro_lang): add resolve_futures and resolve_futures_ordered APIs Feb 25, 2025

shadaj requested changes Feb 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dfir_rs, hydro_lang): add resolve_futures and resolve_futures_ordered APIs #1741

feat(dfir_rs, hydro_lang): add resolve_futures and resolve_futures_ordered APIs #1741

RyanAlameddine commented Feb 23, 2025

shadaj Feb 23, 2025

shadaj left a comment

shadaj Feb 23, 2025

shadaj Feb 23, 2025

shadaj Feb 23, 2025

shadaj Feb 23, 2025

shadaj Feb 23, 2025

shadaj commented Feb 24, 2025

shadaj left a comment

shadaj Feb 25, 2025

shadaj Feb 25, 2025

shadaj Feb 25, 2025

shadaj Feb 25, 2025

shadaj Feb 25, 2025

shadaj Feb 25, 2025

shadaj Feb 25, 2025

feat(dfir_rs, hydro_lang): add resolve_futures and resolve_futures_ordered APIs #1741

Are you sure you want to change the base?

feat(dfir_rs, hydro_lang): add resolve_futures and resolve_futures_ordered APIs #1741

Conversation

RyanAlameddine commented Feb 23, 2025

Choose a reason for hiding this comment

shadaj 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

shadaj commented Feb 24, 2025

shadaj 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