https://pantsbuild.org/ logo
Join Slack
Powered by
<https://github.com/pantsbuild/pants/pull/8855> <@...
# development
h
https://github.com/pantsbuild/pants/pull/8855 @gentle-wolf-58752 this will be helpful I think for the work you're doing with interactively-debuggable tests
👀 1
a
is there a reason we don't just accept an 
ExecuteProcessRequest
for the interactive process execution?
w
@hundreds-breakfast-49010: please see my comment here: https://github.com/pantsbuild/pants/pull/8827#discussion_r360232788 ... i really think that we might be contorting to fit the current implementation, and should revisit 
foreground=True
a
or the interactive request can wrap an 
ExecuteProcessRequest
as a dataclass field (maybe)
w
yea, possibly.
the relevant point from that discussion is that it's really important for the debug case to not require a totally forked codepath (ie, what the 
--debug
PR currently does)
a
oh ok thank you
w
so yea, if that's via a wrapper or a flag, that's fine. but might need to occur deep within the graph
...in order to allow for composibility
a
deep within the graph
sorry, could you describe what this means?
w
the python test rules might be arbitrarily deep in the graph
might/should
a
yes got it
w
so a console_rule specific feature ("at the root of the graph") isn't composible
a
we're already tagging rules with names, could potentially tag them as "should use interactive display, unless this is toggled off"
not sure how to propagate that though mmmmm
w
well, toggling the behavior can be tackled via options
so that part is totally fine.
a
yeah
w
as in: it's totally fine to do 
foreground=pytest.get_options().debug
a
that actually seems great
i was thinking it would be something that would be controlled at a higher level for some reason -- that seems super reasonable
h
I believe @gentle-wolf-58752 is incorporating some some of the changes mentioned in that PR to make the --debug flag duplicate less code
a
if we end up having multiple requests using 
foreground
at once then eh
ok, awesome
h
the 
foreground
flag you're talking about was a proposal for adding a flag on 
ExecuteProcessRequest
right?
if I remember correctly one of the reasons we didn't go that route was because the main thing that 
InteractiveProcess
does differently than 
EPR
was always run locally and always run grabbing the local screen
and that constraint also matters with respect to running EPRs that might be deep in the graph, because it forces all the rules between the goal console_rule and the EPR-running rule to not be cached as well
it might be a good idea for 
InteractiveProcessRequest
to just wrap 
ExecuteProcessRequest
instead of duplicating fields
I'm trying to think whether that's something that we thought about and rejected for any reason
you know I don't think anywhere in the code is actually using 
working_directory
on EPR
g
https://pantsbuild.slack.com/archives/C0D7TNJHL/p1576882959029000?thread_ts=1576881237.023100&amp;cid=C0D7TNJHL IIRC this was part of the concern / why my approach allows only a single test target to be run interactively.
https://pantsbuild.slack.com/archives/C0D7TNJHL/p1576882958028800?thread_ts=1576881237.023100&amp;cid=C0D7TNJHL https://github.com/pantsbuild/pants/pull/8852 should help dedupe a fair amount of whats in the debugger rule (at least around pex creation). It sounds like there’s more to the 
foreground
consideration than I’m familiar with on the engine side
a
i think one hidden facet here is that if we want to run multiple ExecuteProcessRequests in parallel, that would often be a result of running them in parallel with MultiGet as we do with pytest and remoting in ci. in the case of remoting, we’d need to extend the remote execution api to allow for interactive output streams, which are being discussed and can be experimented with via the use of scoot.
but also, we might want to in that case just run all pytests at once instead of sharding them at all if we’re going for interactivity
and then just don’t try to extend it to remoting just yet
not sure if that makes sense
g
I’m trying to follow / catch up with some of the earlier conversation - are you thinking that when running multiple test targets, effectively bundle them into a single interactive process (if debugging) otherwise shard / remote in EPRs as currently done?
a
yes!
✔️ 1
g
So the difference from the current branch would be adding a rule to bundle those test targets, we’d still need a separate rule to build the pex / run in an interactive process (ie. we couldn’t add that to the 
python_test_runner
since those are meant to allow parallel execution)
w
Re: remote output streams: 
foreground
would imply local (otherwise 
--debug
wouldn't work anyway)
Refactoring out the duplication in this test runner probably wouldn't prevent needing to do this for a bunch of other test runners
if the concern is with what the implementation would look like, i can take a look a bit over the holiday.
Open in Slack
PreviousNext
Powered by