fix(sdk): recognize BenchStatus phase=Skipped as terminal (closes #480)#481
Merged
Conversation
The SDK's `_BENCH_TERMINAL_PHASES` frozenset omitted
`BENCH_PHASE_SKIPPED` even though the constant was defined. As a
result `BenchStatus(phase="Skipped").is_terminal` returned False,
and `wait_until_bench_complete` polled until the user-supplied
timeout and then raised `TimeoutError` whose message contained
`(phase=Skipped)` -- proving the SDK had the terminal data but
did not act on it.
The operator (basilica-backend X2 fix, basilica-backend#650 /
basilica-backend#653) emits a terminal `BenchStatus{phase=Skipped,
lastAttemptOutcome="skipped", lastAttemptAt=...}` plus a
`BenchSkipped` Event when workers exit before the bench-controller
observes them. The SDK must recognise this as terminal.
This is a multi-file Skipped-as-terminal alignment, not a one-line
frozenset edit:
- `_BENCH_TERMINAL_PHASES` adds `BENCH_PHASE_SKIPPED`; existing
`BenchStatus.is_terminal` picks the change up by construction.
- `BenchStatus` gains `is_successful` / `is_failed` / `is_skipped`
properties. These pin the semantic that Skipped is terminal but
neither success nor failure -- the probe was not run; the workload
itself may have completed cleanly.
- `wait_until_bench_complete` and `wait_until_bench_complete_async`
docstrings now enumerate all four terminal phases (Succeeded,
Failed, TimedOut, Skipped) and reference the example consumers.
- `examples/20_distributed_diloco.py` and
`examples/22_distributed_with_bench.py` already branch on
`phase != "Succeeded"`, so the natural Skipped path is the
phase-aware print + clean exit. No example refactor required.
- 18 new tests in `tests/test_bench_status_skipped.py` cover the
frozenset membership, BenchStatus property matrix across all
six phases, `from_status_dict` round-trip of the operator's
Skipped shape, sync + async waiter return, and the top-level
`BENCH_PHASE_SKIPPED` export check.
- CHANGELOG.md, pyproject.toml, Cargo.toml, Cargo.lock bumped to
0.29.4.
Cross-repo reference: `one-covenant/basilica-backend#419` Stage 4
take-5 Cell B and `one-covenant/basilica-backend#650 / #653`.
Contributor
WalkthroughThis PR extends ChangesBenchStatus Skipped phase handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/basilica-sdk-python/CHANGELOG.md`:
- Around line 10-35: The CHANGELOG footer reference block is stale and missing
entries for "Unreleased" and "0.29.4", causing the new "## [0.29.4] -
2026-05-18" heading to not resolve; update the reference-link block at the
bottom of CHANGELOG.md to add or refresh the compare links for "Unreleased"
(pointing from the latest tag to HEAD) and "0.29.4" (pointing from the previous
release, e.g. 0.29.3, to 0.29.4) so the bracketed headings resolve
correctly—look for the existing footer reference section in CHANGELOG.md and add
entries matching the style of the other links for the "Unreleased" and "0.29.4"
labels.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1e74507-c096-4807-91f3-f4eaa4b38a18
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/basilica-sdk-python/CHANGELOG.mdcrates/basilica-sdk-python/Cargo.tomlcrates/basilica-sdk-python/pyproject.tomlcrates/basilica-sdk-python/python/basilica/distributed.pycrates/basilica-sdk-python/tests/test_bench_status_skipped.py
Comment on lines
+10
to
+35
| ## [0.29.4] - 2026-05-18 | ||
|
|
||
| ### Fixed | ||
| - `BenchStatus` recognises `phase=Skipped` as a terminal state. | ||
| `_BENCH_TERMINAL_PHASES` now contains all four operator-side terminal | ||
| phases (`Succeeded`, `Failed`, `TimedOut`, `Skipped`), so | ||
| `BenchStatus.is_terminal` returns `True` on `Skipped` and | ||
| `wait_until_bench_complete` / `wait_until_bench_complete_async` return | ||
| the terminal `BenchStatus` instead of polling until the user-supplied | ||
| timeout and raising `TimeoutError`. Pre-fix, the SDK had the data | ||
| (the operator wrote terminal `BenchStatus{phase=Skipped, | ||
| lastAttemptOutcome="skipped"}` to the UD CR) but did not act on it | ||
| -- the `TimeoutError` message literally contained `(phase=Skipped)`. | ||
| Closes #480. Cross-repo reference: | ||
| `one-covenant/basilica-backend#419` Stage 4 take-5 Cell B and the | ||
| basilica-backend operator X2 fix (`one-covenant/basilica-backend#650 | ||
| / #653`). | ||
|
|
||
| ### Added | ||
| - `BenchStatus.is_successful` / `is_failed` / `is_skipped` properties. | ||
| Pin the semantic that `Skipped` is terminal but neither success nor | ||
| failure -- the bench probe was not run (e.g. workers exited before | ||
| the bench-controller observed them). The workload's own exit codes | ||
| remain the source of truth for run success; bench is an opt-in, | ||
| best-effort measurement. | ||
|
|
Contributor
There was a problem hiding this comment.
Add/refresh changelog link references for 0.29.4 and Unreleased compare chain.
The new ## [0.29.4] heading is bracketed, but this file’s reference-link block is stale and does not include 0.29.4 (and Unreleased still compares from 0.14.0). Please update the footer references so version headings resolve correctly.
Suggested footer update
-[Unreleased]: https://github.com/one-covenant/basilica/compare/basilica-sdk-python-v0.14.0...HEAD
+[Unreleased]: https://github.com/one-covenant/basilica/compare/basilica-sdk-python-v0.29.4...HEAD
+[0.29.4]: https://github.com/one-covenant/basilica/compare/basilica-sdk-python-v0.29.3...basilica-sdk-python-v0.29.4
+[0.29.3]: https://github.com/one-covenant/basilica/compare/basilica-sdk-python-v0.29.2...basilica-sdk-python-v0.29.3🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/basilica-sdk-python/CHANGELOG.md` around lines 10 - 35, The CHANGELOG
footer reference block is stale and missing entries for "Unreleased" and
"0.29.4", causing the new "## [0.29.4] - 2026-05-18" heading to not resolve;
update the reference-link block at the bottom of CHANGELOG.md to add or refresh
the compare links for "Unreleased" (pointing from the latest tag to HEAD) and
"0.29.4" (pointing from the previous release, e.g. 0.29.3, to 0.29.4) so the
bracketed headings resolve correctly—look for the existing footer reference
section in CHANGELOG.md and add entries matching the style of the other links
for the "Unreleased" and "0.29.4" labels.
5 tasks
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Closes #480.
crates/basilica-sdk-python/python/basilica/distributed.pydefinedBENCH_PHASE_SKIPPED = "Skipped"(line 210) but omitted it from_BENCH_TERMINAL_PHASES(lines 217-219). As a resultBenchStatus(phase="Skipped").is_terminalreturnedFalse, andwait_until_bench_completepolled until the user-supplied timeout, then raisedTimeoutErrorwhose message contained(phase=Skipped)— proving the SDK already had the terminal data but did not act on it.The basilica-backend operator (X2 fix,
one-covenant/basilica-backend#650 / #653) emits a terminalBenchStatus{phase=Skipped, lastAttemptOutcome="skipped", lastAttemptAt=...}plus aBenchSkippedEvent when workers exit before the bench-controller observes them. The SDK must recognise this as terminal.This is a multi-file Skipped-as-terminal alignment, not a one-line frozenset edit.
Surface changes
python/basilica/distributed.py_BENCH_TERMINAL_PHASESaddsBENCH_PHASE_SKIPPED; the existingBenchStatus.is_terminalproperty picks the change up by construction.BenchStatusgains three explicit semantic properties:is_successful—Trueiffphase == Succeeded.is_failed—Trueiffphase in {Failed, TimedOut}. Skipped is NOT a failure.is_skipped—Trueiffphase == Skipped(terminal-but-not-run).These pin the semantic that Skipped is terminal but neither success nor failure: the probe was not run; the workload itself may have completed cleanly.
wait_until_bench_completeandwait_until_bench_complete_asyncdocstrings now enumerate all four terminal phases (Succeeded, Failed, TimedOut, Skipped) and reference the example consumers.examples/examples/20_distributed_diloco.pyandexamples/22_distributed_with_bench.pyalready branch onphase != "Succeeded", so the natural Skipped path is the existing phase-awareprint(...)+ clean return. No example refactor required; the existing else-branch now handles the operator's Skipped phase end-to-end once the waiter returns instead of raising.tests/test_bench_status_skipped.py(new)18 new tests covering:
Skippedin_BENCH_TERMINAL_PHASES;Pending/Runningexcluded; full four-phase composition.BENCH_PHASE_SKIPPEDimportable from the top-levelbasilicapackage and consistent with the per-module constant.BenchStatus(phase=...)property matrix across all six phases (Succeeded, Failed, TimedOut, Skipped, Pending, Running) — pinsis_terminal/is_successful/is_failed/is_skipped.BenchStatus.from_status_dictround-trip of the operator'sSkippedJSON shape (mode, phase, message, lastAttemptAt, lastAttemptOutcome).wait_until_bench_completereturns theSkippedBenchStatus without raising.wait_until_bench_complete_asynchonours the same contract.Version bump
CHANGELOG.md:[0.29.4] - 2026-05-18entry underFixed+Added.pyproject.toml:0.29.3->0.29.4.Cargo.toml:0.29.3->0.29.4.Cargo.lock: regenerated.Test plan
python -m pytest crates/basilica-sdk-python/tests/ -v --ignore=tests/test_dns_propagation_e2e.py-- 157 passed (18 new + 139 pre-existing). Thetest_dns_propagation_e2e.pycollection error is pre-existing (httpxnot in venv) and unrelated.BenchStatus(phase="Skipped").is_terminal == True;is_skipped == True;is_successful == False;is_failed == False.wait_until_bench_completeagainst a fake client emittingphase=Skippedreturns the BenchStatus rather than raising.test-python-sdkmatrix (3.10 / 3.11 / 3.12 / 3.13).Cargo.toml == pyproject.toml).examples/20_distributed_diloco.pyagainstapi.basilica.aionce this lands; expect exit 0 with phase-aware Skipped message instead of the priorTimeoutError.Cross-ref
one-covenant/basilica-backend#419Stage 4 take-5 Cell B captured the runtime artefact that surfaced this. Operator-side X2 fix isone-covenant/basilica-backend#650 / #653.Cross-ref: one-covenant/basilica-backend#419
Summary by CodeRabbit
Bug Fixes
BenchStatusto treatSkippedphase as terminal, preventing timeouts in wait operations.New Features
is_successful,is_failed, andis_skippedboolean properties toBenchStatusfor improved phase checking.Tests
Skippedbench status handling.Chores