[burn-fusion] save all execution plans for any trigger #1143
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1143 +/- ##
==========================================
+ Coverage 85.82% 85.88% +0.06%
==========================================
Files 516 517 +1
Lines 57215 57659 +444
==========================================
+ Hits 49105 49523 +418
- Misses 8110 8136 +26 ☔ View full report in Codecov by Sentry. |
| operations: &[OperationDescription], | ||
| mode: ExecutionMode, | ||
| ) -> Exploration<'a, O> { | ||
| // When we are executing with the new ops mode, we need to register the last ops of the |
| /// which isn't supposed to be big at any time. | ||
| pub(crate) struct Policy<O> { | ||
| // The potential optimizations that we could apply to the current stream, but their streams | ||
| // The potential explocations that we could apply to the current stream, but their streams |
| } | ||
|
|
||
| /// The result of an exploration done by the [explorer](Explorer). | ||
| pub enum Exploration<'a, O> { |
There was a problem hiding this comment.
There are still mentions of optimizations in here, it's that wanted?
There was a problem hiding this comment.
Yes, the explorer tries to find optimizations, and returns an exploration that may or may not contains an optimization.
| let plan_id_3 = 2; | ||
|
|
||
| // The first builder only contains 2 operations, and the optimization is always available when | ||
| // the pattern is meet. |
There was a problem hiding this comment.
meet -> met
also in next comment
| // Nothing to execute for the first operation. | ||
| stream.add(operation_1()); | ||
| stream.assert_number_of_operations(1); | ||
| stream.assert_number_of_executions(0); |
There was a problem hiding this comment.
builder_1 is still waiting to see next op is operation_2
builder_2 is closed because it's not the right operation
| stream.add(operation_1()); | ||
| stream.assert_number_of_operations(0); | ||
| stream.assert_number_of_executions(1); | ||
| stream.assert_last_executed(plan_id_1); |
There was a problem hiding this comment.
here, does builder_1 simply close because expecting operation_2 and getting operation_1? or does it start anew with the second operation_1?
Like if he expects [1,2], does [1,1,2] allow it to ignore the first one and be activated on the second and third?
There was a problem hiding this comment.
It closes, every builder HAVE to include ALL operations from the START in their optimization. But when one execution is done, then they reset and are open. So right after an execution, all builder are open.
operation 1 ( builder 1 opened, builder 2 closed)
operation 1 (builder 1 closed, builder 2 closed)
execution [RESET] (builder 1 opened, builder 2 opened)
| stream.assert_number_of_executions(2); | ||
|
|
||
| // Now we should trigger the second optimization builder. | ||
| stream.add(operation_1()); |
There was a problem hiding this comment.
Very nice how this one is both the trigger for builder_2 and the first op of builder_1
There was a problem hiding this comment.
Yup that was to test a more complex case :D
| stream.assert_number_of_operations(2); | ||
| stream.assert_number_of_executions(4); | ||
|
|
||
| // On sync we should execute all operations even if their trigger isn't meet. |
burn-fusion/src/stream/store/base.rs
Outdated
| #[allow(clippy::large_enum_variant)] | ||
| // Triggers are stored in a list, and you can have many `OnOperation` entries, | ||
| // but only one `OnSync` entry and one `Always` entry, therefore we don't care if it takes more | ||
| // space to to store them. |
This is another significant refactor of burn-fusion, marking the second one in the series 😅.
Finally, I have discovered a method to effectively write tests that validate the correct functionalities. While it is essentially one large test, it primarily aims to ensure that all cases can be queued sequentially, maintaining a consistently proper state—a genuine way to thoroughly test the code. It's a beginning, but testing is straightforward, given that most "algorithm" structures are generic over the optimization type, not the fusion backend type. Consequently, each component can be tested with ease. More tests will be added in the future, but at least there's a manageable starting point for now.
I have also opted to replace many abbreviations with their full names, as I believe it enhances overall readability.
It's not a "pure" refactor like the last PR #1135; instead, some bugs/edge cases have been addressed and simplified: