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

Show workers in resolve / show / inspect too #3580

Merged
merged 3 commits into from
Sep 26, 2024

Conversation

alexarchambault
Copy link
Contributor

@alexarchambault alexarchambault commented Sep 18, 2024

This PR generalizes some Target-related stuff in Resolve to all NamedTasks with no arguments (targets and workers), and makes sure resolve / show / inspect work fine with workers. In particular, running show on a worker now prints something like

{
  "worker": "foo.theWorker",
  "toString": "build_.package_$foo$$anon$1@4acbdb44",
  "inputsHash": -2075822978
}

@alexarchambault

This comment was marked as outdated.

@alexarchambault alexarchambault changed the title Check Show workers in resolve / show / inspect too Sep 23, 2024
@alexarchambault alexarchambault marked this pull request as ready for review September 23, 2024 08:59
@lefou
Copy link
Member

lefou commented Sep 23, 2024

What's the motivation for this change? Workers aren't of any direct use from the cli. OTOH, to clean them up, they need to be discoverable. Is it that?

@alexarchambault
Copy link
Contributor Author

Yes, that's for #3579

@alexarchambault
Copy link
Contributor Author

I still find it handy to be able to create them from the command-line. I remember having been surprised that wasn't possible when I was discovering Mill, a few years ago.

@lefou
Copy link
Member

lefou commented Sep 23, 2024

I still find it handy to be able to create them from the command-line. I remember having been surprised that wasn't possible when I was discovering Mill, a few years ago.

I was wondering why you would want this, but that's mostly from my knowledge about existing worker implemntations. With the option to start and stop workers from the CLI, it's also possible for users to repurpose workers for other jobs in their build, like dev servers, which we would otherwise use runBackground for. I have to figure out, whether this is something we want to recommend or not.

@alexarchambault alexarchambault force-pushed the resolve-worker-check branch 3 times, most recently from 5e478c2 to bb851e3 Compare September 23, 2024 18:49
@lihaoyi
Copy link
Member

lihaoyi commented Sep 24, 2024

I'm fine with this change in principle, as making resolve/show/inspect more powerful seems like a good thing in general. e.g. #3532 is working to make those tasks work on Modules, which although they are different from tasks and are not serializable at all, would be very useful for a user trying to poke at a build from the command line.

For Workers, some things could work, some things can't. Let's try and enumerate them:

  • inspect could show the file name, line number, and Scaladoc same as other tasks
  • show cannot show JSON since workers are not JSON serializable. Maybe we can just show some stub JSON string like "path.to.worker:${worker.toString}"? Or we could error out and say "cannot show value of worker"
  • resolve listing workers seems fine
  • Running workers from the command line seems like it should work. Notably it would not result in a long lived object with --no-server, whereas runBackground would, so it's unclear how useful it would be.

I think for the purposes of this PR I'm happy to move forward, but we should flesh out the integration tests a bit both for correctness as well as clarity in what we expect the behavior to be, and where the limits are

  1. We can add a worker to integration/feature/docannotations to exercise inspect and make sure that works
  2. We can add an integration test somewhere for resolve to make sure that works
  3. Running workers from the CLI and using show seems like something we can skip for now with an error

@alexarchambault
Copy link
Contributor Author

There are already tests for resolve and inspect (and show too), but unit ones, in main/test/src/mill/main/MainModuleTests.scala.

@alexarchambault
Copy link
Contributor Author

alexarchambault commented Sep 24, 2024

  • Running workers from the command line seems like it should work. Notably it would not result in a long lived object with --no-server, whereas runBackground would, so it's unclear how useful it would be.

I guess that's fine. That's the behavior I'd expect as a user. Running it with --no-server basically checks that the worker can be instantiated fine.

@alexarchambault
Copy link
Contributor Author

alexarchambault commented Sep 24, 2024

  • show cannot show JSON since workers are not JSON serializable. Maybe we can just show some stub JSON string like "path.to.worker:${worker.toString}"? Or we could error out and say "cannot show value of worker"

In the current state of this PR, we get an empty JSON object, but showing a string containing worker.toString somewhere could be nice

Copy link
Member

@lihaoyi lihaoyi left a comment

Choose a reason for hiding this comment

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

I think this looks great. Three things before merging:

  1. Let's flesh out the show output so it's a bit more informative than {}; once we start showing workers in resolve, people will start trying to poke at them via inspect and show. inspect already works as your tests demonstrate, but show is also a common thing to use so we should make it informative. How about something like this
{
  "worker": "<worker-selector>",
  "toString": "<worker.toString>",
  "inputHash": 123456789
}
  1. Should we leave a <worker-name>.json file on disk containing the snippet above? That would make the show output and the "dig through files on disk" output more consistent, so anyone digging through the build using cat or less instead of mill show would be able to inspect workers as well

  2. Fill out the PR description summarizing the current state of the PR

@alexarchambault
Copy link
Contributor Author

alexarchambault commented Sep 25, 2024

@lihaoyi I think your 3 points should be addressed

@lihaoyi lihaoyi merged commit 69243dc into com-lihaoyi:main Sep 26, 2024
24 checks passed
@alexarchambault alexarchambault deleted the resolve-worker-check branch September 26, 2024 08:27
@lefou lefou added this to the 0.12.0-RC3 milestone Sep 26, 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
Development

Successfully merging this pull request may close these issues.

3 participants