Add support for batched pytest execution
#17385
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
All the existing tests for the logic changed here are integration tests, so I'll need to wait until the entire batching implementation is done to check if the new logic works... but this at least makes the types line up.
Same as coverage, the tests for the change logic are all integration tests so we'll need to wait until the end to see if this works properly...
The custom partitioner still creates one partition per field-set, but the runner rule should now be capable of testing an arbitrary number of input field-sets. As part of this, I've defined the metadata type for `pytest` partitions and shifted some logic for calculating metadata fields from the runner rule into the partitioner rule.
If two `python_test` targets have the same value for
`batch_compatibility_tag`, then they are eligible to be batched into the
same `pytest` process. They might _not_ be batched in the same process
if:
* There are "too many" compatible tests as determined by the
`[test].batch_size` config option
* Compatible tests have some incompatible Pants metadata (i.e.
different `extra_env_vars` or `xdist_concurrency`)
Tests that don't set the new field are assumed to be incompatible with
all others, and continue to run one-target-per-process.
danxmoran
requested review from
Eric-Arellano and
stuhood
and removed request for
Eric-Arellano
danxmoran
commented
Oct 28, 2022
Improve batching hit-rate when the vars are the same, but listed in a different order.
Ensure that the expected results don't change in the presence of batching.
stuhood
approved these changes
Oct 28, 2022
src/python/pants/backend/python/goals/coverage_py_integration_test.py
Outdated
Show resolved
Hide resolved
Only calculate it if needed.
Make sure coverage collection works in both cases
I think ideally the plugin wouldn't need to think about this, but for now the core `test` logic will raise an error if a partition spans multiple environments so we should do our best not to get into that situation...
auto-merge was automatically disabled
October 28, 2022 20:24
Head branch was pushed to by a user without write access
|
🎉 |
| if concurrency is None: | ||
| contents = await Get(DigestContents, Digest, field_set_source_files.snapshot.digest) | ||
| concurrency = _count_pytest_tests(contents) | ||
| xdist_concurrency = concurrency | ||
|
|
||
| timeout_seconds: int | None = None | ||
| for field_set in request.field_sets: | ||
| timeout = field_set.timeout.calculate_from_global_options(test_subsystem, pytest) |
There was a problem hiding this comment.
It probably would be good to update the help for the timeout field to mention this summing behavior
Comment on lines
+907
to
+908
| * Compatible tests have some incompatibility in Pants metadata (i.e. different \ | ||
| `resolve`s or `extra_env_vars`). |
There was a problem hiding this comment.
Thoughts on adding logs in this situation?
There was a problem hiding this comment.
Not opposed - in our repo we don't (yet) use the Pants features that cause this sort of incompatibility so I'm not sure how to judge the noisiness vs. usefulness
There was a problem hiding this comment.
Maybe debug level to start
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #14941
It's sometimes safe to run multiple
python_tests within a singlepytestprocess, and doing so can give significant wins by allowing reuse of expensive test setup/teardown logic. To allow users to opt into this behavior we introduce a newbatch_compatibility_tagfield onpython_test, with semantics:pytestprocesspython_test, the test is explicitly incompatible with the other and is guaranteed to not run in the samepytestprocesspython_test, the tests are explicitly compatible and may run in the samepytestprocessCompatible tests may not end up in the same
pytestbatch if:[test].batch_sizeresolveorextra_env_vars)When tests with the same
batch_compatibility_taghave incompatibilities in some other Pants metadata, the custom partitioning rule will split them into separate partitions. We'd previously discussed raising an error in this case when calling the fieldbatch_key/batch_tag, but IMO that approach will have a worse UX - this way users can set a high-levelbatch_compatibility_tagusing__defaults__and then have things continue to Just Work as they add/removeextra_env_varsand parameterizeresolves on lower-level targets.