https://pantsbuild.org/ logo
Join Slack
Powered by
interested in people's opinions about the naming o...
# development
h
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
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
I’m at the grocery store. Can check when I get home
w
@hundreds-breakfast-49010: i wouldn’t block on naming… but consider actually applying to TestResult to fix that todo?
h
@witty-crayon-22786 sure, I think I'm gonna make a couple more edits anyway
h
okay commented
w
… 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
@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
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
Whatever we do, I recommend trying to keep things as concise as possible in the CI. Have a high signal to noise ratio
w
luckily, i think it all sortof overlaps and has the same skeleton… the Result type affects the workunit completion
h
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
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
@witty-crayon-22786 this interface applies specifically to 
Tasks
. although nothing says we couldn't do something similar for Processes
w
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
i.e. change the signature of 
run_wrapped_node
for 
MultiPLatofrmExecuteProcess
w
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
@witty-crayon-22786 not all linters will run via a Process. For example, the dep cycle detection linter
w
@hundreds-father-404: yea, that’s where this interface will come in i think.
because those also won’t have stdout/stderr
h
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
@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
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
if that’s added to the interface
h
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
...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
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
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
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
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
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
@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
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
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
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
yeah, and we can do that already with the 
level()
method (or we can once that PR is green)
w
right.
h
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
so what you said earlier about allowing for a blob of output made a bit more sense to me
yea.
h
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
yes. and that’s attractive.
h
which streaming workunit logic can either extract or not
w
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
I'd like to get @polite-garden-50641’s opinion on this
w
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
but if a test failed, don't we want to show the output again?
w
yes… i’m not sure if that is universally true, but yes in this case.
h
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
no, because we don’t cache failures
h
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
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
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
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
legit
w
so it shouldn’t mkae different decisions about what to print based on this
correct: rules should not. but the engine should.
h
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
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
yeah, i agree it's fine not to try to get this in by 1.30
w
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
agreed
we could ad a new API to 
ProcessRequest
that says "if this particular process fails, print out its stdout/stderr`
w
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
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
not necessarily… it’s just a point in favor of a rule being able to trigger output.
h
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
yea. we can figure out the disjointedness between rules rendering test results and processes actually holding stuff later.
Open in Slack
PreviousNext
Powered by