Skip to content
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

Allow dyn in StagesTuple, add Current Testcase API, Untraitify Progress #1915

Merged
merged 23 commits into from
Mar 11, 2024

Conversation

domenukk
Copy link
Member

@domenukk domenukk commented Mar 7, 2024

Implements proper restarters / progress for Mutational Stages
Also adds new entry API to AnyMap,
Adds .current_testcase and .current_input_cloned APIs
Adds or_insert_with to AnyMap, a fast way to insert or get
Changes execution counters from usize to u64 (for 32 bit systems)
and a lot more...

RFC: Rename ProgressHelpers to RestartHelper

@@ -327,6 +327,16 @@ where

Ok(())
}

fn initialize_progress(&mut self, state: &mut Self::State) -> Result<(), Error> {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addisoncrump technically we could just call this from the stage, not sure what's best

@@ -115,6 +113,18 @@ where

Ok(())
}

#[inline]
fn restart_progress_should_run(&mut self, _state: &mut Self::State) -> Result<bool, Error> {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't 100% like the name but it's the best I found

@@ -356,178 +375,3 @@ where
}
}
}

#[cfg(test)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this got deleted?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's complex and I don't understand what it was doing before my refactor

state.enter_inner_stage()?;
Ok(())
Ok(true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not always true? right?
i think it is depending the result of the inner stage

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inner stage will do its own handling, so it's fine to return true here I think

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should respect the result of the inner's restart_pregress_should_run right? @domenukk

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think so

Copy link
Member

@tokatoka tokatoka Mar 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm okay i understand.

but i noticed another problem.
These conditional stage evaluate the closure given by the user. but you could get stuck when evaluating them right? (even before entering the inner stage)
so how about also adding max # of retry to this helper too? then return true if we can continue retry, return false if we reached the cap

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ClosureStage uses the RetryHelper. For all user-provided stages, they'll have to implement their own restart handling

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think bailing after n retries will always be the best solution (I would even say it's always the worst)

@domenukk
Copy link
Member Author

Replied to the comments

@domenukk
Copy link
Member Author

No, I don't understand how to reply :D

@domenukk
Copy link
Member Author

The 'dyn doesn't help us too much here because the stages need to be 'static

@tokatoka
Copy link
Member

tokatoka commented Mar 11, 2024

I think it's confusing that Stage implements functions named restart_progress_should_run and helpers also implement restart_progress_should_runs.
Shouldn't we rename one of them?

like, the inner one could be named as should_run because it is the main decider on whether you should continue or not and returns boolean

@domenukk domenukk merged commit dd410c5 into main Mar 11, 2024
26 checks passed
@domenukk domenukk deleted the stages_tuple_vec branch March 11, 2024 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants