style: apply ruff format to four unformatted data_processing files - #4525
style: apply ruff format to four unformatted data_processing files#4525dieterolson wants to merge 6 commits into
Conversation
`python -m ruff format --check .` has been failing on main, so every open PR inherits a red format gate regardless of what it changes. The four offending files are all under src/data_processing/data_processor/python/ and predate this branch. This is pure formatter output (`python -m ruff format` on exactly those four paths) with no hand edits. All four parse to an identical AST before and after, so there is no behavior change: performance_benchmark.py AST identical data_loader.py AST identical test_nn_training_worker.py AST identical test_vectorized_filter_engine_contracts.py AST identical Landing this separately unblocks the format gate for every open PR rather than burying the fix inside an unrelated feature branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Performance Benchmark ResultsNo benchmark results available. |
Performance Benchmark ResultsNo benchmark results available. |
Performance Benchmark ResultsNo benchmark results available. |
`tests (3.11)` is one of only two required status checks on `main`, and its runtime has grown into its own 90-minute cap. Measured across the last ten completed `CI Standard` runs: success 77.7 min <- the ONLY pass, at 86% of the budget cancelled 93.5 min <- timeout kill cancelled 91.8 min <- timeout kill cancelled 91.2 min <- timeout kill cancelled 90.5 min <- timeout kill cancelled 52.3 / 27.7 / 13.7 min failure 26.2 / 10.0 min One pass in ten. The runs at 90+ minutes record **zero failed steps** — they are timeout kills, not test failures, so a healthy run has roughly twelve minutes of headroom and any contention on a shared runner consumes it. Demonstrated concretely by PR #4525, a four-file formatting-only change whose reformat was proven AST-identical: its `tests (3.11)` was cancelled at 90.0 minutes, re-run, and cancelled again at 90.3 minutes. No change to that PR can make it pass. Combined with `strict = true` — which forces PRs to merge one at a time and re-run CI after every sibling merge — a ~10% pass rate on the required check makes a queue effectively unlandable without repeated luck. This is a STOPGAP, deliberately reversible, and it does not pretend to be the fix. 150 minutes is ~2x the observed successful run and ~1.6x the observed maximum, which is headroom rather than a new ceiling to grow into. The actual remedies are tracked in #4532: shard the suite across parallel jobs so the required context no longer has a single-point timeout, scope the required lane to the contract-critical set, and run `--durations` first to establish whether this is broad growth or a handful of slow modules. One known contributor is that `addopts` carries `-n auto --dist loadscope`, so Qt tests with 15-second `waitUntil` timeouts are load-sensitive and wall-clock varies with the runner. Worth noting what this is NOT: #4487 already cached the `tools_core` Rust wheel so the lane stops installing a Rust toolchain and rebuilding the wheel per job, and every measurement above is from AFTER that merged. The remaining runtime is the test suite itself, not the toolchain. Only `jobs.tests.timeout-minutes` changes. `quality-gate` (60) and `rust-quality-gate` (15) are untouched. Verified with the repo's own gates: `scripts/validate_workflows.py`, `scripts/check_workflow_pinning.py` and `scripts/check_blocking_quality_gates.py` all pass, and the tests job still parses to its full 27 steps. Part of #4532 Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Performance Benchmark ResultsNo benchmark results available. |
…erial everywhere) (#4548) `tests (3.11)` is one of two required checks and it has been timing out. Raising its cap from 90 to 150 minutes (#4539) did NOT help — the next run consumed 150.3 minutes. That ruled out "slightly too slow" and pointed at the real cause. Per-step timings from the cancelled job on PR #4525 (a four-file, formatting-only change whose reformat was proven AST-identical): step 23 Run Tests with Coverage (Python 3.11) 147.5 min CANCELLED everything else combined ~4 min So the test step is effectively the whole job, and it runs with `-n 0` — fully serial. That override is unconditional, and its stated reason is specific to one runner class: "the memory-constrained local runner fleet has repeatedly crashed xdist workers even at low fan-out". That reason does not hold on a hosted runner. The failing job ran on `GitHub Actions 1000355575` — 4 vCPU / 16 GB, not memory-constrained. And the timings across recent runs line up exactly with runner class rather than with any code change: 77.7 min SUCCESS d-sorg-local-Oglaptop-1 (fleet hardware, fast cores) 150.3 min CANCELLED hosted (slower per-core, same serial run) 93.5 min CANCELLED d-sorg-local-Desktop-3 91.2 min CANCELLED d-sorg-local-Desktop-6 Forcing serial execution onto slower hosted cores is what pushes the job past any cap. So the fan-out is now chosen per runner class instead of globally: PYTEST_FANOUT: ${{ needs.pick-runner.outputs.runner == 'd-sorg-fleet' && '0' || 'auto' }} pytest_args+=(-n "${PYTEST_FANOUT:-0}") Fleet hosts keep `-n 0`, so the crash this override was written to prevent is still prevented. Hosted runners get `-n auto` (4 workers), which should take the test step from ~148 minutes to roughly 40. Two details that make this safer than it looks: - `--dist loadscope` comes from the repo's own addopts and is unchanged, so a module's tests stay on one worker. That matters because the Qt tests use 15 second `waitUntil` timeouts and are load-sensitive; loadscope keeps them from being split across workers. - The default is `0`, not `auto` (`${PYTEST_FANOUT:-0}`). If the variable is ever unset the behaviour is today's serial run, so this fails safe. This follows the conditional the tests job already uses for `PIP_CACHE_DIR`, which switches on the same `needs.pick-runner.outputs.runner == 'd-sorg-fleet'` test. The 150-minute cap from #4539 is left in place as headroom rather than reverted; with parallelism it should be far from binding. If it stops being needed it is a one-line revert. Verified: `scripts/validate_workflows.py`, `scripts/check_workflow_pinning.py` and `scripts/check_blocking_quality_gates.py` all pass; the tests job still parses to 27 steps; and the env var is on the `tests` job and NOT on `quality-gate` (both jobs carry an identical `PIP_DEFAULT_TIMEOUT: "120"` line, so a first-match patch lands in the wrong job — worth knowing when editing this file). Part of #4532 Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
Closing as superseded — this PR is now empty against Its four files were reformatted on Verified before closing:
So the repo-wide condition this PR was opened to fix — every open PR inheriting a red Worth recording what it turned up on the way, since that outlasts the PR: Its #4548 fixed that by choosing the xdist fan-out per runner class, keeping Remaining CI work is tracked in #4532: shard the suite so the required context has no single-point timeout, scope the required lane to the contract-critical set, and run No code is lost by closing this: |
Pull request was closed
Anti-Phantom-Merge Guard — Rule 1 (empty diff) failedThis PR has 0 changed files against the base branch. Empty PRs are not mergeable. If you intentionally want an empty PR, set the title to exactly How to fix
Escape hatchA repo admin can add the label Workflow: |
1 participant
Why
python -m ruff format --check .is currently failing onmain, so every open PR inherits a red format gate regardless of what it changes:All four predate this branch and are unrelated to any feature work. Landing the fix on its own unblocks the format gate for every open PR rather than burying it inside an unrelated feature branch.
What
Pure formatter output —
python -m ruff formatrun on exactly those four paths, with no hand edits and nothing else touched.Verification
Both gates green after the change:
No behavior change. Each file parses to a byte-identical AST before and after (compared against
origin/main, decoding both sides from raw bytes so the comparison is encoding-safe):performance_benchmark.pydata_loader.pytest_nn_training_worker.pytest_vectorized_filter_engine_contracts.pyThe only substantive reflow is ruff moving
assertmessages out of a wrapped parenthesized condition, e.g.Non-ASCII characters are preserved exactly (em-dash counts unchanged, zero U+FFFD introduced).
Note on
spec-checkThis PR touches
src/**, which trips thespec-checkfreshness gate. Since the change is provably behavior-neutral (identical ASTs) and documents nothing, it carries thespec-exemptlabel rather than a spuriousSPEC.mdchangelog row — matching that workflow's own stated rationale for the exemption.