-
Notifications
You must be signed in to change notification settings - Fork 12
feat: mutliple prologue steps #44
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
base: main
Are you sure you want to change the base?
Conversation
a00a951 to
bd5a3fe
Compare
| .prologue() | ||
| .get( | ||
| step_index | ||
| .checked_sub(PROLOGUE_INDEX) |
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.
so we remove 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.
This is basically useless as PROLOGUE_INDEX = usize::MIN but it makes the whole logic correct (else we should just remove PROLOGUE_INDEX altogether). It should be optimized away at compilation anyway.
| // breaking in prologue or epilogue directly stop pipeline execution | ||
| if self.is_epilogue() || self.is_prologue() { |
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.
We can discuss this as well. When we break within a prologue, should we break out of the pipeline, or just go to pipeline steps? I believe breaking out makes more sense
karim-agha
left a comment
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.
How about we reduce complexity here and instead of adding the ability to have multiple epilogue and prologue steps and then worry about all the implications of that, instead we create a new type of step that ships with rblib called say All that combine multiple steps into one?
So what your trying to achieve here would be represented as
pipeline
.with_step(Step1)
.with_prologue(All([Step2, Step3]))Wdyt?
We can then configure the break/continue behavior at this combinatorial step level on a case by case basis.
This will leave the current pipeline logic as is and move the behavior into combinators. We might end up with combinator functions such as:
|
I'm all for encapsulating complexity separately as pipeline is already quite complex. |
A pipeline may include nested pipelines with loops. A combinator step ensures that you have a flat list of steps that will eventually terminate. Another combinator step I can think of is |
To follow up on this. Should we close this PR and revert implementation for epilogue for consistency? |
I would prefer we get something merged for now (and also not revert the epilogue change) since flashtestations requires the additional prologue and flashblocks requires the additional epilogue |
Following #43
Should be enought to close #31 for now, even if it provides "partial" solution