-
Notifications
You must be signed in to change notification settings - Fork 432
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
[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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome. Not much to say but maybe check my comments in the test for my understanding
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ops -> operation
/// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
explorations
} | ||
|
||
/// The result of an exploration done by the [explorer](Explorer). | ||
pub enum Exploration<'a, O> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still mentions of optimizations in here, it's that wanted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
met
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to to
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: