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

hundreds-breakfast-49010

06/17/2020, 5:27 PM
interested in people's opinions about the naming of the Python class that allows rules to modify their workunits in https://github.com/pantsbuild/pants/pull/10053 - @polite-garden-50641 @hundreds-father-404
w

witty-crayon-22786

06/17/2020, 5:30 PM
probably also fine to change it in a followup… we’ll want to apply it to
TestResult
to fix https://github.com/pantsbuild/pants/blob/77e089be985c5f87710344376738754e213a0302/src/python/pants/core/goals/test.py#L35-L37
(…or could do that in this PR i guess)
class TestResult(EngineAware): ...
h

hundreds-father-404

06/17/2020, 5:36 PM
I’m at the grocery store. Can check when I get home
w

witty-crayon-22786

06/17/2020, 5:39 PM
@hundreds-breakfast-49010: i wouldn’t block on naming… but consider actually applying to TestResult to fix that todo?
h

hundreds-breakfast-49010

06/17/2020, 5:40 PM
@witty-crayon-22786 sure, I think I'm gonna make a couple more edits anyway
h

hundreds-father-404

06/17/2020, 6:07 PM
okay commented
w

witty-crayon-22786

06/17/2020, 6:12 PM
… it also occurs to me that the interface might need a little bit more adjustment to actually allow for fixing that TODO. because just rendering at ERROR wouldn’t change the content of the message =x
👍 1
h

hundreds-breakfast-49010

06/17/2020, 6:14 PM
@witty-crayon-22786 so do we also need to be able to edit the description? I'm not sure if we want that
or maybe we want an additional field
on workunits, specifically for error messages
w

witty-crayon-22786

