Skip to content

ci: unbreak CI — inline the test dataset, and ignore PLR0917 from ruff 0.16 - #176

Open
lukekim wants to merge 2 commits into
trunkfrom
fix/ci-inline-quickstart-dataset
Open

ci: unbreak CI — inline the test dataset, and ignore PLR0917 from ruff 0.16#176
lukekim wants to merge 2 commits into
trunkfrom
fix/ci-inline-quickstart-dataset

Conversation

@lukekim

@lukekim lukekim commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Every open PR is red, on two independent causes — neither of them anything the PRs changed. A dependabot bump (#159) and a release-prep branch (#155) fail identically to the feature branches.

Cause 1 — the Spicepod registry serves a broken archive

Getting Spicepod spiceai/quickstart ...
ERROR Invalid argument: Failed to extract Spicepod archive: invalid Zip archive: Could not find EOCD

api.spicerack.org returns the quickstart Spicepod with HTTP 200 and Content-Type: application/zip, but the body is a 152-byte JSON error:

{"Message":"invalid path \"spiceaiquickstartv0.1.0<sha>\": selected encoding not supported","Code":0,"Type":"error"}

so the CLI unzips an error message. Reproduces locally with the released CLI, identical under every Accept-Encoding (identity / gzip / deflate / none), and served with cache-control: public, max-age=5184000. Deterministic — not a flake that retrying clears.

The registry bug should be fixed separately; this PR only stops CI depending on it.

spice add spiceai/quickstart supplied exactly one dataset, and the integration job runs -k "not cloud", so taxi_trips is all it needs — the tpch tables belong to the cloud tests. Defining it inline removes a shared remote dependency that can, and did, fail every PR at once.

Also waits until taxi_trips is queryable rather than until /v1/ready returns. /v1/ready answers before an accelerated dataset finishes loading — locally it passed at 2s against a load completing at 11s — which races the tests into table 'taxi_trips' not found.

Cause 2 — ruff 0.16 stabilized PLR0917

The lint job installs from ruff>=0.15.12, so it took 0.16.0 the moment it released. 0.16 stabilizes PLR0917 (too many positional arguments), which existing client constructors violate at spicepy/_client.py:239 and :332 — failing lint on every PR regardless of content.

PLR0913, the "too many arguments" counterpart, is already in the ignore list, so this codebase has an established position on wide signatures. Satisfying PLR0917 instead would mean making constructor parameters keyword-only — a breaking change for a published SDK.

Verification

Ran the rendered CI steps locally against an isolated runtime:

  • inline pod loads 2,964,624 rows from the public spiceai-demo-datasets bucket — the same data quickstart provides
  • readiness gate passed at 11s, after the load rather than before it
  • pytest tests/test_main.py -k "not cloud"12 passed
  • ruff@0.16.0 check spicepy tests → 2 errors before, clean after; 0.15.12 still clean
  • black --check and pylint unaffected

Unblocks

#175, #174, #173, #172, #159, #155 — all failing on cause 1, and all would hit cause 2 on any re-run. They need trunk merged in once this lands.

Not covered here

Cloud tests fails separately with UNAUTHENTICATED: [FlightSQL] API key not found — the API_KEY secret is rejected, not merely absent. That needs the key rotated; dependabot PRs additionally never receive secrets.

…gistry

Every open PR's integration tests were failing identically, across all four
Python versions on both macOS and Ubuntu, including a dependabot bump and a
release-prep branch that touch nothing related:

    Getting Spicepod spiceai/quickstart ...
    ERROR Invalid argument: Failed to extract Spicepod archive:
          invalid Zip archive: Could not find EOCD

The cause is outside this repo. api.spicerack.org serves the quickstart
Spicepod with HTTP 200 and Content-Type: application/zip, but the body is a
152-byte JSON error:

    {"Message":"invalid path \"spiceaiquickstartv0.1.0<sha>\":
      selected encoding not supported","Code":0,"Type":"error"}

so the CLI unzips an error message. It reproduces locally, is identical under
every Accept-Encoding, and is cached for 60 days — not a flake retrying would
clear.

`spice add spiceai/quickstart` only ever supplied one dataset, and the
integration job runs `-k "not cloud"`, so taxi_trips is all it needs — the tpch
tables belong to the cloud tests. Defining that dataset inline removes a shared
remote dependency that can, and just did, fail every PR at once.

Also wait until taxi_trips is queryable rather than until /v1/ready returns.
/v1/ready answers before an accelerated dataset finishes loading — locally it
passed at 2s against a load that completed at 11s — which races the tests into
"table 'taxi_trips' not found".

Verified locally: the inline pod loads 2,964,624 rows from the public bucket,
and `pytest tests/test_main.py -k "not cloud"` passes 12/12 against it.
Copilot AI review requested due to automatic review settings July 27, 2026 16:30

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the CI integration-test workflow to remove a hard dependency on fetching the spiceai/quickstart Spicepod from the remote registry (which is currently serving invalid ZIP payloads), by defining the required taxi_trips dataset inline and gating test execution on the dataset being queryable.

Changes:

  • Inline the taxi_trips dataset definition in spicepod.yaml instead of running spice add spiceai/quickstart.
  • Replace the /v1/ready poll with a query-based readiness gate (SELECT count(*) FROM taxi_trips) to avoid racing accelerated dataset loading.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread .github/workflows/test.yml
The lint job installs ruff from `ruff>=0.15.12`, so it picked up 0.16.0 as soon
as that released. 0.16 stabilizes PLR0917 (too many positional arguments), which
existing client constructors violate in two places — failing lint on every PR
regardless of what the PR changes.

PLR0913, the "too many arguments" counterpart, is already ignored, so this
codebase has an established position on wide signatures. Satisfying PLR0917
instead would mean making constructor parameters keyword-only, a breaking change
for a published SDK.

Verified with the exact version CI installs: `ruff@0.16.0 check spicepy tests`
goes from 2 errors to clean, and 0.15.12 still passes.
Copilot AI review requested due to automatic review settings July 27, 2026 16:33
@lukekim lukekim changed the title ci: define the test dataset inline instead of fetching it from the registry ci: unbreak CI — inline the test dataset, and ignore PLR0917 from ruff 0.16 Jul 27, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

.github/workflows/test.yml:163

  • The non-Windows readiness loop always falls through after 120s and proceeds to run integration tests even if taxi_trips never became queryable. This can turn an infrastructure/startup issue into noisy test failures; fail fast (and optionally dump spice.log) when the loop times out.
          for i in {1..120}; do
            if curl -s -X POST http://localhost:8090/v1/sql \
                 -d 'SELECT count(*) AS n FROM taxi_trips' 2>/dev/null | grep -q '"n"'; then
              echo "Spice runtime is ready and taxi_trips is queryable"
              break

@lukekim lukekim self-assigned this Jul 27, 2026
lukekim added a commit that referenced this pull request Jul 27, 2026
#176 ignores PLR0917 repo-wide in pyproject.toml, which is the right fix — the
two flagged constructors fail on unmodified trunk, and satisfying the rule would
mean making public constructor parameters keyword-only.

RUF100 is enabled, so leaving per-line noqa directives here would turn into
"Unused `noqa` directive" errors the moment #176 lands. Dropping them makes this
branch depend on #176 rather than fight it.

build_search_body keeps its keyword-only signature — that stands on its own.
@claudespice

Copy link
Copy Markdown

Flagging this one for a reviewer, because it is the only thing standing between this repo's CI and green — every check on it passes, and it has been waiting ~52h.

Measured blast radius right now (conclusion=FAILURE, distinct check names, this morning's runs):

PR ruff PLR0917 Integration/Cloud tests
#155 4 12
#159 4 12
#172 4 12
#175 4 13
#171 1

Both causes are exactly the two this PR fixes, and neither is reachable from the PRs themselves:

  • PLR0917 Too many positional arguments at spicepy/_client.py:241 and :334. Reproduced against a pristine trunk checkout with the versions CI resolves (ruff==0.16.0): the same 2 errors, on trunk, with no PR applied. The rule stabilized in ruff 0.16 and ruff>=0.15.12 floats, so it landed on every branch simultaneously. pyproject.toml already ignores PLR0913 and _client.py:333 already carries # pylint: disable=R0917 for the same rule from the other linter, so the ignore this PR adds matches a decision the repo had already made — it is not a new exemption.
  • Integration/Cloud tests — the spiceai/quickstart Spicepod fetch from the registry (HTTP 503, still failing as of 20:22Z). Inlining the dataset removes the network dependency rather than waiting the outage out.

No action requested of me here and I have not touched this PR — a bot approval cannot satisfy the review requirement, so this needs a human reviewer. Recording the numbers so whoever picks it up can see it is a repo-wide unblock rather than a one-PR change.

For the record on my own #173: I kept it out of this PR's way deliberately (no duplicate pyproject.toml/test.yml edits) and fixed only the failure that was genuinely mine — a black wrap in a test this PR adds (59160b9).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants