-
-
Notifications
You must be signed in to change notification settings - Fork 360
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 workers too from the clean command #3579
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
af3816e
to
1b9f647
Compare
This comment was marked as resolved.
This comment was marked as resolved.
kicked it |
@@ -21,7 +21,14 @@ trait Evaluator { | |||
def outPath: os.Path | |||
def externalOutPath: os.Path | |||
def pathsResolver: EvaluatorPathsResolver | |||
// TODO In 0.13.0, workerCache should have the type of mutableWorkerCache, | |||
// while the latter should be removed | |||
def workerCache: collection.Map[Segments, (Int, Val)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Althouh this must stay for compat, we already can deprecate it in favor of its replacement. We try to deprecate API we intend to remove, to provide a smoother migration path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was imagining a simple change to def workerCache: collection.mutable.Map[Segments, (Int, Val)]
in 0.13
, and a removal of mutableWorkerCache
, that should only be used internally. It's a really small change.
Are there know users of the Evaluator
trait, that don't rely on the Mill internal implementation? Implementing that seems more of an internal thing to me… (and for users of Evaluator
, given that collection.mutable.Map <: collection.Map
, the change should be source-compatible).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now the policy is that the things that are exposed we treat as stable for bincompat. We don't actually know if anyone uses it, but if we expect something to be internal-only we'll mark them as private[mill]
and MIMA will let us do as we like
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current state of the PR doesn't break bin compat. But the question is, should this (minor and mostly source-compatible) change go through a deprecation cycle, or should we do it in one go in 0.13
? A change in one go, for this particular minor change, looks fine to me.
f02b53a
to
1522e43
Compare
1522e43
to
b6e896e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last things before merging, apart from those already mentioned in #3580:
-
Can we make
rm -rf
clean the workers as well? If we leave amodule-name/worker-name.json
file on disk as I suggested in the other PR, we could compare it to what's in memory, and anrm -r out/module-name
would invalidate the worker on next run even without amill clean
. Although not strictly necessary since people "should" be usingclean
instead ofrm -rf
, keeping both approaches working would help maintain the symmetry between theout
folder and the module/task hierarchy -
Update PR description
b6e896e
to
d155578
Compare
This PR ensures the
clean
command works for worker tasks, and allows users to dispose of workers by removing their corresponding metadata file on disk (out/foo/theWorker.json
for workerfoo.theWorker
) - in that case, the worker instance is dropped upon first access to it.Fixes #3276