tests: parallel batching for packages when running unit tests#17621
tests: parallel batching for packages when running unit tests#17621parthea wants to merge 13 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates ci/run_conditional_tests.sh to allow passing specific package paths as optional arguments to run tests only on those directories, defaulting to the entire packages directory if no arguments are provided. The review feedback highlights two key improvements: first, making the check for the "packages" directory robust against trailing slashes (e.g., "packages/") by stripping them before comparison; second, replacing the fragile parsing of ls with safer, more idiomatic Bash globbing.
e9b44e3 to
f2ce4a0
Compare
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates ci/run_conditional_tests.sh to allow passing specific package paths as optional arguments to restrict testing to those directories, falling back to the default behavior of scanning the packages directory if no arguments are provided. The review feedback identifies two robustness issues: first, handling non-existent directory arguments by validating their existence and exiting early; second, preventing issues with unmatched glob patterns when a directory is empty by verifying that each resolved path actually exists before processing.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
8f3ffeb to
d3a042e
Compare
5170f35 to
4898748
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a batching mechanism for CI testing via a new Python script, updates the conditional test runner script to accept specific package paths, and lowers the test coverage threshold for the Django Spanner package. The review feedback highlights a critical bug in the batch slicing logic that can cause packages to run in multiple batches, recommends reverting the coverage threshold reduction, and suggests quoting array expansions in the shell script to prevent word-splitting issues.
| def get_weighted_package_list(): | ||
| """Audits the packages directory and expands the list based on weights.""" | ||
| raw_paths = sorted(glob.glob("packages/*")) | ||
| package_paths = [p for p in raw_paths if os.path.isdir(p)] | ||
|
|
||
| weighted_list = [] | ||
| for path in package_paths: | ||
| pkg_name = os.path.basename(path) | ||
| weight = PACKAGE_WEIGHTS.get(pkg_name, 1) | ||
|
|
||
| for _ in range(weight): | ||
| weighted_list.append(path) | ||
|
|
||
| return weighted_list | ||
|
|
||
| def get_batch_indices(): | ||
| """Returns a JSON string of the array of batch indices for GitHub Actions matrix.""" | ||
| weighted_list = get_weighted_package_list() | ||
| total_units = len(weighted_list) | ||
| num_batches = math.ceil(total_units / BATCH_SIZE) | ||
|
|
||
| if num_batches == 0: | ||
| num_batches = 1 | ||
|
|
||
| return json.dumps(list(range(num_batches))) | ||
|
|
||
| def get_batch_slice(batch_index): | ||
| """Returns a space-separated string of unique package paths for a specific batch index.""" | ||
| weighted_list = get_weighted_package_list() | ||
| total_units = len(weighted_list) | ||
| start_idx = batch_index * BATCH_SIZE | ||
|
|
||
| if start_idx >= total_units: | ||
| return "" | ||
|
|
||
| slice_window = weighted_list[start_idx : start_idx + BATCH_SIZE] | ||
|
|
||
| unique_packages = [] | ||
| for pkg in slice_window: | ||
| if pkg not in unique_packages: | ||
| unique_packages.append(pkg) | ||
|
|
||
| return " ".join(unique_packages) |
There was a problem hiding this comment.
The current slicing logic has a significant bug: duplicating packages in weighted_list and then slicing it by BATCH_SIZE leads to the same package being included in multiple batches if its weight is larger than BATCH_SIZE or if it spans across a batch boundary.
For example, if a package has a weight of 5 and starts at index 8 with a BATCH_SIZE of 10, it will occupy indices 8, 9, 10, 11, and 12. Batch 0 (indices 0-9) and Batch 1 (indices 10-19) will both contain this package, causing its tests to run twice on different runners. This defeats the purpose of weighting and wastes CI resources.
Instead, we should use a greedy load-balancing algorithm (like Longest Processing Time first) to distribute packages into batches such that each package is assigned to exactly one batch, and the total weights of the batches are as balanced as possible.
def get_batches():
"""Distributes packages into batches using a greedy load-balancing algorithm."""
raw_paths = sorted(glob.glob("packages/*"))
package_paths = [p for p in raw_paths if os.path.isdir(p)]
packages_with_weights = []
total_weight = 0
for path in package_paths:
pkg_name = os.path.basename(path)
weight = PACKAGE_WEIGHTS.get(pkg_name, 1)
packages_with_weights.append((path, weight))
total_weight += weight
num_batches = math.ceil(total_weight / BATCH_SIZE)
if num_batches == 0:
num_batches = 1
# Sort packages by weight descending (Longest Processing Time first)
packages_with_weights.sort(key=lambda x: x[1], reverse=True)
batches = [[] for _ in range(num_batches)]
batch_weights = [0] * num_batches
for path, weight in packages_with_weights:
min_idx = batch_weights.index(min(batch_weights))
batches[min_idx].append(path)
batch_weights[min_idx] += weight
return batches
def get_batch_indices():
"""Returns a JSON string of the array of batch indices for GitHub Actions matrix."""
batches = get_batches()
return json.dumps(list(range(len(batches))))
def get_batch_slice(batch_index):
"""Returns a space-separated string of unique package paths for a specific batch index."""
batches = get_batches()
if batch_index < 0 or batch_index >= len(batches):
return ""
return " ".join(batches[batch_index])| "--cov-config=.coveragerc", | ||
| "--cov-report=", | ||
| "--cov-fail-under=75", | ||
| "--cov-fail-under=0", |
There was a problem hiding this comment.
This change lowers the minimum test coverage requirement for django-google-spanner to 0, which effectively disables coverage enforcement. This seems unrelated to the parallel batching changes and lowers the quality gate for this package. Please restore the coverage requirement to 75 or explain why this change is necessary.
| "--cov-fail-under=0", | |
| "--cov-fail-under=75", |
|
|
||
| RETVAL=0 | ||
|
|
||
| for subdir in ${subdirs[@]}; do |
There was a problem hiding this comment.
A legacy bottleneck in the unit test presubmit causes a timeout/crash after 4.5 hours (log) when testing all packages. This is blocking PR #17585 (Python 3.15 testing for all packages).
This PR introduces dynamic parallel execution, dropping runtime from several hours to under 40 minutes.
Key Changes
Add
TEST_ALL_PACKAGESvariable: When set totrue, it forces the workflow to discover and execute testing across all repository packages regardless of the git diff.ci/run_conditional_tests.sh: Accepts optional explicit package paths to run to facilitate batchingAdds a discover-packages job to the GH action workflow to batch testing of packages based on a configurable
BATCH_SIZE(default: 5). It respects theTEST_ALL_PACKAGESvariable which toggles testing of all packages, vs only the ones that have a diff.Parallel Execution: Splitting the workload across a dynamic matrix of runners. Coverage artifacts use unique index names to prevent overwrites.