06/17/2020, 6:15 PM
might be good to take a step back and look specifically at how we want to address that precise case.
👍 1
it’s possible that … what we actually want is for it to be able to indicate that the workunit “failed”. because then we could log
Failed: Run tests for ...
(which would indirectly affect the level)
….sorry about this. i was much too abstract in the ticket. should have spoken directly to that usecase =(
h

hundreds-father-404

06/17/2020, 6:18 PM
Whatever we do, I recommend trying to keep things as concise as possible in the CI. Have a high signal to noise ratio
w

witty-crayon-22786

06/17/2020, 6:18 PM
luckily, i think it all sortof overlaps and has the same skeleton… the Result type affects the workunit completion
h

hundreds-father-404

06/17/2020, 6:18 PM
One concern I have with logging the result is that we can’t have new lines and it will truncate too long of messages, right? The result needs to maintain formatting and colors
w

witty-crayon-22786

06/17/2020, 6:20 PM
the rendering of the stdout/stderr will be via the workunit, rather than via this interface, is what i was thinking…
crap. it’s possible that it’s actually the
Process
intrinsic that should be indicating that it failed.
(at least for tests…? maybe not in general.)
…oh. does that work if this interface is applied to
ProcessResult
…!?!
@hundreds-breakfast-49010: i think that to fix the test TODO in particular, we might want to try to apply this in particular to ProcessResult… which is an intrinsic, unfortunately.
h

hundreds-breakfast-49010

06/17/2020, 6:26 PM
@witty-crayon-22786 this interface applies specifically to
Tasks
. although nothing says we couldn't do something similar for Processes
w

witty-crayon-22786

06/17/2020, 6:27 PM
in the case of intrinsics, i think that we could hack a bit and just assert that the interface is implemented… because we control the types
but yea =/ tweaks
h

hundreds-breakfast-49010

06/17/2020, 6:28 PM
i.e. change the signature of
run_wrapped_node
for
MultiPLatofrmExecuteProcess
w

witty-crayon-22786

06/17/2020, 6:28 PM
maybe.
yea.
but, to try and learn from the first time: the high level goal is to allow the workunit that has the stdout/stderr for a failing process to identify that and render it (we can render the stdout/stderr in a followup… we just need to connect them now so that we can affect the
Completed
message)
it’s also possible that we just want to treat Process as a special case
h

hundreds-father-404

06/17/2020, 6:30 PM
@witty-crayon-22786 not all linters will run via a Process. For example, the dep cycle detection linter
w

witty-crayon-22786

06/17/2020, 6:30 PM
@hundreds-father-404: yea, that’s where this interface will come in i think.
because those also won’t have stdout/stderr
h

hundreds-breakfast-49010

06/17/2020, 6:32 PM
I thought the point of this interface was so that the
@rule
could encode whatever business logic was necessary to figure out if a failure had happened
that implies that if we want to render stdout/stderr for a failing process, it's not theprocess's responsibility, but the rule's
but the process has its own stdout/stderr, not the rule
w

witty-crayon-22786

06/17/2020, 6:33 PM
@hundreds-breakfast-49010: possibly: but think about how much more you’d have to change the interface to do that
@hundreds-breakfast-49010: as Eric pointed out, there will be cases where the rule failing a workunit is necessary. but: https://pantsbuild.slack.com/archives/C0D7TNJHL/p1592418555196200?thread_ts=1592414877.187000&cid=C0D7TNJHL
but the process has its own stdout/stderr, not the rule
right, exactly =/
h

hundreds-breakfast-49010

06/17/2020, 6:34 PM
the rule will have access to the stdout/stderr bytes on the
ProcessResult
, so the rule can choose to print this out if it wants
👍 1
w

witty-crayon-22786

06/17/2020, 6:35 PM
if that’s added to the interface
h

hundreds-breakfast-49010

06/17/2020, 6:36 PM
yeah, we might need an additional abstract method
text_output(self) ->Opitional[String]
or something like that
👍 1
then it's not just "print the stdout/stderr" but "print something, after the rule has had a chance to potentially modify the string"
👍 1
w

witty-crayon-22786

06/17/2020, 6:36 PM
...possibly…?
i’m not sure whether doing that on Process is better. it feels like it might be… but it’s also possible that it’s not much of a special case if you do it by marking
ProcessResult
EngineAware
, and then having the special case just be that we use the same interface for the intrinsic.
h

hundreds-father-404

06/17/2020, 7:40 PM
I think doing on the
Process
wouldn’t generalize well enough for cases where we don’t use a
Process
, right? It seems more generic to have this mechanism live on the
@rule
itself
h

hundreds-breakfast-49010

06/17/2020, 7:44 PM
yeah I was thinking that, in addition to this interface (I'm just about to update the PR btw @witty-crayon-22786 @hundreds-father-404)
that applies on rules
ther ewould be a separate special-case in the intrinsic-handling portion of the engine for the result of a process
which isn't a
Task
, it's a
MultiPlatformExecuteProcess
. so the current code path doesn't affect it
but then we'd have to decide what behavior we want out of the
Process
intrinsic case. and that would only affect the process's workunit, not the workunit of the rule that ran the process
w

witty-crayon-22786

06/17/2020, 7:48 PM
whatever we do, let’s ensure that we can resolve that TODO with it… sorry, i should have created a ticket much more specific to this usecase, but got lazy
h

hundreds-breakfast-49010

06/17/2020, 7:49 PM
agreed, but it should probably be a separate issue/PR
this PR has been pending for a bit and I'm eager to get it merged
w

witty-crayon-22786

06/17/2020, 7:51 PM
ok. as long as we understand that i messed up, and that the primary reason to work on this right now* was to fix that TODO 😃
h

hundreds-breakfast-49010

06/17/2020, 7:54 PM
@witty-crayon-22786 so, my thinking is, we would actually want to have the
coordinater_of_tests
rule be the one that sets success/failure on its workunit
well either that or
run_tests
itself
so`Test` is the type that would subclass
EngineAware
, and it would set a warning level or not based on
TestResult
, just like the logger call currently does
that way, the Process intrinsic that runs the process that executes the test doesn't need to do anything special
and it makes sense to me that the Process intrinsic shouldn't know what constitutes success or failure for the
pytest
binary that it executes, so long as it didn't actually crash
it's the job of
goals/test.py
to inspect the output of that pytest run and decide what does and does not constitute a test failure at that level
w

witty-crayon-22786

06/17/2020, 7:58 PM
so long as it didn’t actually crash
it did, by virtue of returning 1
doing it at the @rule level might work for this case, but it means that rules will need a way to attach stdout/stderr cleanly (again, not now, but in a followup)… whereas at the Process level, we already have it.
(but on the flip side, “returning 1” will not always be a signal that we should `warn`/`error` a Process either… and successful processes will want to go to
warn
in some cases.)
so… yea. whatever allows for fixing the TODO (and having stdout/stderr rendered in the future)
h

hundreds-breakfast-49010

06/17/2020, 9:59 PM
so right now we get the string that we print for a started/completed workunit from this code: https://github.com/pantsbuild/pants/blob/master/src/rust/engine/workunit_store/src/lib.rs#L66
which just uses the description on
WorkunitMetadata
we could in principle just add a new method on
EngineAware
that lets a rule update the description
on the other hand, maybe we don't want to do this (especially becuase we currently truncate the description in the console)
and instead we want a completely new optional metadata field, controllable by
EngineAware
, for printing potentially-lengthy amounts of information
w

witty-crayon-22786

06/17/2020, 10:01 PM
that might be reasonable, but it would be dynamic… you might not want to “change the description” if it fails… rather, set a status slug, or just mark the thing failed
h

hundreds-breakfast-49010

06/17/2020, 10:02 PM
yeah, and we can do that already with the
level()
method (or we can once that PR is green)
w

witty-crayon-22786

06/17/2020, 10:02 PM
right.
h

hundreds-breakfast-49010

06/17/2020, 10:02 PM
but it seems like we want to be able to do things like print a test stdout to the console in a failure, as determined by an
@rule
👍 1
w

witty-crayon-22786

06/17/2020, 10:02 PM
so what you said earlier about allowing for a blob of output made a bit more sense to me
yea.
h

hundreds-breakfast-49010

06/17/2020, 10:03 PM
the rule has access to the Bytes of stdout on its
TestResult
instance,and it can call a method on its own output type to set this "blob" to that output (or maybe just a subset of that output)
the point is, it's the rule, not the process that determiens what to do
👍 1
but the process still ran and there's still a
Digest
with that same stdout/stderr attached as an artifact to its own workunit
w

witty-crayon-22786

06/17/2020, 10:03 PM
yes. and that’s attractive.
h

hundreds-breakfast-49010

06/17/2020, 10:04 PM
which streaming workunit logic can either extract or not
w

witty-crayon-22786

06/17/2020, 10:04 PM
right: that aspect is mismatched.
there is a mismatch in that the Process workunit will have the output attached, but the rule output will do logging
h

hundreds-breakfast-49010

06/17/2020, 10:04 PM
I'd like to get @polite-garden-50641’s opinion on this
w

witty-crayon-22786

06/17/2020, 10:04 PM
it’s also mismatched because the
@rule
doesn’t know whether it was a cache hit
so it will render output even for tests that we didn’t actually run.
h

hundreds-breakfast-49010

06/17/2020, 10:05 PM
but if a test failed, don't we want to show the output again?
w

witty-crayon-22786

06/17/2020, 10:06 PM
yes… i’m not sure if that is universally true, but yes in this case.
h

hundreds-breakfast-49010

06/17/2020, 10:06 PM
when will this happen in practice? someone runs
pants test
on their code, some test fails, then they run it again without changing any of the source files, the test would fail in exactly the same way if it ran again, but we cache the result
w

witty-crayon-22786

06/17/2020, 10:07 PM
no, because we don’t cache failures
h

hundreds-breakfast-49010

06/17/2020, 10:07 PM
the rule shouldn't care whether its result was cached or not. all it knows is that there were failures in the underlying
TestResult
, so it displays them
w

witty-crayon-22786

06/17/2020, 10:07 PM
it’s more that if we cache hit, and a rule at
info
says “here’s some output that i think just actually ran, but which is actually a cache hit!“… then we wouldn’t want to render that.
…this is pretty micro. i expect that this can be made to work. there will just need to be more tweaking
h

hundreds-breakfast-49010

06/17/2020, 10:08 PM
I'm not sure that's the case. a rule shouldn't be aware of whether its dependencies were cached or not, right?
so it shouldn't mkae different decisions about what to print based on this
w

witty-crayon-22786

06/17/2020, 10:09 PM
it’s actually possible that we don’t want to land this until 1.30 is cut, and then we can have a bit more time for these tweaks
h

hundreds-breakfast-49010

06/17/2020, 10:09 PM
legit
w

witty-crayon-22786

06/17/2020, 10:09 PM
so it shouldn’t mkae different decisions about what to print based on this
correct: rules should not. but the engine should.
h

hundreds-breakfast-49010

06/17/2020, 10:10 PM
so now it seems to me like the behavior we actually want is for a rule to be able to control the print behavior of one of its child workunits
not its own workunit
w

witty-crayon-22786

06/17/2020, 10:10 PM
https://pantsbuild.slack.com/archives/C0D7TNJHL/p1592431745226300?thread_ts=1592414877.187000&cid=C0D7TNJHL given this -> maybe not addressing the TODO quite yet is the right call, and we can address it after 1.30 is cut.
h

hundreds-breakfast-49010

06/17/2020, 10:10 PM
yeah, i agree it's fine not to try to get this in by 1.30
w

witty-crayon-22786

06/17/2020, 10:11 PM
the “behavior of child workunits” thing feels pretty scary to me. because you don’t know how many children you have, in general
the creator of a process might be able to add stuff to it though… i think that’s why
Process.description
works so well.
h

hundreds-breakfast-49010

06/17/2020, 10:14 PM
agreed
we could ad a new API to
ProcessRequest
that says "if this particular process fails, print out its stdout/stderr`
w

witty-crayon-22786

06/17/2020, 10:15 PM
or: if it fails, change it’s level to X
… i guess that’s equivalent? but also, fails isn’t nuanced enough: i think
warn
will definitely be a thing.
and we think that
warn
can probably only be triggered by a rule inspecting the output
h

hundreds-breakfast-49010

06/17/2020, 10:17 PM
then we're back to the situation where a rule can change properties of its child workunits
acutally I don't think this would even work, because by the time the rule gets the output, the child workunit that ran the process will have finished
w

witty-crayon-22786

06/17/2020, 10:17 PM
not necessarily… it’s just a point in favor of a rule being able to trigger output.
h

hundreds-breakfast-49010

06/17/2020, 10:18 PM
yeah, I think this is fine. it just means that the result of a test failure is associated with the rule workunit, not the process workunit. but I think this is broadly the right thing to do
👍 1
w

witty-crayon-22786

06/17/2020, 10:19 PM
yea. we can figure out the disjointedness between rules rendering test results and processes actually holding stuff later.