https://pantsbuild.org/ logo
#development
Title
# development
h

hundreds-breakfast-49010

08/19/2020, 10:58 PM
so re: https://github.com/pantsbuild/pants/issues/10650, what behavior do we want to happen when a started workunit is invalidated?
I thought about introducing a new
WorkunitState::Canceled
, which would percolate into the
StreamingWorkunitHandler
code
@polite-garden-50641 @witty-crayon-22786
w

witty-crayon-22786

08/19/2020, 11:01 PM
good question.
generally “Canceled” is a form of “failure”
Completed/Started/Canceled might make sense. but Started/Succeeded/Failed, with cancellation being a particular message in Failed might also make sense.
👍 1
but it’s a better question for Asher and @fast-nail-55400 i think.
Asher as a consumer, and Tom as a producer of workunits
h

hundreds-breakfast-49010

08/19/2020, 11:04 PM
another question is, do we want to do anything with the log level when a node cancelation happens?
I noticed while testing my change that the workunits that get canceled are all Debug level, so the user won't normally see the message associated with their cancelation
we might want to bump the level to WARN
a node cancelation should be an exceptional state but not a failure state?
f

fast-nail-55400

08/19/2020, 11:18 PM
there are a few uses for workunits, right? tracing is one, and tracking and reporting results is another.
does cancelation remove a workunit from the graph?
h

hundreds-breakfast-49010

08/19/2020, 11:19 PM
@fast-nail-55400 yes, workunits are responsible for logging the "Started"/"Completed" messages in the console when pants runs, and are passed to plugins, including Toolchain's
no, the issue right now is that a
Started
entry for a workunit will be emitted whenever a workunit starts
but there won't be a corresponding
Completed
one
f

fast-nail-55400

08/19/2020, 11:20 PM
that would confuse me to not see some end to the story told by “Started”
better to just show me what work was done
h

hundreds-breakfast-49010

08/19/2020, 11:20 PM
so this means the workunit json handled by a plugin needs to handle the case where a completed workunit corresponding to a started one just never shows up, and also we'll log multiple "Started" lines without a "Completed" line
f

fast-nail-55400

08/19/2020, 11:21 PM
and the dynamic UI can show what is actively being worked on in the moment
h

hundreds-breakfast-49010

08/19/2020, 11:21 PM
I think it makes sense to log a "Canceled" message to the console, which is what my PR currently does
f

fast-nail-55400

08/19/2020, 11:22 PM
but what is doing the cancelation? speculation? or user ctrl-c’ing?
h

hundreds-breakfast-49010

08/19/2020, 11:23 PM
should be anything that causes the future associated with a node to not complete, which includes invalidating a node because the underlying files changed
f

fast-nail-55400

08/19/2020, 11:25 PM
using file changes as a specific example, telling me that work is canceled doesn’t add useful information . just say “files changed. recomputing graph” would be sufficient (if Pants were continually watching a tree). The same principle can be extended to other examples, does informing the user of cancelation add any new and useful info?
if ctrl-C is pressed, just let me know that everything is canceled in one line, don’t tell me about every single work item that Pants would have done. I as the user don’t care. I pressed Ctrl-C and that is that.
h

hundreds-breakfast-49010

08/19/2020, 11:48 PM
@fast-nail-55400 hm that's a good point, we might want to deliberately not log node cancellations due to ctrl-c
so those should see cancellation, and they are mostly Info
if they’re not seeing cancellation… then… that would be my bug.
h

hundreds-breakfast-49010

08/20/2020, 12:04 AM
@witty-crayon-22786 yeah no INFO-level nodes are getting canceled, so if we print a cancellation message at the same level as teh workunit, the user will only see them with -ldebug
w

witty-crayon-22786

08/20/2020, 12:04 AM
@fast-nail-55400, @hundreds-breakfast-49010: this is primarily the “watching a tree case”… but i feel like rendering which processes were cancelled via control+c would not be unreasonable.
@hundreds-breakfast-49010: hm. weird. but yea, i can follow up on that independently. the primary thing that i think is important is accurate is the dynamic ui
and that consumes debug as well.
..hm, no. it uses with_workunits. weird!
h

hundreds-breakfast-49010

08/20/2020, 12:27 AM
what uses
with_workunits
?
@witty-crayon-22786
w

witty-crayon-22786

08/20/2020, 12:27 AM
BoundedCommandRunner
h

hundreds-breakfast-49010

08/20/2020, 12:29 AM
so in the dynamic UI case, if it's active we won't ever print a "Started" message, so the manifestation of a canceled workunit will be the absence of a "Completed" message for it. which is fine
with --no-dynamic-ui we'll see Started without Completed messages, so to make that clearer we might want to log a "Canceled" message at the same level
w

witty-crayon-22786

08/20/2020, 12:31 AM
@hundreds-breakfast-49010: it’s not fine, because that workunit will just “run forever” in the dynamic ui
(that’s was the issue i observed that caused me to file the ticket)
h

hundreds-breakfast-49010

08/20/2020, 12:33 AM
I mean in terms of what to log, if anything, in the canceled case
w

witty-crayon-22786

08/20/2020, 12:34 AM
got it.