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

Re-support parallel evaluation of TestModule.test command #3566

Closed
lihaoyi opened this issue Sep 17, 2024 · 4 comments · Fixed by #3617
Closed

Re-support parallel evaluation of TestModule.test command #3566

lihaoyi opened this issue Sep 17, 2024 · 4 comments · Fixed by #3617
Milestone

Comments

@lihaoyi
Copy link
Member

lihaoyi commented Sep 17, 2024

Otherwise we cannot set def defaultCommandName to testCached, because people who run __.test will pick up both the foo.test.testUncached default command name and also the foo.test.test test command.

TBH not quite sure what the right thing to do here is

This is a binary incompatible change and will need to go into 0.13.0

@lihaoyi lihaoyi added this to the 0.13.0 milestone Sep 17, 2024
@lolgab
Copy link
Member

lolgab commented Sep 17, 2024

The biggest problem with this change is that now to pass arguments to tests you need to add a .testUncached at the end and know the difference between testCached and testUncached.
Since tests are very often not pure (flaky, etc.) I think they should not be cached by default.
If I know the whole story, the problem with the current test command is that commands are now run sequentially.
Have you thought about adding some flag to commands to make them parallel?
It could be an annotation, or a special T.parallelCommand method.

@lefou
Copy link
Member

lefou commented Sep 17, 2024

@lolgab
I don't think we should explicitly request parallelism via the T API. Instead, we should demarcate commands that need serial or interactive execution if we think this is required.

The run commands last and sequentially should be configurable via a CLI option or some other settings. It's already configurable in the evaluator.

@lihaoyi
Copy link
Member Author

lihaoyi commented Sep 17, 2024

@lolgab you are right that making testCached the default causes problems passing arguments to the tests, which is very common. So that approach may be a non starter

For demarcating commands, something like T.parallelCommand seems reasonable. Some commands like clean should always run sequentially, while others like .test should be safe to run in parallel. So controlling it via a global flag doesnt really work.

Bazel has flags like exclusive = true that you can use to annotate actions

@lihaoyi lihaoyi changed the title Rename def test to def testUncached Rename def test to def testUncached to allow __.test to be parallel? Sep 18, 2024
lihaoyi added a commit that referenced this issue Sep 30, 2024
…nt` to `Task(persistent = true)` (#3617)

fixes #3566. Mostly a
straightforward implementation of what was discussed.

We use `Task.Command(exclusive = true)` to make
`console`/`repl`/`clean`/etc. run serially, all other commands should
run in parallel. That includes most `test` commands. The name
`exclusive` is taken from the Bazel action/target tag with the same
meaning: that the tagged action runs alone with no other actions in
parallel

`Task.Persistent` was changed to `Task(persistent = true)` for
consistency, and also for other reasons: the new syntax is more
composable, e.g. a user can more easily choose whether they want
`persistent = true` or `persistent = false` based on a computed
`Boolean` value, and Mill can in future extend it such that we can have
`Task(exclusive = true, persistent = true)`, `Task.Command(exclusive =
true, persistent = true)`, or other such combinations perhaps with even
more flags (e.g. Bazel has a pretty long list
https://bazel.build/reference/be/common-definitions#common.tags).

To make overload resolution work correctly, I make the first parameter
of the `(exclusive = true)` or `(persistent = true)` parameter list
`dummy: NamedParameterOnlyDummy.type = NamedParameterOnlyDummy`. This
ensures that there is no normal value the user could pass in
positionally that would select that overload of `def Command` or `def
apply`, and the only way to select it is by passing in `exclusive =
true` or `persistent = true` as a named parameter

Tested manually by adding `println`s and making sure that
`leafSerialCommands` no longer contains test tasks when I run tests in
`example/scalalib/basic/1-simple`.
@lefou lefou modified the milestones: 0.13.0, 0.12.0-RC3 Sep 30, 2024
@lefou
Copy link
Member

lefou commented Sep 30, 2024

We closed this, without renaming test to testUncached, since with #3617 we essentially run the test command in parallel to other targets now.

@lefou lefou changed the title Rename def test to def testUncached to allow __.test to be parallel? Re-support parallel evaluation of TestModule.test command Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants