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

hundreds-breakfast-49010

12/20/2019, 10:33 PM
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

aloof-angle-91616

12/20/2019, 10:48 PM
is there a reason we don't just accept an
ExecuteProcessRequest
for the interactive process execution?
w

witty-crayon-22786

12/20/2019, 10:53 PM
@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

aloof-angle-91616

12/20/2019, 10:54 PM
or the interactive request can wrap an
ExecuteProcessRequest
as a dataclass field (maybe)
w

witty-crayon-22786

12/20/2019, 10:56 PM
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

aloof-angle-91616

12/20/2019, 10:57 PM
oh ok thank you
w

witty-crayon-22786

12/20/2019, 10:58 PM
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

aloof-angle-91616

12/20/2019, 10:59 PM
deep within the graph
sorry, could you describe what this means?
w

witty-crayon-22786

12/20/2019, 10:59 PM
the python test rules might be arbitrarily deep in the graph
might/should
a

aloof-angle-91616

12/20/2019, 10:59 PM
yes got it
w

witty-crayon-22786

12/20/2019, 10:59 PM
so a console_rule specific feature ("at the root of the graph") isn't composible
a

aloof-angle-91616

12/20/2019, 11:00 PM
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

witty-crayon-22786

12/20/2019, 11:00 PM
well, toggling the behavior can be tackled via options
so that part is totally fine.
a

aloof-angle-91616

12/20/2019, 11:00 PM
yeah
w

witty-crayon-22786

12/20/2019, 11:01 PM
as in: it's totally fine to do
foreground=pytest.get_options().debug
a

aloof-angle-91616

12/20/2019, 11:01 PM
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

hundreds-breakfast-49010

12/20/2019, 11:02 PM
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

aloof-angle-91616

12/20/2019, 11:02 PM
if we end up having multiple requests using
foreground
at once then eh
ok, awesome
h

hundreds-breakfast-49010

12/20/2019, 11:03 PM
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

gentle-wolf-58752

12/21/2019, 12:55 AM
https://pantsbuild.slack.com/archives/C0D7TNJHL/p1576882959029000?thread_ts=1576881237.023100&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&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

aloof-angle-91616

12/21/2019, 1:01 AM
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

gentle-wolf-58752

12/21/2019, 1:10 AM
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

aloof-angle-91616

12/21/2019, 1:10 AM
yes!
✔️ 1
g

gentle-wolf-58752

12/21/2019, 1:13 AM
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

witty-crayon-22786

12/21/2019, 3:30 AM
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.