You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
feat(cayenne): runtime-wide adaptive_tuning and target_* settings - #14844
Moves Cayenne's tuning settings out of per-dataset and per-catalog parameters into one runtime-wide switch and runtime-wide targets, without the cayenne_ prefix:
runtime.adaptive_tuning is a typed field. Any other value, including true, fails the Spicepod load: Invalid `runtime.adaptive_tuning` value '{value}': expected `enabled` or `disabled`. See: https://spiceai.org/docs/reference/spicepod/runtime.
Applies to every Cayenne-accelerated dataset and to Cayenne catalogs, which now also receive the target_* values. Targets still do nothing unless adaptive tuning is enabled, and every existing adaptive precondition and fallback is unchanged.
The old names are no longer read. Each one still in use gets a single startup warning that names its replacement, in either spelling (with or without cayenne_), on datasets, catalogs and runtime.params, and it is not applied. For example: Dataset 'orders' sets `cayenne_tuning`, which is no longer a dataset parameter, so it has no effect. Set `runtime.adaptive_tuning` instead. See: https://spiceai.org/docs/reference/spicepod/runtime. These keys no longer also produce the generic "Ignoring parameter" warning.
Clearer warnings for ignored targets:
Targets set while adaptive tuning is disabled get one warning per dataset or catalog.
The compaction fallback (adaptive tuning enabled but cayenne_compaction_background_interval_ms: 0) gets a single warning that also says the targets are ignored.
A runtime change to runtime.adaptive_tuning is handled like a runtime.params change, so it applies to recreated components.
Adds ParameterSpec::moved_to so a retired parameter is dropped with its own warning and left out of the generated schema.
Updates docs/cayenne/cayenne.md, the Cayenne README, and the adaptive benchmark and scenario Spicepods. Cayenne's cayenne_goal_* metrics keep their names.
cayenne_tuning and cayenne_goal_* (dataset, catalog or runtime.params) are no longer applied. Each warns at startup with its replacement. A Spicepod that relied on them runs with static tuning until it is updated.
Per-dataset target overrides are gone; targets are runtime-wide.
An invalid runtime.adaptive_tuning fails the load.
📚 Docs
The Spicepod runtime reference needs adaptive_tuning and params.target_*. The Cayenne accelerator page needs the dataset parameters removed.
👀 Notes for Reviewers
Before, on trunk's spiced: a runtime-level switch is ignored (runtime.params.tuning is not a recognized runtime parameter; ignoring, then Cayenne auto-tuned config: … tuning="auto"), and only the dataset's cayenne_tuning: adaptive turns adaptive on.
crates/runtime/tests/cayenne_runtime_tuning.rs loads real Spicepods through AppBuilder::build_from_path and reads each dataset's resolved tuning and targets. It covers:
runtime.adaptive_tuning: enabled turns adaptive on, and the default is static;
runtime.params.adaptive_tuning does not turn it on;
an invalid value fails the load;
retired dataset keys in both spellings are not applied and warn once, with no generic warning;
a renamed runtime key warns once;
the compaction fallback produces a single warning.
It runs in the make nextest gate. With the accelerator pointed back at the old keys, the enable cases fail.
crates/runtime/tests/cayenne_catalog_tuning.rs starts a scheduler and an executor and registers a real Cayenne catalog. It checks the enabled and default modes, that the targets reach catalog tables, the ignored-targets warning, and the retired catalog keys. The Cayenne catalog connector exists only with runtime/spicebench, so it runs by hand: cargo test -p runtime --features spicebench --test cayenne_catalog_tuning -- --test-threads=1.
DataAccelerator::adaptive_tuning_seeds now takes the typed mode and the runtime params. Only Cayenne overrides it; every other engine keeps the default, which has no adaptive controller.
Not run: the new warnings from a built spiced.
Review gate
Covers only the two trunk merges 27af2f6 and e804b4e pushed by fix-failing-prs (each a MakefileNEXTEST_FILTER conflict, resolved by keeping both sides), not the author's original work.
Adversarial review: codex, verdict needs-attention, scoped to origin/trunk...27af2f6b5f. No findings on the merge resolution. One unverified finding (code inspection only) on the existing catalog hot-reload path is passed to the author in a PR comment and is not fixed here.
Security review: the resolution diff adds one nextest filter clause and three comment lines in Makefile. It adds no runtime, input-handling or secret-bearing surface.
Simplify: n/a (no code written beyond the conflict hunk).
Start a real scheduler and executor and read the slot count the executor
registered with the scheduler, for the scheduler-supplied value, an
executor-own override, and the CPU budget default. Link the task_slots
validation errors to the runtime reference page and record the
datafusion-ballista pin's branch accurately.
Update Spicepod schema with executor and task_slots fields
crates/spicepod/src/component/runtime.rs:1442
The checked-in .schema/spicepod.schema.json contains neither executor nor task_slots, so schema-based validation and editor completion will not expose this new setting or its range even though the PR description says the JSON schema carries it. Regenerate and commit the Spicepod schema with this change.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
The public configuration surface lacks required PM/Design and DX/UX approval and its linked user documentation.
Review effort: Balanced Findings: None
Previously missed (1)
In code that hasn't changed since last review
Document runtime.executor.task_slots behavior before shipping
crates/spicepod/src/component/runtime.rs:1446
This validation error links users to the runtime reference, but the new runtime.executor.task_slots entry is still outstanding and Enhancement #14845 has Documentation unchecked. Publish the precedence, I/O-bound guidance, shared-memory implication, and startup-only behavior at the linked reference before shipping the field.
Reject case and whitespace variants of enabled/disabled
crates/spicepod/src/component/runtime.rs:212
The documented surface says only enabled and disabled are valid and that any other value fails, but trimming plus case-insensitive comparison also accepts ENABLED, Enabled, and values with surrounding whitespace. A representative run of this exact normalization (python over strip() + case-insensitive comparison) produced 'ENABLED' -> Enabled, ' enabled ' -> Enabled, and 'Enabled' -> Enabled. Match the two specified literals exactly (and update the acceptance tests/doc comment) so parsing implements the reviewed contract.
The retired-parameter warnings were emitted only when the resolved tuning config changed, but a retired parameter never changes it. Deduplicate them once per table and key instead.
…uning
Adds runtime.adaptive_tuning (enabled | disabled) and drops the retired cayenne_tuning and cayenne_goal_* acceleration parameters. Only these hunks are included; the generator output also carries unrelated drift from other merged changes.
…he tuning tests
The schema allows only enabled and disabled, so the parser no longer accepts a different case or surrounding whitespace. The tuning integration tests use #![expect] instead of #![allow].
The conflict with trunk is one file and both hunks are append/append, so it does not need a judgement call. Posting the resolution rather than pushing it, for the reason at the bottom.
Scope.git merge-tree --write-tree --name-only origin/trunk 3b67dd23b3 against trunk at 86b7741973 (merge base 15f70ac0fa) reports exactly one conflicted path:
CONFLICT (content): Merge conflict in Makefile
Every other changed file auto-merges, including crates/accelerators/accelerator-cayenne/src/lib.rs, crates/runtime/src/builder.rs and crates/spicepod/src/component/runtime.rs.
Both hunks.trunk introduced RUST_GATE_FEATURES and added (package(=runtime-acceleration) & binary(=optional_snapshot_files)) to NEXTEST_FILTER; this branch added (package(=runtime) & binary(=cayenne_runtime_tuning)) to the same line and three comment lines above it. Nothing was edited twice.
The comment hunk: keep this branch's three cayenne_runtime_tuning lines, then trunk's rewritten sentence (the one beginning They need and naming RUST_GATE_FEATURES), not this branch's older wording of it.
NEXTEST_FILTER: take trunk's line and insert (package(=runtime) & binary(=cayenne_runtime_tuning)) + after the rate_control term.
Checked the union, since a filter line is where a silent coverage drop would hide. Splitting both sides and the resolution on +: trunk 18 terms, this branch 18, resolution 19 unique, and the set difference is empty in both directions — no term dropped, none invented. Produced with git merge-file from the three blobs; it leaves zero conflict markers.
One thing to check while you are in there, unverified — code inspection only. This branch adds two test targets under crates/runtime/tests/, cayenne_runtime_tuning.rs and cayenne_catalog_tuning.rs (355 lines), neither with a [[test]] entry in crates/runtime/Cargo.toml. NEXTEST_FILTER names runtime's test binaries one by one — there is no (package(=runtime) & kind(=test)) term — and only cayenne_runtime_tuning was added. So cayenne_catalog_tuning reads as compiled-but-never-run under the sign-off gate. I have not run anything to confirm that; cargo nextest list -E '<the merged NEXTEST_FILTER>' on the merged tree would settle it either way (there is no print-nextest-filter target; copy the line out of the Makefile). If it is deliberate, ignore this.
Why this is a comment and not a push.trunk's ruleset carries dismiss_stale_reviews_on_push: true and require_extra_approval_for_unattributed_changes: true, so a merge commit pushed by this bot would drop the approval on this PR and then ask for two approvals instead of one. A hand-resolved merge also does not inherit the existing sign-off, so it would need a fresh one on a head that is currently attested. Resolving it yourself costs the approval once; having me do it costs it once plus an extra reviewer. Say the word and I will push it anyway.
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
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
Moves Cayenne's tuning settings out of per-dataset and per-catalog parameters into one runtime-wide switch and runtime-wide targets, without the
cayenne_prefix:runtime.adaptive_tuning: enabled | disabled(defaultdisabled)cayenne_tuning(adaptive→enabled,auto→disabled)runtime.params.target_replication_lag,target_freshness,target_query_latencycayenne_goal_*(global and per-dataset)runtime.params.target_convergence_windowcayenne_goal_convergence_windowruntime.params.target_qphruntime.params.cayenne_goal_qphruntime.adaptive_tuningis a typed field. Any other value, includingtrue, fails the Spicepod load:Invalid `runtime.adaptive_tuning` value '{value}': expected `enabled` or `disabled`. See: https://spiceai.org/docs/reference/spicepod/runtime.Applies to every Cayenne-accelerated dataset and to Cayenne catalogs, which now also receive the
target_*values. Targets still do nothing unless adaptive tuning is enabled, and every existing adaptive precondition and fallback is unchanged.The old names are no longer read. Each one still in use gets a single startup warning that names its replacement, in either spelling (with or without
cayenne_), on datasets, catalogs andruntime.params, and it is not applied. For example:Dataset 'orders' sets `cayenne_tuning`, which is no longer a dataset parameter, so it has no effect. Set `runtime.adaptive_tuning` instead. See: https://spiceai.org/docs/reference/spicepod/runtime. These keys no longer also produce the generic "Ignoring parameter" warning.Clearer warnings for ignored targets:
cayenne_compaction_background_interval_ms: 0) gets a single warning that also says the targets are ignored.A runtime change to
runtime.adaptive_tuningis handled like aruntime.paramschange, so it applies to recreated components.Adds
ParameterSpec::moved_toso a retired parameter is dropped with its own warning and left out of the generated schema.Updates
docs/cayenne/cayenne.md, the Cayenne README, and the adaptive benchmark and scenario Spicepods. Cayenne'scayenne_goal_*metrics keep their names.🔗 Related
🚨 Breaking Changes
cayenne_tuningandcayenne_goal_*(dataset, catalog orruntime.params) are no longer applied. Each warns at startup with its replacement. A Spicepod that relied on them runs with static tuning until it is updated.runtime.adaptive_tuningfails the load.📚 Docs
adaptive_tuningandparams.target_*. The Cayenne accelerator page needs the dataset parameters removed.👀 Notes for Reviewers
Before, on trunk's
spiced: a runtime-level switch is ignored (runtime.params.tuning is not a recognized runtime parameter; ignoring, thenCayenne auto-tuned config: … tuning="auto"), and only the dataset'scayenne_tuning: adaptiveturns adaptive on.crates/runtime/tests/cayenne_runtime_tuning.rsloads real Spicepods throughAppBuilder::build_from_pathand reads each dataset's resolved tuning and targets. It covers:runtime.adaptive_tuning: enabledturns adaptive on, and the default is static;runtime.params.adaptive_tuningdoes not turn it on;It runs in the
make nextestgate. With the accelerator pointed back at the old keys, the enable cases fail.crates/runtime/tests/cayenne_catalog_tuning.rsstarts a scheduler and an executor and registers a real Cayenne catalog. It checks the enabled and default modes, that the targets reach catalog tables, the ignored-targets warning, and the retired catalog keys. The Cayenne catalog connector exists only withruntime/spicebench, so it runs by hand:cargo test -p runtime --features spicebench --test cayenne_catalog_tuning -- --test-threads=1.DataAccelerator::adaptive_tuning_seedsnow takes the typed mode and the runtime params. Only Cayenne overrides it; every other engine keeps the default, which has no adaptive controller.Not run: the new warnings from a built
spiced.Review gate
Covers only the two
trunkmerges 27af2f6 and e804b4e pushed by fix-failing-prs (each aMakefileNEXTEST_FILTERconflict, resolved by keeping both sides), not the author's original work.codex, verdictneeds-attention, scoped toorigin/trunk...27af2f6b5f. No findings on the merge resolution. One unverified finding (code inspection only) on the existing catalog hot-reload path is passed to the author in a PR comment and is not fixed here.Makefile. It adds no runtime, input-handling or secret-bearing surface.