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

Add concurrent mode: any success #67

Closed

Conversation

cBournhonesque
Copy link
Contributor

@cBournhonesque cBournhonesque commented Jan 6, 2023

Hi,
I wanted to have an Action that executes multiple actions concurrently, and succeeds when any of them works.

I tested it locally, works well :)

Added an example as well.

@@ -323,6 +323,7 @@ pub fn steps_system(
*/
#[derive(Debug)]
pub struct ConcurrentlyBuilder {
mode: ConcurrentMode,
Copy link
Owner

Choose a reason for hiding this comment

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

I think my preference here would be to add a new Action instead, called Race, which is what this kind of operation is usually called, ime, as opposed to parameterizing Concurrently. Although if we're gonna go that way, maybe Concurrently should be renamed to Join, too...

Copy link
Contributor Author

@cBournhonesque cBournhonesque Jan 6, 2023

Choose a reason for hiding this comment

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

ah interesting. Where do you get those names from? Do you have a resource about utilityAI?

I wanted to re-use Concurrently because there's a lot of common code, but i can also split them up, and try to share code between the 2 actions, with a ConcurrentAction trait

Copy link
Owner

@zkat zkat Jan 6, 2023

Choose a reason for hiding this comment

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

this naming is more from the world of threading/async :)

I like splitting them up even with that duplication because it's nicer to have individual but very simple building blocks, rather than increasingly parameterizing the same thing to tbh completely change their semantics.

Maybe some of the common logic can be moved to utility functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I split up between Race and Join, removed Concurrently from the prelude, and added a cool little example that show clearly how Race and Join work :)

@cBournhonesque cBournhonesque requested a review from zkat January 7, 2023 19:15
name = "thirst"

[[example]]
name = "concurrent"
Copy link
Owner

Choose a reason for hiding this comment

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

why are we listing examples explicitly?

Copy link
Contributor Author

@cBournhonesque cBournhonesque Jan 9, 2023

Choose a reason for hiding this comment

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

Not 100% sure if it's required, but it was to make sure that the examples use dev-dependencies. (https://doc.rust-lang.org/rust-by-example/testing/dev_dependencies.html)
In particular my concurrent example needs rand

@zkat
Copy link
Owner

zkat commented Jan 30, 2023

Closing in favor of #68

@zkat zkat closed this Jan 30, 2023
@cBournhonesque
Copy link
Contributor Author

Hi, you didn't want to include the example? I thought it could be helpful to illustrate the types of concurrency.
Is there any part I could change?

@zkat
Copy link
Owner

zkat commented Jan 30, 2023

oh!!! I totally forgot to move that over too. Sorry, yes. The example would be good (although since I didn't keep Join/Race (sorry, I know), that'll need to be adapted. Do you want to PR it or would you rather I do it?

@cBournhonesque
Copy link
Contributor Author

I'll let you do the adaptation if that's ok?

@zkat
Copy link
Owner

zkat commented Jan 31, 2023

yeah that's totally fine. I'll take care of it. :)

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.

3 participants