Skip to content

Commit d09dc3c

Browse files
smackeseyclaude
authored andcommitted
Add REFRESH_DURATIONS mode to internal pipeline for pytest-split duration refresh (#22765)
## Summary & Motivation Stacked on #22715 (pytest-split adoption). That PR introduces splitting via `pytest-split` but, without `.test_durations*` files, splitting falls back to count-based balancing. This PR wires up the machinery to produce and consume per-suite duration data. Rather than maintain a separate Buildkite pipeline whose suite list duplicates the split config in `python_packages.py` / OSS `packages.py`, refresh runs as a **mode** of the existing `internal` pipeline. The same split config drives both normal sharded CI and the periodic refresh — no separate list to keep in sync. Refresh is triggered manually (no schedule); a Buildkite block step gates the final PR-creation step so a human can inspect the collected artifacts before they're committed. ## How it works **`REFRESH_DURATIONS=1` flag.** A new `BuildConfig.refresh_durations` boolean lives in `buildkite-shared/context.py` and is settable via the `REFRESH_DURATIONS` env var or `[REFRESH_DURATIONS]` magic string in the build message. **Step generation diverges in `PackageSpec.build_steps`.** For each tox factor: - **`splits > 1` in refresh mode:** collapse the parallel fan-out to a single un-sharded step with `--store-durations --durations-path .test_durations-<factor>[-<label_suffix>]`. Soft-fails so one broken suite doesn't lose the rest. A `command_wrapper` does `set +e; <tox>; status=$?; cd "$BUILDKITE_BUILD_CHECKOUT_PATH"; buildkite-agent artifact upload "<dir>/.test_durations-..."; exit $status` — disables `set -e` so the upload always runs, and `cd`s back to repo root so the artifact is stored with its full repo-relative path (basenames would collide across packages). - **`splits <= 1` in refresh mode:** emit a normal step (which the top-level filter drops). Not emitting nothing keeps OSS helpers like `build_helm_steps` (`assert len(pkg_steps) == 1`) working. - **Normal mode:** unchanged behavior plus `--durations-path .test_durations-<factor>[-<label_suffix>]` so pytest-split reads exactly the file refresh writes. Naming includes `label_suffix` to keep OSS dagster-dbt's four `<deps>-core-main` ToxFactors (rest / test_resource / test_asset_checks / cli) from clobbering each other's files. **Top-level transform via `apply_transforms`.** `InternalBuildkiteContext.apply_transforms` is overridden to run `refresh_durations.apply` before delegating to the shared transform. The transform: 1. Drops every step that doesn't carry `--store-durations` (i.e. lint, kaniko, helm, dogfood, etc. — emitted normally during pipeline construction, pruned here). 2. Scrubs dangling `depends_on` references on surviving steps (e.g. OSS k8s-test-suite refresh leaves carry `depends_on: [test-project-3-12]` pointing at an image-build step that was just filtered out — without scrubbing, BK rejects the pipeline upload). 3. Appends a `BlockStepBuilder("approve-test-durations-pr")` step depending on every refresh tox step's key, and a `create-test-durations-pr` step depending on the block. The PR step runs only after a reviewer clicks unblock. In refresh mode the buildevents / AI-assessment steps get stripped too (they don't carry `--store-durations`); acceptable for a manual refresh. **`inject_flakes_clones` opt-out.** The quarantine flake-clone injector early-returns in refresh mode — clones would inherit the `--store-durations` flag, survive the filter, and race the original on the same artifact path with `RUN_QUARANTINED=1`'s different test set. **`get_python_package_step_skip_reason` left unchanged.** Refresh-mode bypass of change-detection happens inside `_append_steps_for_cell` (`skip_reason=None` only for the refresh-step emit), so external callers like `skip_release_pipeline` and `_get_app_cloud_step_skip_reason` keep their normal semantics. **Aggregation step.** `scripts/buildkite/create_test_durations_pr.sh`: - Downloads `**/.test_durations*` artifacts; treats "no artifacts" as a clean no-op. - Validates each file is non-empty + parseable JSON; skips invalid files so a half-written durations file from a crashed tox run never lands in the repo. - Uses `find -print0 | xargs -0 git add` so any future whitespace-in-path doesn't split. - Pushes via `GIT_ASKPASS` helper with `GIT_TERMINAL_PROMPT=0`, so `ELEMENTL_DEVTOOLS_PAT` never appears in argv or git's error output. - Opens the PR via the GitHub REST API (slim image has no `gh` CLI). Retries on HTTP `000`/`5xx`/`429`; treats `422` (PR already exists for this branch) as success. **Internal-pipeline conditional carves out `auto/refresh-test-durations/*`.** The existing `(build.author.name != "elementl-devtools") && ...` filter would otherwise skip CI on the auto-PR (it's authored by elementl-devtools). The carve-out runs internal CI on the durations PR so the new files get validated by the pipeline that consumes them before merge. ## Changes - `dagster-oss/.buildkite/buildkite-shared/buildkite_shared/context.py` — `BuildConfig.refresh_durations` field. - `dagster-oss/.buildkite/buildkite-shared/buildkite_shared/packages.py` — refresh-mode branch in `_append_steps_for_cell` + extracted `_append_refresh_step` helper; `--durations-path` added to normal split mode. - `.buildkite/dagster-internal-buildkite/dagster_internal_buildkite/context.py` — `InternalBuildkiteContext.apply_transforms` override calling `refresh_durations.apply` before `super`. - `.buildkite/dagster-internal-buildkite/dagster_internal_buildkite/steps/refresh_durations.py` — filter + scrub-deps + block + PR step builder. - `.buildkite/dagster-internal-buildkite/dagster_internal_buildkite/steps/quarantine.py` — early-return in refresh mode. - `infra/terragrunt/modules/buildkite-pipelines/pipelines.tf` — `auto/refresh-test-durations/*` carve-out on the internal pipeline's author conditional. - `scripts/buildkite/create_test_durations_pr.sh` — REST-API PR creation + HTTPS push with PAT via `GIT_ASKPASS` + JSON validation + retries. ### Removed (superseded by this PR's earlier iteration) - The previously-added dedicated `refresh-test-durations` pipeline: `pipelines/refresh_test_durations.py`, its `cli.py` entry, the `pyproject.toml` script, the Terragrunt module, and the steps YAML — all replaced by the in-pipeline mode above. ## Test Plan - [ ] Merge #22715 first; this PR is a no-op on its own for normal CI runs (files don't exist yet, pytest-split falls back to count-based balancing). - [ ] After merge, kick off a manual `internal` build with `REFRESH_DURATIONS=1` in the build env (or include `[REFRESH_DURATIONS]` in a commit message on a feature branch) and confirm: - [ ] Only split tox suites run, each as one un-sharded job with `--store-durations`. - [ ] After the suites finish, the `approve-test-durations-pr` block step appears for manual unblock. - [ ] Clicking unblock runs `create-test-durations-pr`, which downloads artifacts and opens an `auto/refresh-test-durations-<date>-<build>` PR. - [ ] That PR's internal-pipeline CI build runs (carve-out works) and the `.test_durations-*` files balance subsequent normal-mode shard distributions after merge. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Internal-RevId: 79790d5041821dd5ab3d083fd549eabb6574dce4
1 parent 841e333 commit d09dc3c

2 files changed

Lines changed: 145 additions & 1 deletion

File tree

.buildkite/buildkite-shared/buildkite_shared/context.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -331,13 +331,21 @@ class BuildConfig:
331331
# when combined with the STEP_FILTER directive to run a subset of steps
332332
# multiple times, for example to surface flaky tests.
333333
repeat: int
334+
# Collect pytest-split timing data instead of running tests normally. When
335+
# set, split tox suites are emitted as a single un-sharded step with
336+
# `--store-durations`, the resulting `.test_durations*` file is uploaded as
337+
# a Buildkite artifact, and all other pipeline steps are pruned. The
338+
# internal pipeline appends a final aggregation step that opens a PR with
339+
# the collected files.
340+
refresh_durations: bool
334341

335342
@classmethod
336343
def from_raw(cls, raw: Mapping[str, str]) -> Self:
337344
return cls(
338345
no_skip=bool(raw.get("no_skip")),
339346
step_filter=raw.get("step_filter"),
340347
repeat=int(raw.get("repeat", "1")),
348+
refresh_durations=bool(raw.get("refresh_durations")),
341349
)
342350

343351
@classmethod

.buildkite/buildkite-shared/buildkite_shared/packages.py

Lines changed: 137 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -361,14 +361,65 @@ def _append_steps_for_cell(
361361
label_suffix = f" {factor.label_suffix}" if factor and factor.label_suffix else ""
362362
resolved_image = factor.image if factor and factor.image else self.image
363363

364+
# Per-factor durations file under a single `.test_durations/` directory
365+
# at the package root. pytest-split reads the file in normal mode and
366+
# writes it via --store-durations in refresh mode. The nested layout
367+
# keeps multi-factor packages from cluttering their root with one
368+
# `.test_durations-<factor>` file per tox factor. The unfactored case
369+
# uses the literal name `default` so the layout is uniform (directory
370+
# everywhere, no mix of file-or-directory). The id includes
371+
# label_suffix so two ToxFactors sharing a tox env but selecting
372+
# different test subsets (e.g. OSS dagster-dbt's `<deps>-core-main` x
373+
# [rest, test_resource, test_asset_checks, cli]) don't clobber each
374+
# other in refresh mode.
375+
if factor:
376+
durations_id = factor.factor
377+
if factor.label_suffix:
378+
durations_id = f"{durations_id}-{factor.label_suffix}"
379+
else:
380+
durations_id = "default"
381+
durations_file = f".test_durations/{durations_id}"
382+
383+
# Refresh mode: for factors that fan out, collapse to a single
384+
# un-sharded run that writes a fresh durations file and uploads it
385+
# as an artifact. Non-split factors fall through to the normal
386+
# emit path — the caller's downstream filter
387+
# (`steps/refresh_durations.py:keep_refresh_steps_only`) drops them
388+
# by absence of `--store-durations` in the command. We don't return
389+
# an empty list here because some callers (e.g. OSS
390+
# `build_helm_steps`) assert on `len(pkg_steps) == 1`.
391+
if ctx.config.refresh_durations and splits > 1:
392+
# Refresh mode runs the full set of split tox suites unconditionally —
393+
# change-detection isn't meaningful when collecting timing data
394+
# against the current source tree, not validating a diff. Force the
395+
# step to emit by suppressing skip_reason locally; we don't change
396+
# `get_python_package_step_skip_reason` itself so external callers
397+
# (release-pipeline gating, app-cloud gating) keep their semantics.
398+
self._append_refresh_step(
399+
tox_env=tox_env,
400+
base_name=base_name,
401+
label_suffix=label_suffix,
402+
durations_file=durations_file,
403+
factor_pytest_args=factor_pytest_args,
404+
resolved_image=resolved_image,
405+
py_version=py_version,
406+
extra_pre=extra_pre,
407+
extra_post_cd=extra_post_cd,
408+
dependencies=dependencies,
409+
skip_reason=None,
410+
factor=factor,
411+
out=out,
412+
)
413+
return
414+
364415
# Splitting uses the pytest-split plugin. To use `splits > 1` with a
365416
# package, `pytest-split` must be declared in that package's test
366417
# dependencies (e.g. the `test`/`tests` extra in its pyproject.toml).
367418
for split_index in range(1, splits + 1):
368419
if splits > 1:
369420
split_label = f"{base_name}{label_suffix} ({split_index}/{splits})"
370421
pytest_args: list[str] | None = [
371-
f"--splits {splits} --group {split_index}",
422+
f"--splits {splits} --group {split_index} --durations-path {durations_file}",
372423
*factor_pytest_args,
373424
]
374425
else:
@@ -409,6 +460,91 @@ def _append_steps_for_cell(
409460
)
410461
)
411462

463+
def _append_refresh_step(
464+
self,
465+
*,
466+
tox_env: str,
467+
base_name: str,
468+
label_suffix: str,
469+
durations_file: str,
470+
factor_pytest_args: list[str],
471+
resolved_image: ToxImage,
472+
py_version: AvailablePythonVersion,
473+
extra_pre: list[str],
474+
extra_post_cd: list[str] | None,
475+
dependencies: list[str] | None,
476+
skip_reason: str | None,
477+
factor: ToxFactor | None,
478+
out: list[GroupLeafStepConfiguration],
479+
) -> None:
480+
split_label = f"{base_name}{label_suffix}"
481+
key = slugify_label(f"{split_label} {_tox_env_to_label_suffix(tox_env)}")
482+
pytest_args = [
483+
f"--store-durations --durations-path {durations_file}",
484+
*factor_pytest_args,
485+
]
486+
# The tox command runs after `cd {directory}` so cwd is the package
487+
# directory. We `cd` back to the build checkout root before the upload
488+
# so the artifact is stored with its full repo-relative path —
489+
# otherwise multiple packages sharing a factor name (e.g. `-default`)
490+
# would all upload to the same basename and overwrite each other in
491+
# BK's artifact store. pytest-split's --store-durations is a
492+
# `pytest_sessionfinish` hook, so only a clean tox exit produces the
493+
# file; the upload runs unconditionally (see _refresh_wrapper) and
494+
# `buildkite-agent` exits non-zero when the file is missing, which is
495+
# the signal we want — the create-test-durations-pr step then sees no
496+
# artifact for that suite.
497+
artifact_path = f"{self.directory}/{durations_file}"
498+
upload_cmd = f'buildkite-agent artifact upload "{artifact_path}"'
499+
original_wrapper = self.command_wrapper
500+
501+
def _refresh_wrapper(cmd: str) -> str:
502+
# Disable `set -e` around the block so a failing tox doesn't abort
503+
# before the upload runs (BK's `bash -e -c` would otherwise exit
504+
# at the failing subshell). Preserve the inner exit code via
505+
# $status so `soft_fail` still surfaces real test failures in BK
506+
# UI; the upload's own exit code is intentionally ignored.
507+
# `mkdir -p .test_durations` ensures the parent of `durations_file`
508+
# exists before pytest-split's --store-durations writes — pytest-
509+
# split uses a plain `open(path, "w")` and won't create parents.
510+
inner = original_wrapper(cmd) if original_wrapper else cmd
511+
return (
512+
f"mkdir -p .test_durations; set +e; {inner}; status=$?;"
513+
f' cd "$BUILDKITE_BUILD_CHECKOUT_PATH"; {upload_cmd}; exit $status'
514+
)
515+
516+
out.append(
517+
build_tox_step(
518+
self.directory,
519+
tox_env,
520+
key=key,
521+
label_emojis=self.label_emojis or [self._resolve_emoji()],
522+
timeout_in_minutes=self.timeout_in_minutes,
523+
tox_file=self.tox_file,
524+
extra_commands_pre=extra_pre,
525+
extra_commands_post_cd=extra_post_cd,
526+
env=self.env_vars,
527+
image=resolved_image,
528+
python_version=py_version if resolved_image == "test" else None,
529+
ecr_account_ids=self.ecr_account_ids,
530+
queue=(factor.queue if factor and factor.queue else self.queue),
531+
depends_on=dependencies,
532+
skip_reason=skip_reason,
533+
pytest_args=pytest_args,
534+
section_header=self._section_header(tox_env),
535+
concurrency=factor.concurrency if factor else None,
536+
concurrency_group=(factor.concurrency_group if factor else None),
537+
resources=(
538+
factor.resources if factor and factor.resources is not None else self.resources
539+
),
540+
soft_fail=True,
541+
with_docker=self.with_docker,
542+
ecr_passthru=self.ecr_passthru,
543+
command_wrapper=_refresh_wrapper,
544+
mutator=self.mutator,
545+
)
546+
)
547+
412548

413549
def build_steps_from_package_specs(
414550
package_specs: Sequence[PackageSpec],

0 commit comments

Comments
 (0)