feat(sdk): collapse bench API to bool opt-in + lazy training.bench (fixes one-covenant/basilica-backend#661)#483
Merged
Conversation
…efs basilica-backend#661)
Part of the SDK API simplification plan
(docs/plans/SDK-API-SIMPLIFICATION-PLAN.md on basilica-backend main).
The bench API surface was over-exposed for an opt-in diagnostic helper:
- bench: str modes ("on-start" / "off") -- two string tokens for a binary
- training.wait_until_bench_complete(timeout=...) raises or returns
- BenchStatus with four terminal phases + four is_* properties
- Two access paths to the result (bench_status.result vs training.bench)
Target after S2 (this change):
- bench: bool -- True opts in, False opts out (default)
- training.bench: BenchResult | None (unchanged; lazy)
- training.bench_diagnostics: Optional[Dict[str, Any]] (new) -- small
debug dict with phase / message / timings, for the rare case where
the user wants to know WHY a probe didn't measure
- bench: str ("on-start" / "off") still accepted with DeprecationWarning
- wait_until_bench_complete[_async] and bench_status emit
DeprecationWarning pointing at the lazy training.bench accessor
- Removed in next major
Changes:
- python/basilica/__init__.py: _normalize_bench_param helper; bench param
type Union[bool, str] with default False on deploy_distributed,
deploy_distributed_async, deploy_distributed_managed,
deploy_distributed_managed_async; deprecation warning emitted by helper
- python/basilica/decorators.py: @distributed bench param type
Union[bool, str] with default False (forwarded verbatim, normalized
downstream)
- python/basilica/distributed.py: new training.bench_diagnostics lazy
property; bench_status, wait_until_bench_complete[_async] emit
DeprecationWarning; internal _bench_status_no_warn reads the
BenchStatus without warning
- tests/test_bench_bool_simplification.py: 22 tests pinning the new
surface (bool acceptance, str deprecation, diagnostics dict shape,
lazy bench BenchResult|None semantics, wait_until_bench_complete +
bench_status deprecation warnings)
- pyproject.toml + Cargo.toml + Cargo.lock: bump 0.29.4 -> 0.29.5
All 179 existing SDK tests pass; new tests bring total to 201.
Wire contract is unchanged: distributed.bench.mode is still
"on-start" / "off" on the operator-facing JSON. Only the user-facing
SDK parameter type narrows. Operator + CRD schema untouched.
Contributor
WalkthroughThis PR refactors the ChangesBench API bool simplification and deprecation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/basilica-sdk-python/python/basilica/__init__.py (1)
1432-1434:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid deprecated waiters inside the supported
wait_for_benchpath.These helpers currently call
wait_until_bench_complete[_async](), sodeploy_distributed(..., wait_for_bench="best_effort"|"required")now emits aDeprecationWarningeven though the caller did not use a deprecated API. That is misleading, and it will also break consumers that run with warnings-as-errors. Please switch these helpers to a non-warning internal polling path instead of routing through the deprecated public waiter.Also applies to: 2540-2542
🤖 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/python/basilica/distributed.py`:
- Around line 541-551: The internal accessor _bench_status_no_warn should treat
a published bench block with "mode": "off" as no bench; after retrieving
bench_raw in _bench_status_no_warn, check if bench_raw.get("mode") == "off" (or
equivalent string) and return None instead of calling
BenchStatus.from_status_dict, so callers like bench_status and
wait_until_bench_complete[_async] see None when benching was opted out; keep the
existing behavior for other modes and only collapse the "off" case to None.
🪄 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: cae08adb-47f3-4a14-a049-bca86296536b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/basilica-sdk-python/Cargo.tomlcrates/basilica-sdk-python/pyproject.tomlcrates/basilica-sdk-python/python/basilica/__init__.pycrates/basilica-sdk-python/python/basilica/decorators.pycrates/basilica-sdk-python/python/basilica/distributed.pycrates/basilica-sdk-python/tests/test_bench_bool_simplification.py
Comment on lines
+541
to
551
| def _bench_status_no_warn(self) -> Optional[BenchStatus]: | ||
| """Internal: read the full BenchStatus without emitting the | ||
| deprecation warning. Used by ``bench_diagnostics`` and the | ||
| legacy ``wait_until_bench_complete`` waiter so they remain | ||
| callable without double-warning the user.""" | ||
| if self._cached_status is None: | ||
| self.refresh() | ||
| bench_raw = (self._cached_status or {}).get("distributed", {}).get("bench") | ||
| if not bench_raw: | ||
| return None | ||
| return BenchStatus.from_status_dict(bench_raw) |
Contributor
There was a problem hiding this comment.
Collapse mode="off" to None in the internal bench accessor.
The operator can still publish a bookkeeping bench block when the user opted out, e.g. {"mode": "off", "phase": "Skipped"}. Returning a BenchStatus here breaks the documented None-on-bench-off behavior for bench_status and wait_until_bench_complete[_async], and it can make wait_for_bench="required" fail even though bench was explicitly disabled.
💡 Suggested fix
def _bench_status_no_warn(self) -> Optional[BenchStatus]:
"""Internal: read the full BenchStatus without emitting the
deprecation warning. Used by ``bench_diagnostics`` and the
legacy ``wait_until_bench_complete`` waiter so they remain
callable without double-warning the user."""
if self._cached_status is None:
self.refresh()
bench_raw = (self._cached_status or {}).get("distributed", {}).get("bench")
if not bench_raw:
return None
+ if bench_raw.get("mode", "off") != "on-start":
+ return None
return BenchStatus.from_status_dict(bench_raw)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _bench_status_no_warn(self) -> Optional[BenchStatus]: | |
| """Internal: read the full BenchStatus without emitting the | |
| deprecation warning. Used by ``bench_diagnostics`` and the | |
| legacy ``wait_until_bench_complete`` waiter so they remain | |
| callable without double-warning the user.""" | |
| if self._cached_status is None: | |
| self.refresh() | |
| bench_raw = (self._cached_status or {}).get("distributed", {}).get("bench") | |
| if not bench_raw: | |
| return None | |
| return BenchStatus.from_status_dict(bench_raw) | |
| def _bench_status_no_warn(self) -> Optional[BenchStatus]: | |
| """Internal: read the full BenchStatus without emitting the | |
| deprecation warning. Used by ``bench_diagnostics`` and the | |
| legacy ``wait_until_bench_complete`` waiter so they remain | |
| callable without double-warning the user.""" | |
| if self._cached_status is None: | |
| self.refresh() | |
| bench_raw = (self._cached_status or {}).get("distributed", {}).get("bench") | |
| if not bench_raw: | |
| return None | |
| if bench_raw.get("mode", "off") != "on-start": | |
| return None | |
| return BenchStatus.from_status_dict(bench_raw) |
🤖 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/python/basilica/distributed.py` around lines 541 -
551, The internal accessor _bench_status_no_warn should treat a published bench
block with "mode": "off" as no bench; after retrieving bench_raw in
_bench_status_no_warn, check if bench_raw.get("mode") == "off" (or equivalent
string) and return None instead of calling BenchStatus.from_status_dict, so
callers like bench_status and wait_until_bench_complete[_async] see None when
benching was opted out; keep the existing behavior for other modes and only
collapse the "off" case to None.
This was referenced May 18, 2026
test(sdk): filter bench-related deprecations in S2 acceptance tests (refs basilica-backend#661)
#485
Merged
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
boolopt-in (bench=True/bench=False, defaultFalse).training.bench_diagnostics: Optional[Dict[str, Any]]debug surface alongside the existing lazytraining.bench: BenchResult | None.bench: strmodes ("on-start"/"off"),training.bench_status, andtraining.wait_until_bench_complete[_async]withDeprecationWarningpointing at the new surface. Legacy paths remain functional for two minor versions.basilica-sdk-pythonfrom0.29.4to0.29.5(S2 is a user-facing API surface change per the plan).Why
Per
docs/plans/SDK-API-SIMPLIFICATION-PLAN.mdonbasilica-backendmain(Problem 3): the bench API exposed too much state for an opt-in diagnostic helper:bench: strmodes ("on-start"/"off") — two string tokens to memorize for a binary opt-intraining.wait_until_bench_complete(timeout=...)— raisesTimeoutErroror returns a four-phaseBenchStatusBenchStatuswith four terminal phases (Succeeded/Failed/TimedOut/Skipped) + fouris_*propertiesbench_status.resultvstraining.bench)For an OPT-IN measurement helper, that is six concepts for a yes/no question. The user wants to know whether the probe measured (
training.bench is not None), not which of four terminal phases it landed in. Diagnostic detail moves to the rarely-neededtraining.bench_diagnosticsaccessor.Surface after this PR
Changes
python/basilica/__init__.py: new_normalize_bench_paramhelper;bench: Union[bool, str](defaultFalse) ondeploy_distributed,deploy_distributed_async,deploy_distributed_managed,deploy_distributed_managed_async. Legacy str values emitDeprecationWarningand forward the wire token verbatim.python/basilica/decorators.py:@distributedbenchparam type narrowed toUnion[bool, str](defaultFalse); normalization happens downstream indeploy_distributed.python/basilica/distributed.py:DistributedTraining.bench_diagnosticsreturningOptional[Dict[str, Any]]withphase/message/mode/started_at/completed_at/last_attempt_at/last_attempt_outcome. ReturnsNonewhen bench wasn't requested (mode != on-start) OR no operator status block.DistributedTraining.bench_status,wait_until_bench_complete,wait_until_bench_complete_asyncnow emitDeprecationWarningpointing attraining.bench/training.bench_diagnostics. Internal_bench_status_no_warnkeeps the waiters readable without double-warning.tests/test_bench_bool_simplification.py: 22 new tests pinning the post-fix shape (bool acceptance, str deprecation, diagnostics dict shape, lazybench: BenchResult | Nonesemantics, deprecation warnings on the legacy surface).0.29.4->0.29.5acrosspyproject.toml/Cargo.toml/Cargo.lock.Wire contract
Unchanged.
distributed.bench.modeon the operator-facing JSON stays"on-start"/"off". Only the user-facing SDK parameter type narrows. Operator + CRD schema untouched — this is a pure SDK ergonomics change.Test plan
pytest tests/test_bench_bool_simplification.py— 22 tests assert the post-fix shape; pre-fix run produced 15 failures (anti-pattern reproduced) before the implementation landedpytest tests/ --ignore=tests/test_dns_propagation_e2e.py— 179 existing + new = 201 total, all passingpytest tests/test_bench_status_skipped.py— sibling bench tests still green (theSkipped-terminal-phase fix from fix(sdk): recognize BenchStatus phase=Skipped as terminal (closes #480) #481 remains intact)cargo fmt --all -- --checkcleancargo check -p basilica-sdk-pythoncleanMigration impact
Existing users on
bench="on-start"/bench="off"continue to work and see aDeprecationWarningpointing at the bool form. Existing users ofwait_until_bench_complete/bench_statuscontinue to work and see aDeprecationWarningpointing attraining.bench/training.bench_diagnostics. No breaking change in this release; deprecated paths removed at the next major.Cross-ref: follow-on work after basilica-backend issue 419. See SDK simplification plan for the broader S1-S7 ticket map (S1
Trainingcontext-manager-able, S3command=factory, S4source=Callable-only, S5 example migration, S6 docstrings, S7 major bump).Summary by CodeRabbit
Version 0.29.5 Release
New Features
benchparameter for distributed training methods now accepts boolean values (True/False) for a simplified API.bench_diagnosticsproperty to access benchmark debug information.benchlazy result property for retrieving benchmark outcomes.Deprecations
benchparameter values are deprecated; use boolean values instead.wait_until_bench_complete()andwait_until_bench_complete_async()methods are deprecated in favor of the newbenchproperty.bench_statusproperty is deprecated; usebenchorbench_diagnosticsinstead.