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

[WIP] Make Evaluator only available to Task.Command(exclusive = true) #3717

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lihaoyi
Copy link
Member

@lihaoyi lihaoyi commented Oct 11, 2024

This is a breaking change that tightens things up a bit following the introduction of Task.Command(exclusive = true): now that we run commands in parallel, running multiple evaluator commands in parallel is dangerous, so they should be limited to exclusive commands

It's another step toward #502, by declaring that exclusive commands are different from normal commands, and thus need special treatment

Despite being binary compatible, we might not want to land this in 0.12.0, because it would break basically all usages of Evaluator commands in third-party plugins and force them to be updated, which defeats the purpose of maintaining bincompat. Probably better to wait till 0.13.0 when third-party plugins need to be fixed/recompiled anyway, and we can change the API (e.g. move to Task.evaluator) to force people to update their commands rather than happily compiling and then failing later at runtime

Had to jump through some hoops to do this without breaking bincompat:

  • We inject the Evaluator parameter during the resolution phase, before evaluation starts,
  • At that time, we do not yet know whether a command is exclusive or not.
  • The solution is to instead pass a ProxyEvaluator that exposes all the same API, but looks up the current Evaluator.currentEvaluatorSafe on-demand
  • Evaluator.currentEvaluatorSafe is a version of Evaluator.currentEvaluator that only exists within exclusive commands, so we can fail with a nice error message if someone tries to use the evaluator inside a non-exclusive command
  • We cannot remove other usages of Evaluator.currentEvaluator,
    • e.g. in the mill.main.Tasks.TokensReader actually needs to resolve the tasks early on and return a Seq, which we cannot easily stub out.
    • But that's probably OK, because it doesn't expose the entire Evaluator, and the tasks themselves are immutable and probably safe to use concurrency

@lihaoyi
Copy link
Member Author

lihaoyi commented Oct 11, 2024

This might not be possible without breaking bincompat due to the fact that we inject the Evaluator parameter during the resolution phase, before evaluation starts, during which we do not yet know whether a command is exclusive or not.

@lihaoyi lihaoyi changed the title Make Evaluator only available to Task.Command(exclusive = true) [WIP] Make Evaluator only available to Task.Command(exclusive = true) Oct 11, 2024
@lihaoyi lihaoyi closed this Oct 11, 2024
@lihaoyi lihaoyi reopened this Oct 11, 2024
@lihaoyi lihaoyi added this to the 0.13.0 milestone Oct 11, 2024
@lefou
Copy link
Member

lefou commented Oct 12, 2024

This would defeat the initial use case, that let us to introduce the exclusive tag in the first place.

Dependency/showUpdates is an evaluator command, that is non-exclusive but profits heavily from being parallel-runnable.

@lihaoyi
Copy link
Member Author

lihaoyi commented Oct 14, 2024

@lefou If I'm not mistaken Dependency/showUpdates should be fine, since only the top-level mill.scalalib.Dependency/showUpdates command would need to be exclusive, and indeed only one showUpdates task is running at a time. Internally, showUpdates ends up calling a bunch of javaModule.* targets, and those both are neither exclusive nor are they evaluator commands. So I think it should continue to work unchange even if we land this limitation

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