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

[RFC] concurrent progn action #6937

Closed
Alizter opened this issue Jan 26, 2023 · 4 comments · Fixed by #6933
Closed

[RFC] concurrent progn action #6937

Alizter opened this issue Jan 26, 2023 · 4 comments · Fixed by #6933
Labels
proposal RFC's that are awaiting discussion to be accepted or rejected

Comments

@Alizter
Copy link
Collaborator

Alizter commented Jan 26, 2023

Desired Behavior

There are various situations in Dune in which we would like to run an action like (progn ) but the sequentiality ends up giving a sub-par user experience. Here are a few examples that I have in mind.

  1. Running multiple unrelated programs. If you have a user action where you wish to run multiple programs and one of the programs fail, then they all collectively fail. In this situation, you will only see the first error message that is encountered, even if actions afterwards would succeed.

  2. Running inline tests. In Run inline tests in parallel #1516 it is noted that errors occurring during the running of inline tests will run into the same issue as above, due to the use of (progn ). This also means that only one error at a time can get registered for promotion.

  3. Diffing multiple files. There are situations where you wish to diff multiple files, but only the diffs that get run get registered for promotion. This means for the reasons above. early errors will always cause the promotion of all the diffs to be delayed.

In all of these cases, the use of (progn ) could be replaced by a new action for concurrently running a list of actions. Let us call such an action (concurrent ).

I have designed and partially specified such an action in #6933 and am open to comments about it's implementation. Since I have to add to the build system to do this, it is good to discuss the idea here first.

Another application I have in mind is that diffing multiple files like this will make directory diffing, as we wanted to do for #6648, easier to implement.

There is the question of what to name this action. I have gone for (concurrent ) in #6933.

cc @snowleopard

@Alizter Alizter added the proposal RFC's that are awaiting discussion to be accepted or rejected label Jan 26, 2023
@Alizter
Copy link
Collaborator Author

Alizter commented Jan 26, 2023

Here is an example of the diffing bug that I mentioned:

It can be fixed easily by using (concurrent) inside the inline_tests.ml action.

@snowleopard
Copy link
Collaborator

snowleopard commented Jan 27, 2023

I think it's worth clearly separating two concerns:

  • Stop-at-first-error vs collect-all-errors behaviours, which has nothing to do with concurrency or parallelism. This could be just an extra argument passed to progn.

  • Running actions in sequence or in parallel, which is orthogonal to error-handling.

I think this is what confused me in the PR #6933, where words like "concurrent" and "parallel" made me worry about the -j flag.

@rgrinberg
Copy link
Member

rgrinberg commented Jan 27, 2023

Yes, that makes sense. Although I think that "stop-at-first-error" doesn't make sense for concurrent execution. The "first error" for concurrent execution is not deterministic so we should collect all errors.

Running actions sequentially but collecting all the errors is a valid use case, but I don't have much use for it at the moment. All the use cases I have in mind would benefit from the concurrency as well.

@snowleopard
Copy link
Collaborator

snowleopard commented Jan 27, 2023

Although I think that "stop-at-first-error" doesn't make sense for concurrent execution.

I think it does, although I'm not arguing it's very useful. I can imagine wanting to run a bunch of slow tests in parallel and fail immediately if any of them fails.

@Alizter Alizter linked a pull request Feb 5, 2023 that will close this issue
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal RFC's that are awaiting discussion to be accepted or rejected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants