Skip to content

feat(cayenne): runtime-wide adaptive_tuning and target_* settings - #14844

Merged
peasee merged 45 commits into
trunkfrom
peasee/261007-executor-task-slots
Oct 12, 2026
Merged

peasee merged 45 commits into
trunkfrom
peasee/261007-executor-task-slots

Conversation

@peasee

@peasee peasee commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

📝 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:

    New setting Replaces
    runtime.adaptive_tuning: enabled | disabled (default disabled) dataset and catalog param cayenne_tuning (adaptive → enabled, auto → disabled)
    runtime.params.target_replication_lag, target_freshness, target_query_latency cayenne_goal_* (global and per-dataset)
    runtime.params.target_convergence_window per-dataset cayenne_goal_convergence_window
    runtime.params.target_qph runtime.params.cayenne_goal_qph
    runtime:
      adaptive_tuning: enabled
      params:
        target_freshness: 30s
  • 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.

🔗 Related

🚨 Breaking Changes

  • 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 Makefile NEXTEST_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).

Copilot AI balanced review requested due to automatic review settings October 7, 2026 09:28
@peasee
peasee requested a review from a team as a code owner October 7, 2026 09:28
@github-actions github-actions Bot added area/config area/docs kind/dependencies Pull requests that update a dependency file kind/feature labels Oct 7, 2026
@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

✅ Pull with Spice Passed

Passing checks:

  • ✅ Title meets minimum length requirement (10 characters)
  • ✅ No banned labels detected
  • ✅ Has a label from required category kind/
  • ✅ Has a label from required category area/
  • ✅ Has at least one assignee: peasee

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The configuration lacks required surface approval and published documentation, while its fork pin and cluster integration remain unfinished.

Review effort: Balanced
Findings: 1 Medium severity · 3 Low severity

Open (4)
What changed in this PR

Adds configurable cluster executor task slots while keeping task-runner threads CPU-budgeted.

Changes:

  • Adds validated runtime.executor.task_slots with documented precedence.
  • Separates Ballista task slots from task-runner thread sizing.
  • Updates startup logs, overrides, metrics, tests, and fork tracking.
File Description
Cargo.toml Updates Ballista dependency pins.
Cargo.lock Locks updated Ballista revisions.
bin/​spiced/​src/​lib.rs Applies executor configuration and CLI overrides.
crates/​cpu-budget/​src/​lib.rs Adds task-runner thread sizing.
crates/​cpu-budget/​src/​snapshots/​cpu_budget__tests__derived_sizing.snap Updates sizing snapshot.
crates/​runtime/​src/​cluster/​mod.rs Resolves slots and configures Ballista.
crates/​runtime/​src/​init/​pods_watcher.rs Classifies executor settings as startup-only.
crates/​runtime/​src/​lib.rs Logs ineffective standalone configuration.
crates/​spicepod/​src/​component/​runtime.rs Defines, validates, and tests the setting.
docs/​dev/​fork_patches.md Records the Ballista fork patch.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread crates/runtime/src/cluster/mod.rs Resolved
Comment thread docs/dev/fork_patches.md Resolved
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.
Copilot AI balanced review requested due to automatic review settings October 7, 2026 10:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Surface approvals, referenced user documentation, and a behavioral fork guard remain unresolved.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Resolved since last review (4)

Comment thread docs/dev/fork_patches.md Resolved
Copilot AI balanced review requested due to automatic review settings October 7, 2026 10:50
@cursor

cursor Bot commented Oct 7, 2026

Copy link
Copy Markdown

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The user-facing surface lacks required sign-off and the checked-in Spicepod schema was not regenerated.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity 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.

Copilot AI balanced review requested due to automatic review settings October 7, 2026 10:54
@peasee peasee self-assigned this Oct 7, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Low severity 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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The checked-in schema and PR documentation are inconsistent, catalog tests are not gated, and hot reload can suppress migration warnings.

4 open findings

🧠 Review effort: Balanced

Comment thread Makefile Resolved
Comment thread tools/spicepodschema/src/transform.rs Resolved
Copilot AI balanced review requested due to automatic review settings October 9, 2026 03:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The parser exceeds the reviewed value contract, and the new tests use attributes that conflict with the repository lint gate.

4 open findings
2 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity 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.

🧠 Review effort: Balanced

peasee added 2 commits October 9, 2026 14:10
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.
Copilot AI balanced review requested due to automatic review settings October 9, 2026 04:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Runtime parsing accepts values rejected by the generated Spicepod schema, leaving the public configuration contract inconsistent.

3 open findings
2 resolved since last review

🧠 Review effort: Balanced

…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].
Copilot AI balanced review requested due to automatic review settings October 9, 2026 05:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The generated schema incorrectly claims adaptive-tuning values ignore case and whitespace, while the parser rejects them.

1 open finding
3 resolved since last review

🧠 Review effort: Balanced

Comment thread .schema/spicepod.schema.json Resolved
Copilot AI balanced review requested due to automatic review settings October 9, 2026 05:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The breaking migration spans parsing, reload behavior, accelerator configuration, and distributed catalog wiring, warranting final human validation.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

lukekim
lukekim previously approved these changes Oct 9, 2026
@grokspice

grokspice commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

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.

  1. 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.
  2. 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.

Copilot AI balanced review requested due to automatic review settings October 11, 2026 07:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The new catalog integration test remains excluded from the automated nextest and signoff gate.

0 open findings

🧠 Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Catalogs recreated during hot reload currently resolve adaptive tuning from the previous application configuration.

1 open finding

🧠 Review effort: Balanced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enhancement: Runtime-wide adaptive tuning (runtime.adaptive_tuning)

9 participants