feat(optimization): run-scoped usage, budget, and cancellation contracts (RFC #6357, PR 1/2)#6358
feat(optimization): run-scoped usage, budget, and cancellation contracts (RFC #6357, PR 1/2)#6358caohy1988 wants to merge 5 commits into
Conversation
…ntracts (OptimizationRunContext, OptimizerCapabilities, terminal_status) RFC: google#6357. PR 1 of 2 (contracts only; no optimizer behavior change). Adds the one-shot OptimizationRunContext ledger with truthful usage coverage (verified/partial/unreported, never zero-coerced), call/token budgets with commit-then-terminate overshoot semantics and configurable on_budget_exceeded (raise | return_partial), cooperative cancellation, conservative OptimizerCapabilities defaults on AgentOptimizer, an optional keyword-only run_context parameter on optimize(), and an experimental terminal_status field on OptimizerResult. Enforcement in the two prompt optimizers lands in the stacked PR 2.
|
Verification update: after installing the missing local test dependencies ( |
Code Review ResultsScope: google/adk-python PR #6358, Triage Groups
P1 -- High
P2 -- Moderate
P3 -- Low
Verification
Testing gaps
Actionable Findings
|
…e token compliance, granular capabilities, OptimizerResult unchanged Per the precision pass on RFC google#6357: logical-call limits are the only hard enforcement (atomic slot admission); reported-token ceilings terminate reactively over authoritative totals with within_limit/exceeded/indeterminate compliance (missing totals are never proof of compliance); call status is split from run status; capabilities gain accepts_run_context and split logical-call vs reported-token enforceability; terminal_status is removed from OptimizerResult -- the caller-owned snapshot is authoritative; adds OptimizationProviderError for governed in-band error termination.
Fresh review of the contracts PRReviewed at Verdict: keep this PR in draft / request changes. The revised direction is right, but the run context is not yet an authoritative, immutable terminal record. Several earlier load-bearing findings remain present. What is now right
[P1] Terminal lifecycle is incompleteThe context has terminal status values but no complete terminal state machine:
The fix should be an explicit, lock-protected terminal transition API: completed / budget exceeded / cancelled / failed, exactly once, with first-terminal-wins semantics. [P1] Provider failure is only a call annotation, not a governed terminal outcome
Today, usage that crosses the token limit causes Please make terminal provider commit atomic and structured. The public typed error should expose the sanitized code and final snapshot; raw provider/request text should not be copied into the ledger. [P1] The concurrency-safe, one-shot ledger can be corrupted or mutated
Bind every handle to a private context identity, validate ownership under the lock, make close atomic, replace the attachment sentinel with an explicit boolean/private sentinel, copy/freeze budgets at construction, and return frozen snapshots with tuples/frozen event models. [P2] Contract precision and independent landability
Verification
The passing suite validates the positive accounting path, but it does not currently exercise the adversarial invariants above. Please add deterministic tests for every terminal transition, terminal precedence, cross-context handles, concurrent double-close, post-terminal admission, immutable budgets/snapshots, and native cancellation before treating the contract PR as complete. This is a good API direction, but the ledger is the governance boundary. Its lifecycle and integrity guarantees need to be true in code, not only in the RFC prose. |
…en ledger, provider-failure precedence, independent type-cleanliness - Explicit first-terminal-wins state machine (completed/budget_exceeded/ cancelled/failed) with finalize_success/finalize_cancelled/finalize_failed; begin_model_call rejects any terminal context; a late cancellation cannot overwrite an earlier terminal state. - Provider failure is a governed terminal: end_model_call with sanitized error_code/error_type commits usage, transitions the run to failed, and raises OptimizationProviderError; provider failure takes precedence over a simultaneous token overshoot while preserving usage and token-compliance evidence. Raw exception text never enters the ledger. - Ledger integrity: handles bound to their context (foreign commit is a typed error), atomic close under the context lock (concurrent double-close commits once), private unattached sentinel (attach(None) is one-shot), frozen OptimizationBudgets (ge=0 validated), frozen snapshots/events (tuples), snapshot carries budgets + terminal sequence + terminal error metadata; finalize_cancelled closes open call events. - Stage is an extensible string with documented constants. - Built-in optimizers accept the keyword and reject a supplied context with UnsupportedOptimizationContextError in this PR, so the abstract-signature change is independently type-clean; real support lands in the stacked enforcement PR. - isort + pyink applied.
Review disposition — all three P1s and the P2s implemented (head
|
Fresh adversarial review of the hardened contracts headReviewed at Verdict: keep draft / request changes. The rewrite genuinely fixes the previous attachment, handle ownership, immutability, extensible-stage, terminal-admission, and provider-precedence findings. The governance boundary still has two load-bearing correctness gaps plus several smaller contract/gate issues. Verified improvements
[P1] The metadata boundary is not normalized or exception-safe
The commit order also makes invalid usage non-atomic: the record is marked closed and receives This becomes a live failure in #6359: a raised provider exception with numeric Fix: normalize/cap error identifiers at this boundary, validate/extract usage into local values before mutating the record, and perform the complete commit only after conversion succeeds. Invalid provider metadata should terminate with a defined sanitized failure or explicitly degrade coverage to indeterminate, never partially mutate the ledger. [P1] First-terminal-wins is incomplete for pending cancellation and already-in-flight calls
There is a second concurrency hole in Fix: success finalization must atomically observe pending cancellation and require no open calls. An already-in-flight call may settle and commit after another call wins the terminal race, but its caller must receive the first terminal outcome before returning to downstream work. [P2] Failed-run typing conflates provider and non-provider failures
Either add a generic typed run-failure/finalized outcome or retain terminal error kind/source so only provider failures become [P2] Cancelled-call counters are internally inconsistent
If this counter means terminal/settled calls, increment it when cancellation closes an event. If it means successful/provider-settled calls only, rename and document it so downstream reconciliation does not infer missing calls. [P2] Persisted
|
Fresh review round — contracts PR (
|
…pes (round-2 adversarial findings) - Validate-before-commit: usage counters must be finite non-negative numbers (NaN/inf/negative -> not reported, never a crash on a half-closed record); error metadata is normalized to bounded single-token identifiers at the context boundary (numeric provider codes become strings; multiline/path text is scrubbed); the event, counters, and terminal transition then commit atomically under one lock acquisition. - Every caller observes the committed terminal: an already-admitted call settles for truthful accounting and then receives the existing terminal error; finalize_success observes pending cancellation and commits/raises CANCELLED instead of recording a false success. - OptimizationFailedError added for non-provider failures; OptimizationProviderError is reserved for governed provider-call terminals (terminal provenance tracked). - end_model_call(cancelled=True) settles a call as cancelled while preserving usage evidence; completed_calls documented and counted as settled calls regardless of call status (finalize_cancelled counts its closures). - Event timestamps are wall-clock epoch seconds for durable, cross-worker attempt records. - Built-in optimize() signatures fully typed (Optional[OptimizationRunContext]). - 10 new regression tests reproduce every round-2 finding. Suite: 70/70.
Round-2 disposition — all three P1s and both clarifications implemented (head
|
Round-3 fresh review — contracts head
|
…zation, local-abort settlement, private module packaging, typed internals - finalize_success is an invariant boundary: idempotent only on COMPLETED; any other existing terminal re-raises its typed outcome; a pending cancellation commits CANCELLED and raises; an open (unsettled) call commits FAILED (OPEN_MODEL_CALLS) and raises OptimizationFailedError so a run can never escape nonterminal with an open event. - returned_model_version is normalized (bounded optional string) before mutation -- a malformed value from a custom BaseLlm can no longer make every subsequent snapshot raise ValidationError. - Fractional token counters are unreported, not silently truncated (ints excluding bool; floats only when integral); zero and very large integers accepted -- tested. - abort_model_call settles an admitted call that failed locally as call-status ABORTED with run-status FAILED via OptimizationFailedError: truthfully distinct from provider failures and cancellation (needed by the enforcement PR's post-admission exception boundary). - ADK visibility rule: module renamed to _run_context.py; the supported experimental surface is exported from google.adk.optimization with explicit __all__; internal imports and tests use the package surface. - Typed _CallRecord/_CallHandle constructors and attributes; optional .value access narrowed (the five repo-config mypy errors named in review; exact gate reproduction is blocked in this environment by a numpy-stub / python_version=3.11 syntax failure, noted honestly). - New tests: success-with-open-call, success-after-budget/provider/cancel, malformed model version, fractional/integral/zero/large usage, local abort, concurrent attach (1 of 16 wins), concurrent admission (exactly k of 16, unique sequences, shared terminal), success-vs-settlement race. Suite: 54 contract tests, full optimization directory green.
RFC: #6357 — please review the RFC before this code. PR 1 of the two-PR structure: contracts only, zero optimizer behavior change. PR 2 (#6359, stacked) instruments
SimplePromptOptimizerandGEPARootAgentPromptOptimizer.Hardened across two adversarial review rounds (terminal state machine, frozen ledger, atomic settlement, truthful failure types).
What this adds
optimization/run_context.py(new, experimental):OptimizationRunContext— optional, one-shot, caller-owned, concurrency-safe ledger of logical optimizer-owned model invocations (control-plane metadata only; no prompt/response content).completed | budget_exceeded | cancelled | failedvia ledger operations orfinalize_success/finalize_cancelled/finalize_failed.begin_model_callrejects terminal contexts; a late cancellation cannot overwrite an earlier terminal;finalize_successobserves pending cancellation and can never record a cancelled run as success.completed | provider_error | cancelled; the run iscompleted | budget_exceeded | cancelled | failed. The triggering call of a token overshoot stayscompletedwhile the run becomesbudget_exceeded.end_model_call(cancelled=True)settles a call with its usage evidence preserved.verified / partial / unreportedper call and per run;token_budget_status: within_limit | exceeded | indeterminate— missing totals are never read as compliance. Only logical-call limits are hard (atomic slot admission);max_provider_reported_tokensterminates reactively, commit-then-terminate.OptimizationProviderErroris reserved for governed provider-call terminals (provider-error-first precedence over simultaneous token overshoot);OptimizationFailedErrorcovers sampler/adapter/optimizer failures viafinalize_failed.on_budget_exceeded: "raise" | "return_partial"(defaultraise). The publicOptimizerResultschema is unchanged — the caller-owned snapshot is authoritative; snapshots/events are frozen models (tuple events) carrying budgets, terminal sequence, and terminal error metadata; event timestamps are wall-clock for durable attempt records.attach(None)is one-shot), frozenge=0-validated budgets.OptimizerCapabilities— granular, conservative defaults:accepts_run_context,model_calls_observable,logical_call_limits_enforceable,reported_token_limits_enforceable,cooperative_cancellation,sampler_usage_included(alwaysFalse). Callers omit the keyword when preflight says unsupported.AgentOptimizer.optimize(...)gains keyword-onlyrun_context: Optional[OptimizationRunContext] = None; the three built-ins accept the typed keyword and reject a supplied context with a typed error in this PR, so the abstract-signature change is independently type-clean; real support lands in the stacked PR.Verification
tests/unittests/optimization/isolated on this head: 70/70 — including both adversarial rounds' probes as first-class tests: foreign handles, 8-thread concurrent double-close, post-terminal admission (budget and completed), frozen budgets/snapshot mutation attempts, attach-None reuse, terminal precedence (late cancel vs budget), NaN/inf/negative usage, numeric error codes, sanitized/bounded error metadata, in-flight settlement after terminal, cancelled settlement with usage evidence, generic-vs-provider failure typing, wall-clock timestamps.ruff --select F401,F811,F841clean; isort + pyink applied. Stacked with #6359: 96/96.Downstream consumer: GoogleCloudPlatform/BigQuery-Agent-Analytics-SDK#318.