@witty-crayon-22786 @happy-kitchen-89482 (and anyone else interested) here is the proposal for batching

pytest

calls you asked for on Friday: https://docs.google.com/document/d/1U0Q43bRod_EeVP4eQpcN36NMlxZ4CpRzTl2gMQ5HHvg/edit?usp=sharing

thanks! will take a look before EOD

thanks! responded with some questions - also added a short blurb at the bottom about interactions w/ sharding

@sparse-lifeguard-95737 alternative idea: a new target type called

python_test_batch

with a

sources

field. That avoids all the awkwardness with merging

extra_env_vars

, summing

timeout

I think that's pretty easy to implement also

@hundreds-father-404 the batch would need to span more than 1 dir’s worth of tests

regardless of syntax, is it important that you can still only run a single file within the batch? Or it's fine that the batch is run as one atomic unit?

running a single file within the batch is still important

the batch would need to span more than 1 dir’s worth of tests

That's fine,

sources

works with subdirs too, including

**/*_test.py

yeah but it’s not what

./pants tailor

will set up for you, AIUI - and not the convention we typically tell people to adopt 🙂

yeah but it’s not what ./pants tailor will set up for you, AIUI - and not the convention we typically tell people to adopt

Yeah, we would still be defaulting to 1 target per file because we can't infer intent of what you want your batches to be. But you can manually create a

python_test_batch

target instead, which will cause tailor to not try to add

python_tests

for those "owned" sources

it’s also important that

--changed-since

only chooses the actually-changed files within the batch for execution (vs. returning the entire batch when any one has changed)

it’s also important that --changed-since only chooses the actually-changed files within the batch for execution (vs. returning the entire batch when any one has changed)

Yeah, that's the biggest downside of

python_batch_target

imo. We used to have code to for

SpecsWithOrigin

, which meant we preserved the specific files users specified to us. That code got deleted when we added file-level targets,

python_test

vs

python_tests

We would likely to need to add back something like that, which is a bummer

another issue (unless I’m missing something) of

python_test_batch

is dealing with hierarchy. if you have

python_test_batch(sources=["**/test_*.py"])

at the ~root of your repo, but then you end up wanting to exclude specific subtrees / specific files, you have to both edit the root

sources

list to exclude files and add new targest. vs.

__defaults__

which (I think/hope) will Just Work with hierarchical settings

at the ~root of your repo, but then you end up wanting to exclude specific subtrees / specific files, you have to both edit the root sources list to exclude files and add new targest.

Agreed. This right here is precisely why we added

overrides

🙂 it used to be the case that you either changed metadata for every generated target, or had to create a new target and modify the existing one to exclude those old sources

i still view batching as a tradeoff, so i don’t really think it should be a default behavior. cache keys become larger (and there isn’t really a safe path to fixing that). so i don’t think that we’d want to do it by default.

Yeah, I think you opt in to batching, and I think a batch target is overkill right now

Read through the latest comments on the doc - sounds to me like

batch_id

+

__defaults__

is the approach. I am eager to get started 🙂 I’ll poke around in the code to get some ideas on the impl - @witty-crayon-22786 if you already have some thoughts, can you send them my way?

Yeah, I think batch_id is the move. Wiring up that field should be pretty easy with the Target API, and is a good first step One prefactor you could probably do is rewrite

pytest_runner.py

to have new types and a new rule that generifies running Pytest. Whereas right now the logic lives inside the

TestFieldSet

rule, instead create this new rule that takes args like the file names to specify. It will roughly be a copy-and-paste of the body of the current rule. Then, the

TestFieldSet

rule will call it

How does this interact with lockfiles? I assume batches also cannot span multiple lockfiles?

agreed (I’ve been saying “resolve” internally instead of lockfile - are those effectively the same thing?)

Yep, resolve==lockfile

even if

batch_id

is generic, I think we’ll need a per-target batching strategy - for example

xdist_concurrency

will need to be the same for all tests in a pytest batch, but that field doesn’t exist for other test types

It’s easy to make it generic later, I think, so maybe start with just python_test?

I recommend starting only with Python

python-specific was also my thought but Stu raised the idea of making it generic so I’ll let him weigh in 🙂

I suspect trying to implement this change via

core/goals/test.py

will be a lot harder to pull off, vs the strategy I outlined above. But maybe necessary depending on what we want to do with test sharding Although I suspect it will be possible to only change

core/goals/test.py

to consider batch IDs when sharding, without needing to change anything else in the file. And then all the rest of the changes are in

pytest_runner.py

yeah I am wondering how generic the “plumbing” should be, even if we only end up adding the

batch_id

field to Python for now. high-level options that occur to me: 1. change all the existing types in the existing

test

flow to use batches. subsystems that don’t use batching will just use single-element collections everywhere 2. add a parallel type tree + goals for batched test requests / responses. subsystems register rules for either the single-file or batched types depending on their capabilities. the top-level

core/goals/test.py

sends requests for both single-element and batched field sets 3. isolate the changes to the

python

subsystem, somehow reconstruct batches from the single-element requests currently send by

core/goals/test.py

I am not sure if 3 is feasible (were you saying you have an idea for how it could work @hundreds-father-404?). 1 is nice in theory but potentially a pain for subsystem developers if it turns out most things aren’t batchable. 2 is complex but (hopefully?) makes it easy for subsystems to only implement what they need

yeah, I'm pretty confident #3 is possible thanks to the engine's design of deduplicating identical requests: https://pantsbuild.slack.com/archives/C0D7TNJHL/p1663863514744679?thread_ts=1663700934.944129&cid=C0D7TNJHL

@witty-crayon-22786 if you already have some thoughts, can you send them my way?

will do by EOD: thanks!

Dan and I DMed more about approach #3, and he correctly pointed out it won't work because an individual TestFieldSet doesn't know about any other TestFieldSets in play, so it wouldn't know which other files to run with Given that, I suspect that a mix of #1 and #2 is going to be cleanest. We have a lot of infrastructure around FieldSet to do things like discover what to run on. Given that you're still using the

python_test

target for batched runs, those targets are going to match against the current

PythonTestFieldSet

we have, but we need to run those with the batched runner and not individaul runner I think the rough implementation would look like: 1. Keep our current PythonTestFieldSet and the

TargetRootsToFieldSets

line in

test.py

2. But then filter the results into two sets: the individual runner vs batch runner 3. call the two distinct APIs,

Get(TestResult, TestFieldSet)

and

Get(TestResult, TestBatch)

(or return

TestBatchResult

maybe)

hm, possibly. i don’t like the idea of two distinct test APIs… would really be preferable to have a single API which supports multiple FieldSets which have already been declaratively declared compatible because they have the same x/y/z fields

it’s all connected 🕸️

@sparse-lifeguard-95737 LMK if you wan the TL;DR of my changes

yes please!

I'll make a new 🧵

meanwhile: i’m going to braindump the computed Fields idea somewhere, because i think it has legs.