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

Clean up RunScript helpers and Evaluator code #4094

Open
lihaoyi opened this issue Dec 9, 2024 · 5 comments
Open

Clean up RunScript helpers and Evaluator code #4094

lihaoyi opened this issue Dec 9, 2024 · 5 comments
Milestone

Comments

@lihaoyi
Copy link
Member

lihaoyi commented Dec 9, 2024

Not sure exactly how, but they definitely feel a bit out of place, as a bunch of static helpers wrapping Evaluator

Noticed when working on #4091

Would need to be done in 0.13.0 when we can break bincompat

@lihaoyi lihaoyi added this to the 0.13.0 milestone Dec 9, 2024
@lihaoyi lihaoyi changed the title Clean up RunScript helpers Clean up RunScript helpers and Evaluator code Dec 10, 2024
@lihaoyi
Copy link
Member Author

lihaoyi commented Dec 16, 2024

#4144 helps a bit, but more to be done. In particular we can get rid of the Terminal.Task case class and branches since outside of test suites nobody should be running anonymous tasks directly

@lefou
Copy link
Member

lefou commented Jan 26, 2025

In particular we can get rid of the Terminal.Task case class and branches since outside of test suites nobody should be running anonymous tasks directly

What about evaluator commands? We heavily use anonymous tasks in GenIdeaImpl for example.

@lihaoyi
Copy link
Member Author

lihaoyi commented Feb 5, 2025

Good question, we'll need to see if there's any alternatives that can be provided, if not we're stuck supporting them whether we like it or not

@lihaoyi
Copy link
Member Author

lihaoyi commented Feb 7, 2025

I think what needs to happen here is to split up the internal Evaluator component from the user-facing Evaluator component. The naming is a bit problematic with internal and external Evaluators, we probably need a new name for one of them. Maybe Executor or something?

  1. The internal Evaluator takes a Seq[Task_]] graph, evaluates it, and returns the resultant values
  2. The user-facing Executor should encapsulate all of the user facing APIs: Resolve, Plan, Evaluator, etc. some of which is currently in random RunScript helpers

The goal being that user-land tasks should not need to dig around the various packages in mill.{eval,resolve,main} to do stuff, and should be presented a single Executor object that exposes the APIs they need

lihaoyi added a commit that referenced this issue Feb 9, 2025
#4094

- Move `Evaluator` into a downstream module `core/` that depends on
`core/{define,resolve,eval}`
- Remove `RunScript` and inline its functionality into `Evaluator`
- Rename `eval/` into `exec/`, along with all internal classes
- Wrap `Resolve.{Task,Segments}` APIs as
`Evaluator.resolve{Tasks,Segments}`

Effectively this splits up:

* The user-facing portion of `Evaluator`, still called `Evaluator`
* The execute-task-graph part of `Evaluator`, now called
`exec`/`Execution`

This preserves the user-facing name `evaluator: Evaluator`, and renames
mostly internal classes to `Execution` to avoid churn in the user-facing
API. Now that `Evaluator` is its own thing, we can also move more APIs
into it (e.g. `Resolve.*`) without it getting too bloated. This also
exposes a consistently usable `Evaluator` object to user code, rather
than constantly needing to pass bits and pieces of it to static helpers
like passing `.rootModule` to `mill.resolve.Resolve`
@lihaoyi
Copy link
Member Author

lihaoyi commented Feb 14, 2025

Made a lot of progress here, but more to be done. All user-facing APIs have been consolidated into Evaluator, though the signatures themselves are still very messy e.g.

  def resolveEvaluate(
      scriptArgs: Seq[String],
      selectMode: SelectMode,
      selectiveExecution: Boolean = false
  ): Result[(Seq[Watchable], Result[Seq[(Any, Option[(Evaluator.TaskName, ujson.Value)])]])]

  def evaluate(
      targets: Seq[Task[Any]],
      selectiveExecution: Boolean = false
  ): (Seq[Watchable], Result[Seq[(Val, Option[(Evaluator.TaskName, ujson.Value)])]]) = {

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

No branches or pull requests

2 participants