chore: fix stale docs and CI checks that were not running - #2438
NoahKusaba wants to merge 18 commits into
Conversation
Documentation had drifted from the code in several places, and a set of GitHub-style anchor links never resolved on the published site. Correctness: - AQE was documented as experimental and disabled by default in three places; `ballista.planner.adaptive.enabled` defaults to true. - Scheduler policy default is push-staged, documented as pull-staged; event-loop buffer default is 1000, documented as 10000. - `ballista-cli` has no `spark-compat` feature, but two guides told users to install and build with it. - Scheduler REST table was missing five endpoints and both health probes; the Prometheus list carried a `failed` metric that does not exist. - Every doc that started a second executor moved only `--bind-port`, which collides with the first executor's gRPC port. - CLI usage blocks were clap 3 output missing `--vcores`, `--color` and `--tui`; regenerated from the built binary. - extending-components built a runtime with `RuntimeConfig`, removed in DataFusion 55, and used helper names that had been renamed. - License badge pointed at datafusion-comet; adopter rows referenced image files that are not in the repo. Site rendering: - Set `myst_heading_anchors`, without which MyST emits no heading ids and all 14 `file.md#heading` cross-references 404. These work when reading the files on GitHub, which is why the breakage went unnoticed. - code-organization.md started its headings at H2. Structure: - Delete docs/developer/, which sat outside the Sphinx tree and so was never published, after porting its Multi-Partition Tasks section and the SchedulerGrpc method table into the contributors guide. Its diagram was dropped rather than ported: it labels arrows `ExecutePartition`, which is not an RPC in the current proto. - Pull the Rust examples from examples/examples/ with `literalinclude` instead of pasting them; all three copies had already drifted from the shipped source. - Replace the hand-maintained config tables in the tuning guide with generated regions, so the existing CI check covers them. - Give ballista-core, ballista-scheduler and ballista-executor real READMEs; each is a docs.rs landing page via include_str! and was four lines long. - Standardise on "vcores" over "task slots" and "concurrency levels". - Collapse cluster-start instructions from five copies to two canonical pages, which is the duplication that produced the port collision above. Verified with prettier, sphinx-build -W, cargo doc under -D warnings, cargo test --doc, and the config-docs drift check.
The user guide now pulls remote-sql.rs and remote-dataframe.rs in with `literalinclude` rather than pasting copies of them. The deploy workflow only triggered on `docs/**`, so editing an example would have changed the rendered site without republishing it. Add `examples/examples/**` to the trigger paths.
Several checks were scoped by an explicit list that had drifted, and two were not running at all. Workflows that GitHub never saw: - Move stale.yml into .github/workflows/. It was committed to the repository root in apache#1488, where GitHub does not look for workflows, so the stale-PR automation has never run once. It is left manual-only (workflow_dispatch) rather than switched on, since nobody has decided the project wants stale PRs closed automatically. - Delete the root copy of take.yml. The live one is .github/workflows/take.yml, and the two had already diverged: the root copy lacks timeout-minutes and is otherwise dead weight. Lint scopes that missed files: - clippy named seven crates, so ballista-api-types, ballista-history and ballista-chaos were never linted. Use --workspace instead, keeping ballista-cli separate because its tui and web features target different platforms and are not meant to be enabled together. - ruff ran only inside python/, leaving twelve files unchecked, including every dev/ script and docs/source/conf.py. Lint from the repository root instead. Ruff resolves the nearest config per file, so python/ keeps python/pyproject.toml and a new root ruff.toml covers the rest. - Fix the three real findings this surfaced (an unused import, a duplicate import, an unused local) and reformat. The E402s in the benchmark scripts are deliberate and are ignored per-file rather than restructured. - dev/rust_lint.sh, which CONTRIBUTING calls "run them all at once", ran four of the eight CI lint scripts, so a clean local run could still fail CI on the config-docs, proto and rustdoc checks. Checks that could pass without running: - run-rat.sh invoked `python`, not `python3`. On a host with no `python` the call failed, filtered_rat.txt was left empty, and the script then reported "No unapproved licenses" because there was nothing to count. It now distinguishes "found unapproved files" from "did not run". - docs/build.sh ran plain `make html` with SPHINXOPTS unset, which is how 16 Sphinx warnings sat unnoticed. Build with -W --keep-going. Cleanup: - Drop Arrow-inherited entries from .gitignore (cpp, R, Visual Studio, perf) and from rat_exclude_files.txt (vcpkg, Cython, yarn, helm, and two CHANGELOG paths that do not exist). Verified with apache-rat that the pruned list produces the same result as the original. - Drop the prettier exclusion for ballista/CHANGELOG.md, which does not exist, from both CI and CONTRIBUTING. - pre-commit.sh referenced "apache arrow" and required a nightly toolchain that rust-toolchain.toml does not install. - Document dev/update_datafusion_versions.py, which nothing referenced.
|
The sensible thing would have been to split this up into many smaller PR's, but I don't want to spend much time on this, as I will be refocusing efforts on the iceberg-ballista integration, as the iceberg-datafusion crate has migrated to it's own repo. This is clearly important work that should get done, but I'm hoping the other more experienced maintainers can focus their efforts on feature integrations to close the gap with Spark's performance on benchmarks. Let me know how you would like me to proceed, or if we can just merge this as-is. I have read every line, and it makes sense to me. -- I may have gotten a little overboard with also widening and cleaning up python linting. My hope is that fixing the docs can attract more people to Ballista. |
andygrove
left a comment
There was a problem hiding this comment.
Thanks for digging into this. The four checks that were never actually running are a great find, especially run-rat.sh reporting "No unapproved licenses" when the checker had not run at all. I spot-checked the factual claims against the tree rather than taking them on trust, and most hold up: push-staged really is the default, prometheus-metrics really is not a default scheduler feature, ballista-cli really has no spark-compat, the added REST rows and both probes exist in api/routes.rs and api/health.rs, the coalesce and parallel-window keys exist and default to false, the port-collision fix is right against the executor defaults, and :lines: 18- lands exactly on the first use in both included examples.
No public API changes here, both .rs edits are string-only, and the one on task_distribution is a rustdoc line on a field with an explicit clap help, so --help output does not move. No plan shape or execution changes either, so no benchmark numbers needed for this one.
A few things inline before it goes in. The allow_http one is the only real blocker, the rest are small or process notes.
| .await? | ||
| .show() | ||
| .await?; | ||
| ctx.sql("SET s3.allow_http = true").await?.show().await?; |
There was a problem hiding this comment.
I do not think this line should go. examples/examples/custom-client.rs still has it, and ballista/core/src/object_store.rs:205 rejects an http:// endpoint unless s3.allow_http is true. The line just above this sets s3.endpoint = 'http://localhost:9000', so with this removed the documented sequence fails against minio as written.
Can you put it back?
There was a problem hiding this comment.
This is actually a redundant call!
If you check line 185, the ctx already calls "SET s3.allow_http = true".
custom-client.rs also has the redundant "SET s3.allow_http = true" as well.
I've removed it from the example to remove confusion (in my follow up commit). Let me know if that works for you?
I also verified the example runs and compiles without the duplicate.
| | scheduler-policy | Utf8 | pull-staged | Sets the task scheduling policy for the scheduler, possible values: pull-staged, push-staged. | | ||
| | event-loop-buffer-size | UInt32 | 10000 | Sets the event loop buffer size. for a system of high throughput, a larger value like 1000000 is recommended. | | ||
| | scheduler-policy | Utf8 | push-staged | Sets the task scheduling policy for the scheduler, possible values: pull-staged, push-staged. | | ||
| | event-loop-buffer-size | UInt32 | 1000 | Sets the event loop buffer size. for a system of high throughput, a larger value like 1000000 is recommended. | |
There was a problem hiding this comment.
1000 is correct for this table since it is the CLI default at ballista/scheduler/src/config.rs:135, so no change needed here.
Worth knowing though that SchedulerConfig::default() at ballista/scheduler/src/config.rs:429 is still 10000, so anyone constructing the config in Rust rather than via the CLI gets a different number than this now claims. That is a pre-existing inconsistency in the code, not something you introduced, but it would be good to open a follow-up issue for it.
There was a problem hiding this comment.
Should I just set the config.rs to 1000?
| cd python | ||
| uv run --no-project ruff check --output-format=github . | ||
| uv run --no-project ruff format --check . | ||
| uv run --no-project ruff check --output-format=github .. |
There was a problem hiding this comment.
Widening the scope is right, but this loses the inline annotations for everything it adds. Ruff emits paths relative to the working directory, so running from python/ with .. reports a finding in dev/foo.py as ../dev/foo.py, and GitHub cannot map that back to a file. The annotations quietly stop appearing for every file outside python/.
Running from the repository root instead should fix it and keep the per-file config resolution you describe in the comment.
There was a problem hiding this comment.
AI Validation:
Thanks — I checked this by planting a deliberate F401 and reading the raw
workflow command. Ruff doesn't emit ../; with --output-format=github it
writes an absolute path into file=, which is what GitHub uses to place the
annotation.
For the same finding, file= is byte-identical across all three:
before (cd python, `.`) file=/…/python/python/ballista/jupyter.py
this PR (cd python, `..`) file=/…/python/python/ballista/jupyter.py
root (root, `.`) file=/…/python/python/ballista/jupyter.py
So annotations behave exactly as they did before this PR. What does change with
the working directory is the human-readable text after the ::, which GitHub
ignores for placement — I suspect that's what you spotted in the log.
Happy to run from the root anyway if you prefer the shorter log paths, though
the cd python is load-bearing (it's how uv run --no-project finds the synced
venv), so it'd need to invoke python/.venv/bin/ruff directly.
| # ballista-cli is excluded here and linted separately below: its `tui` and `web` | ||
| # features target different platforms and are not meant to be enabled together, | ||
| # so --all-features is not meaningful for it. | ||
| cargo clippy --all-targets --workspace --exclude ballista-cli --all-features -- -D warnings |
There was a problem hiding this comment.
Agreed on moving to --workspace, the explicit list had clearly gone stale.
One side effect worth being aware of: --workspace --all-features now turns on ballista-core's force_hash_collisions while linting every other crate, which the old per-package invocations did not do. It is lint-only so no test behaviour changes, just noting it in case a confusing cfg-dependent warning shows up later.
…mple The example set `s3.allow_http = true` twice, once before the credential and endpoint SETs and once after. The second call is dead: `S3Options::set` writes one field per key, so setting the endpoint cannot clear `allow_http`, and `CustomObjectStoreRegistry::get_store` builds the S3 store from a fresh read of the config on each call, so only the final state is ever observed. The extending-components guide mirrors this example and had the same duplicate; it was removed there earlier in this branch.
Each of the three S3 tests set `s3.allow_http = true` twice, once before the credential and endpoint SETs and once after. The second is dead: `S3Options::set` writes exactly one field per key, nothing resets the config wholesale, and `allow_http` is only read inside `s3_object_store_builder`, which `get_store` calls from a fresh config read per request. The endpoint scheme validation that the trailing SET looks like it is guarding runs at store-build time, not at SET time, so the first call already satisfies it. Matches the same removal from custom-client.rs and the extending-components guide earlier in this branch.
"Composition with in-flight DataFusion AQE" built a three-level thesis on two upstream PoCs. apache/datafusion#23026 is still an open draft, last touched 2026-08-24, and apache/datafusion#23167 was closed without merging. Anchoring a contributors-guide section to those is the drift this branch is trying to remove. The Multi-partition tasks section above it stays: it describes dispatch behaviour that exists today. The one place a draft link is still warranted is partitioned_bounded_window_agg.rs, where the rustdoc cites #23026 to explain why the wrapper is temporary.
The `session_state_with_s3_support` snippet had drifted from the shipped function: it dropped the `ballista_scalar_functions`, `ballista_aggregate_functions` and `ballista_window_functions` chains and narrowed the return type. The surrounding prose claims these are the shipped implementations, so the copy being wrong is the exact failure this branch is about. Pull all three helpers in with `literalinclude` and `:start-at:`/`:end-before:` anchors rather than re-copying them, so they track the source. Add `ballista/core/src/object_store.rs` to the docs deploy trigger for the same reason `examples/examples/**` is there.
The doc comment on `PrometheusMetricsCollector` listed a \*failed\* metric. There is no such export: the counter is registered as `job_failed_total` and only the struct field is named `failed`. The same stale name was removed from the metrics user guide earlier in this branch. The header says "7 metrics" while the list had 8 entries, which is the other half of the tell. Doc list and registered names now both come to 7 and match exactly.
|
Hey @milenkovicm there is a stale.yml file in the CI that I left disabled in this PR, which automatically closes old inactive PR's. |
|
Please do,I was wondering why is it not working |
Merging main brought in the OpenAPI endpoint (apache#2397) and build-side staging for AQE joins (apache#2434). Neither was reflected in the docs this branch had already made complete, and auditing them turned up two older inaccuracies in the same sections. From the merged changes: - Document the new `utoipa` feature on ballista-core in the root and crate READMEs, and note that the scheduler's `rest-api` feature now also serves the OpenAPI spec. - Add build-side staging to the AQE optimization lists in the tuning and architecture guides, and to the 55.0.0 upgrade guide, since it is a new default that changes query plans. Pre-existing inaccuracies: - The tuning guide's Join Strategy section only described the static planner. With AQE on by default, `prefer_hash_join` is not consulted; the join is chosen at runtime from the broadcast threshold and `hash_join_max_build_partition_bytes`. The section now covers both paths, and notes that the static planner only promotes hash joins to broadcast. - Join reordering compares byte sizes first and falls back to row counts, not row counts alone. - The REST API was described as an optional feature to enable, but `rest-api` is on by default.
Restore the nightly schedule on the stale PR workflow and drop the comment that kept it manual-only. It marks pull requests with no activity for 60 days and closes them 7 days later. Issues are left alone. Drafts are not exempt, so draft PRs are included.
Keep only the comments that stop a reader from undoing something, cut each to a line or two, and drop the ones that restate the code.
|
I cleaned up the comments, which were intended to explain changes to reviewers, but aren't really needed + resolved conflict and updated docs with latest merged changes. Also enabled the stale.yml file, per the discussion with milenkovicm. |
|
I'm getting the impression we don't feel comfortable shotgun merging alot of cleanups at once with AI yet. |
Came in from main via the merge; the repo-root ruff lint now covers dev/. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Rationale for this change
I have noticed for a long time that Ballista's documentation has drifted from the code: wrong defaults, examples that no longer compile against the shipped API, and passages duplicated across pages that have since come to disagree with each other.
Chasing that turned up the same pattern in the checks that were supposed to
prevent it. Several were scoped by an explicit list that had gone stale, and four
were not running at all:
myst_heading_anchorswas never set, so MyST emits no heading ids and all 14
file.md#headinglinks404. They resolve on GitHub, which is why nobody noticed.
sphinx-buildonmainreports 16 warnings; this PR takes it to 0.docs/developer/was never published. It sits outside the Sphinxsource/tree, yet Multi-partition tasks: partition_slice plumbing + SortShuffleWriter refactor #2038 and fix: repair AQE plan rewrites, remove a redundant shuffle, and enable adaptive query planning by default #2315 both added architecture docs to it.
stale.ymlhas never run. ci: add take and stale #1488 committed it to the repository root, whereGitHub does not look for workflows.
run-rat.shcould pass without checking anything. It invokedpython, notpython3; with nopythonon PATH the checker never ran, left an empty report,and the script then printed "No unapproved licenses". I hit exactly this.
What changes are included in this PR?
71 files, +1088/−1303. Most of the net deletion is duplicated content collapsed to
a single source. Twelve commits, each separable.
Documentation correctness
ballista.planner.adaptive.enableddefaults totrue.push-staged, documented aspull-staged.Event-loop buffer default is 1000, documented as 10000.
ballista-clihas nospark-compatfeature, but two guides told users toinstall and build with it. Both commands fail as written.
Prometheus list carried a
failedmetric that does not exist.--bind-port, colliding withthe first executor's gRPC port.
--vcores,--color,--tui.Regenerated from the built binary.
extending-componentsbuilt a runtime withRuntimeConfig, removed inDataFusion 55, using helper names that had been renamed.
datafusion-comet; adopter rows referenced imagesthat are not in the repo.
Documentation rendering and structure
myst_heading_anchors = 4, fixing the 14 dead cross-references. Also a shorttitle overline in
index.rstand a link to a directory rather than a document.docs/developer/after porting its Multi-Partition Tasks section and theSchedulerGrpcmethod table into the contributors guide. Its diagram was droppedrather than ported: it labels arrows
ExecutePartition, not an RPC in thecurrent proto.
examples/examples/withliteralincluderatherthan pasting them. All three copies had already drifted.
regions, so the existing config-docs gate covers them.
ballista-core,ballista-schedulerandballista-executorreal READMEs.Each is a docs.rs landing page via
include_str!and was four lines long.duplication is what produced the port collision above, in four places.
Change to the user-personas contract
Calling this out on its own, because
user-personas.mdis the contract PRs arereviewed against and is append-only by its own terms. One persona's wording
changed, in Persona 2 (the Spark user):
"executors with task slots" became "executors with vcores".
My reading is that this corrects a guarantee the page was already not providing
rather than withdrawing one. Multi-partition tasks landed in #2038, and
ballista.scheduler.max_partitions_per_taskdefaults to0, so the schedulerpacks several partitions per task by default. The page was promising Spark's
strict one-task-per-partition dispatch, which Ballista had stopped doing.
The capability itself is not removed, and I added a line saying how to get it
back: set
max_partitions_per_taskto1. Everything else that persona dependson is untouched.
Flagging it because the page says a change like this needs explicit discussion
and sign-off rather than landing quietly, and a 69-file diff is exactly where it
could land quietly. Happy to revert the wording and leave the contract stale if
you would rather correct it in its own PR.
CI and repo hygiene
stale.ymlinto.github/workflows/and switch it on. It runs nightly,marks PRs with no activity for 60 days, and closes them 7 days later. Issues are
left alone, and drafts are not exempt.
Delete the root
take.yml, a diverged duplicate of the live one.ballista-api-types,ballista-historyandballista-chaosunlinted. Use--workspace, keepingballista-cliseparatebecause its
tuiandwebfeatures target different platforms.python/, leaving twelve files unchecked including everydev/script anddocs/source/conf.py. Lint from the repository root; a newroot
ruff.tomlcovers whatpython/pyproject.tomldoes not. Fixes the threereal findings this surfaced; the
E402s in benchmark scripts are deliberate andignored per-file.
dev/rust_lint.sh, which CONTRIBUTING calls "run them all at once", ran four ofthe eight CI lint scripts.
run-rat.shnow distinguishes "found unapproved files" from "did not run".docs/build.shbuilds with-W --keep-going, so a broken cross-reference failsinstead of shipping.
examples/examples/**to the docs deploy trigger, now that the guide embedsthose files.
.gitignoreandrat_exclude_files.txt, and aprettier exclusion for a
CHANGELOG.mdthat does not exist. Fixpre-commit.sh,which referenced "apache arrow" and required an uninstalled nightly toolchain.
Document
dev/update_datafusion_versions.py, which nothing referenced.Follow-ups from review
SET s3.allow_http = truefromcustom-client.rsandfrom the three S3 integration tests, matching the earlier removal in the
extending-components guide. It is dead:
S3Options::setwrites one field perkey, nothing resets the config, and
allow_httpis read only insides3_object_store_builder, whichget_storecalls from a fresh config read.Confirmed by running the S3 integration tests, which pass.
architecture guide. It built on Parallel bounded RANGE-frame window functions without PARTITION BY (draft) datafusion#23026, still an open draft,
and [PoC/Proposal] AQE-lite: change plan properties at runtime based on stats from pipeline breakers datafusion#23167, closed without merging. Moved to Design note: composing multi-partition tasks with plan-level AQE in DataFusion #2445. The
multi-partition tasks section above it stays, since that describes dispatch we
have. The one remaining draft link, in
partitioned_bounded_window_agg.rs, isdeliberate: it explains why that wrapper is temporary.
extending-componentshad drifted from theshipped functions:
session_state_with_s3_supportwas missing itswith_scalar_functions/with_aggregate_functions/with_window_functionschains, its
use, and its full return type, while the prose claimed thesnippets were the shipped implementations. They are now pulled from
ballista/core/src/object_store.rswithliteralinclude, anchored on doccomments rather than line numbers. Verified against the built HTML: the
rendered block is byte-identical to the source slice.
ballista/core/src/object_store.rsjoinsexamples/examples/**on the docs deploy trigger. Every
literalincludetarget in the docs is nowcross-checked against that path list.
failedmetric from thePrometheusMetricsCollectorrustdoc that was removed from the metrics guide.The counter is registered as
job_failed_total; only the struct field isnamed
failed. The header claimed 7 metrics while listing 8; doc list andregistered names now both come to 7 and match exactly.
Keeping up with
mainMerged
mainwhile this was in review. #2397 and #2434 conflicted with the RESTtable and the generated config tables; resolved by keeping both sides and
regenerating the tables from source. Both also made docs this branch had already
corrected incomplete:
utoipafeature toballista-core, now in both feature tables,and the scheduler's default
rest-apifeature now also serves the OpenAPI spec.tuning and architecture guides, and has a 55.0.0 upgrade entry, since it changes
query plans.
Auditing those sections turned up three older inaccuracies:
default,
datafusion.optimizer.prefer_hash_joinis not consulted; the join ischosen at runtime from
broadcast_join_threshold_bytesandhash_join_max_build_partition_bytes. The section now covers both paths, andnotes that the static planner only promotes hash joins to broadcast.
counts alone.
rest-apiison by default.
Are there any user-facing changes?
No API, behaviour, or configuration-value changes.
Five
.rsfiles are touched, none of them library logic:ballista/core/src/config.rsConfigEntrydescription stringballista/scheduler/src/config.rsTaskDistributionvariant that does not existballista/scheduler/src/metrics/prometheus.rsexamples/examples/custom-client.rsSETexamples/tests/object_store.rsSETin three testsTen
.pyfiles are touched. Seven are reformat-only, which I checked bycomparing parsed ASTs before and after. The other three are the lint fixes
described above plus the one-line
conf.pysetting.For contributors, lint scope widens and the docs build now fails on warnings. Both
are green as of this branch.
Notes for reviewers
docs/source/conf.pychanges how every Markdown page renders. One line,site-wide effect.
docs/developer/is the only irreversible change. Unique contentwas ported first.
stale.ymlis now enabled, and will act on existing PRs. It has never runbefore, so its first nightly run sweeps the whole backlog. As of 2026-09-13, 8
of the 45 open PRs have had no activity for 60 days, including 3 drafts. They
would be marked stale and closed 7 days later unless someone comments. That is
a project decision worth an explicit yes from a maintainer.
section above; this is the one change here that wants a deliberate yes or no.
SET prefer_hash_join = truefor hash joins. Under AQE, the default, thatsetting is ignored, so the section now says so and scopes that advice to AQE
turned off.
Verification
sphinx-build -Wmain)prettier@2.7.1 --check./dev/update_config_docs.sh --checkruff check/ruff format --check, repo rootcargo docunderRUSTDOCFLAGS="-D warnings"cargo test --docapache-ratgit archiveof the branch; pruned exclude list matchesmain's resultcargo clippy --workspace,-D warnings--features testcontainersHow this was produced
Drafted with Claude Opus 5, starting from "scan all documentation and do a full
cleanup/correctness fix", then iterated into the structural and CI changes above.
Every factual claim was checked against the source tree rather than taken on the
model's word, and the table above is the gate it was held to. Reviewers should
still read the prose themselves.