Skip to content

Commit a1cf2cd

Browse files
dlovellclaude
andauthored
fix(deps): declare packaging, numpy, pygments and xxhash as runtime dependencies (#2268)
## The bug `python/xorq/ibis_yaml/packager.py:40` imports `packaging` at module level, but `packaging` was never declared in `[project].dependencies`. It reaches every development environment transitively — `pytest`, `black`, `matplotlib` and `snowflake-connector-python` all depend on it — so nothing local ever noticed. Installing only the declared dependencies fails: ```console $ uv tool run --isolated --python 3.12 --with xorq==0.4.0 xorq run builds/f68b0e436e96 File ".../xorq/ibis_yaml/packager.py", line 40, in <module> from packaging.specifiers import SpecifierSet ModuleNotFoundError: No module named 'packaging' ``` `xorq build` survives, because it doesn't reach `packager` on that path. `xorq run`, `xorq catalog` and the TUI (`catalog/tui.py:82`) are dead on a clean install, as is `backends/pandas/executor.py`. **This affects 0.4.0 as published, so it needs a patch release, not just a fix on main.** ## The fix Declared `packaging`. Auditing the rest of the always-loaded code surfaced three more module-level imports of undeclared distributions, arriving via `pandas`, `rich`/`textual` and `xorq-dasher`: | import | reached today via | declared range of the provider | | --- | --- | --- | | `numpy` | `pandas` | `pandas>=2.2.3,<3` | | `pygments` | `rich`, `textual` | `rich>=13.9.4` | | `xxhash` | `xorq-dasher` | `xorq-dasher>=0.1.1` | Every one of those is an open-ended range in someone else's metadata. Declared all three rather than depend on them. ### Correction: declaring is not free under `--resolution lowest-direct` My first pass claimed this "adds no install surface". That was wrong, and `ci-test-lowest-direct (3.13)` caught it. Floor-lowering applies to **direct** dependencies, so promoting `numpy` and `xxhash` from transitive to declared newly subjected them to the floor lock, where a bare `>=` selects a release predating the interpreter: ``` numpy==1.26.0 # no cp313 wheel -> sdist -> meson: "No BLAS library detected!" xxhash==3.0.0 # no cp313 wheel either; queued to fail right after numpy ``` Previously `pandas`' own per-interpreter numpy floors happened to keep this installable. Floors are now the first release shipping wheels for each supported interpreter, following the existing precedent for `matplotlib` and `scikit-learn` in the `examples` extra: | | 3.10 | 3.11 | 3.12 | 3.13 | | --- | --- | --- | --- | --- | | `numpy` | 1.22.4 | 1.23.2 | 1.26.0 | 2.1.0 | | `xxhash` | 3.0 | 3.2 | 3.4 | 3.5 | Gated rather than raised outright, so numpy 1.x stays usable on older interpreters — xorq only touches `numpy.ndarray` and `numpy.dtype`, so forcing 2.1 everywhere would constrain downstreams for nothing. `pygments` is pure python and needs no gate. Verified with `uv pip compile --resolution lowest-direct` for each of 3.10–3.13, cross-checking every resolved version against the PyPI file list — all twelve now ship a usable wheel. Everything else that turned up in the audit is either extras-gated (`pyiceberg`, `adbc_*`, `snowflake`, `databricks`, `sklearn`, `gcsfs`) or `try`/`except ImportError`-guarded (`regex` in `backends/pandas/kernels.py:10`, `importlib_metadata` in `__init__.py:1`). ## Tests ### `python/xorq/tests/test_declared_dependencies.py` — static, `core` marker AST-scans **all of `python/xorq`**, minus twelve extras-gated modules named by path, for third-party imports at module scope and asserts each is declared. Scoping by exclusion rather than inclusion means a new module anywhere in xorq is guarded by default; `test_extras_gated_modules_are_all_still_needed` stops the exclusion list becoming a place to park problems. Walking `tree.body` rather than `ast.walk` is what makes it precise: imports nested in `try`/`except ImportError`, `if` blocks or function bodies are guarded or deferred on purpose, so their absence is already handled. That classification is itself parametrized and tested. **There is deliberately no allowlist** for "it arrives via some other dependency": - An allowlist encodes the *same* inference that shipped this bug. `packaging` was already an unwritten allowlist entry — *"pytest pulls it, we're fine."* True in every dev env, false in every user env. Writing it down makes it auditable, not true. - It asserts a fact that isn't ours to hold. "pygments comes via rich" is a claim about rich's metadata under an open range; rich can drop it and nothing in this repo changes. - A guard test over `uv.lock` doesn't fix that. The lock is one dev resolution at one moment, while users resolve fresh from PyPI. And *reachable ≠ present*: `pandas` lists `numpy` twice under split `python_version` markers, so a naive graph walk reports "covered" for an edge whose marker is false on the user's interpreter. - The cost is upside down — lock parsing plus marker evaluation, maintained forever, to avoid writing three lines. Since there's no exception mechanism, there's no configuration surface to argue about: an undeclared module-level import is a hard failure with a one-line fix. Runs in the `core` job and in `ci-test-lowest-direct`, which also exercises the new floors under `--resolution lowest-direct`. ### `python/xorq/tests/test_bare_install.py` — end-to-end, `bare_install` marker Builds the wheel and drives the CLI through `uv tool run --isolated`, which installs `[project].dependencies` and nothing else. This covers what the static scan structurally cannot: most packager imports in `cli.py` are lazy, inside function bodies (`# noqa: PLC0415`). It asserts a **build → run round trip**, not just `--help`, because that distinction is the whole bug. ## Verified both tests fail before the fix Deleting `"packaging>=22"` from `pyproject.toml` and re-running: ``` drop packaging -> 3 failed, 19 passed # static scan -> 2 failed, 5 passed # bare install drop numpy -> 2 failed, 20 passed drop xxhash -> 2 failed, 20 passed drop pygments -> 2 failed, 20 passed ``` with the original traceback reproduced through `cli.py:348 -> packager.py:40`. Note what still passes on a `packaging` drop: **every `--help` test stayed green** — which is why the round trip, not `--help`, is the thing worth asserting. ### What each layer actually guards Only `packaging` is caught by *both* layers. Removing `numpy`, `pygments` or `xxhash` leaves all bare-install tests green, because each still arrives transitively via `pandas`, `rich`/`textual` and `xorq-dasher`. The bare-install layer exercises those import paths; **the static scan is what guards the declarations.** An earlier revision of this PR overstated that, and the docstring now says it plainly. Restored, all 29 pass. ## CI `ci-test-library` and `ci-test-install` already installed the wheel into a bare environment — but only smoke-imported `xorq` itself, which succeeds even when broken. Worse, `ci-test-library`'s pytest step runs `--with pytest --with pytest-cov`, and **pytest depends on `packaging`**, so that environment is contaminated against exactly this class of bug. Both workflows now import the modules on the build, run and catalog paths in the bare environment, with no pytest present, on wheel and sdist. `ci-test-install` covers that across 3.10–3.13 and three operating systems. The module list lives in one place — `python/xorq/tests/bare_install_modules.txt`, driven by `check_bare_imports.py` — read by the test and by all four workflow steps, rather than being repeated as five hand-maintained `python -c "import …"` one-liners. The `bare_install` tests also needed a step of their own: `ci-test.yml` filters on the backend matrix name, `ci-test-library` on `"library or xorq"`, and every other workflow names explicit paths — so nothing selected that marker and the round trip ran only locally. Now wired into the `core` matrix entry, which already has uv and a synced project; the test builds its own wheel and spawns its own isolated environment, so the surrounding dev/test groups don't contaminate what's under test. ## Known limitations Stated rather than left to be discovered: - **`vendor/` is outside the static scan.** Its module-level imports are upstream ibis's and re-vendoring would churn any exclusion list kept here. Not a hole: `xorq.api` imports `vendor/ibis`, so the bare-install layer catches an undeclared import there that is genuinely absent from a bare install. - **The exclusion-minimality guard only catches import roots that match their distribution name.** An aliased one (`sklearn` → `scikit-learn`) would not be flagged as stale, because pre-registering that alias collides with `test_import_root_mapping_has_no_stale_entries`. - **The twelve exclusions are verified non-stale, not verified extras-gated.** Several of those distributions aren't declared in any extra — pre-existing, and out of scope here. - **The `>= '3.13'` floors are open upward**, safe only while `requires-python` caps at `<3.14`. Noted in `pyproject.toml` where the next bump will see it. ## Follow-ups - **0.4.0 on PyPI carries this bug.** Merging fixes `main`; it does not help anyone already installed. Needs a patch release. - **#2269** — unrelated, found while diagnosing CI here: Flight replaces exception messages over ~1–2 KB with an opaque gRPC metadata-size error, so real failures with chained tracebacks surface as transport errors. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent f7948a8 commit a1cf2cd

9 files changed

Lines changed: 443 additions & 0 deletions

File tree

.github/workflows/ci-test-install.yaml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,7 @@ jobs:
111111
mkdir -p test-wheel && cd test-wheel
112112
python -c "import pathlib; print(f'Current working directory: {pathlib.Path.cwd()}')"
113113
python -c "import xorq; print(f'Successfully imported {xorq.__name__} version {xorq.__version__}')"
114+
python "$GITHUB_WORKSPACE/python/xorq/tests/check_bare_imports.py"
114115
cd ..
115116
deactivate
116117
@@ -125,6 +126,7 @@ jobs:
125126
python -m pip install --upgrade pip
126127
python -m pip install dist/*.tar.gz
127128
python -c "import xorq; print(f'Successfully installed xorq {xorq.__version__} from source distribution')"
129+
python "$GITHUB_WORKSPACE/python/xorq/tests/check_bare_imports.py"
128130
deactivate
129131
130132
- name: Test extras installation from wheel

.github/workflows/ci-test-library.yml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,11 +53,15 @@ jobs:
5353
- name: test wheel
5454
run: |
5555
uv run --isolated --no-project -p ${{ matrix.python-version }} --with dist/*.whl -- python -c "import xorq; print(f'Successfully imported {xorq.__name__} version {xorq.__version__}')"
56+
# Kept out of the pytest run below, which supplies `packaging` itself.
57+
uv run --isolated --no-project -p ${{ matrix.python-version }} --with dist/*.whl -- python python/xorq/tests/check_bare_imports.py
5658
uv run --isolated --no-project -p ${{ matrix.python-version }} --with dist/*.whl --with pytest --with pytest-cov -- pytest --import-mode=importlib --cov --cov-report=xml -m "library or xorq"
5759
5860
- name: test source distribution
5961
run: |
6062
uv run --isolated --no-project -p ${{ matrix.python-version }} --with dist/*.tar.gz -- python -c "import xorq; print(f'Successfully imported {xorq.__name__} version {xorq.__version__}')"
63+
# Kept out of the pytest run below, which supplies `packaging` itself.
64+
uv run --isolated --no-project -p ${{ matrix.python-version }} --with dist/*.tar.gz -- python python/xorq/tests/check_bare_imports.py
6165
uv run --isolated --no-project -p ${{ matrix.python-version }} --with dist/*.tar.gz --with pytest --with pytest-cov -- pytest --import-mode=importlib --cov --cov-report=xml -m "library or xorq"
6266
6367
- name: Upload coverage reports to Codecov

.github/workflows/ci-test.yml

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,19 @@ jobs:
157157
POSTGRES_PORT: 5432
158158
POSTGRES_DATABASE: ibis_testing
159159

160+
# Builds its own wheel and isolated env, so the dev/test groups installed
161+
# here cannot mask a missing declaration.
162+
- name: bare-install CLI round trip
163+
if: matrix.backend.name == 'core'
164+
timeout-minutes: 10
165+
run: >
166+
uv run --no-sync pytest
167+
--import-mode=importlib
168+
python/xorq/tests/test_bare_install.py
169+
-m bare_install
170+
-v --durations=20
171+
working-directory: ${{ github.workspace }}
172+
160173
- name: Upload coverage reports to Codecov
161174
uses: codecov/codecov-action@v5.5.4
162175
with:

pyproject.toml

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,22 @@ dependencies = [
3333
"pandas>=2.2.3,<3 ; python_version >= '3.10' and python_version < '4.0'",
3434
"atpublic>=5.1",
3535
"parsy>=2",
36+
"packaging>=22",
37+
# Per-interpreter floors so `--resolution lowest-direct` gets a wheel and not
38+
# a source build. Each is the first release with wheels for that Python,
39+
# except numpy on 3.10, where 1.22.4 is pandas' own floor (1.21.3 was the
40+
# first with cp310 wheels). The `>= '3.13'` rows are open upward and only
41+
# safe while requires-python caps at <3.14; raising that cap needs a new row
42+
# per interpreter, or lowest-direct picks a release with no wheel for it.
43+
"numpy>=1.22.4; python_version < '3.11'",
44+
"numpy>=1.23.2; python_version == '3.11'",
45+
"numpy>=1.26.0; python_version == '3.12'",
46+
"numpy>=2.1.0; python_version >= '3.13'",
47+
"xxhash>=3.0; python_version < '3.11'",
48+
"xxhash>=3.2; python_version == '3.11'",
49+
"xxhash>=3.4; python_version == '3.12'",
50+
"xxhash>=3.5; python_version >= '3.13'",
51+
"pygments>=2.13",
3652
"python-dateutil>=2.8.2",
3753
"pytz>=2022.7",
3854
"sqlglot>=23.4,!=26.32.0,<28.7.0",
@@ -324,6 +340,7 @@ markers = [
324340
"databricks: Databricks tests",
325341
"xorq_datafusion: xorq DataFusion backend tests",
326342
"uv_export: tests that call uv export against the project lockfile",
343+
"bare_install: tests that build the wheel and drive the CLI against declared dependencies only",
327344
"bigquery: BigQuery tests",
328345
]
329346
consider_namespace_packages = true
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
# Modules that must import with only [project].dependencies installed.
2+
# Read by python/xorq/tests/test_bare_install.py and by the bare-environment
3+
# smoke steps in ci-test-library.yml and ci-test-install.yaml.
4+
xorq
5+
xorq.api
6+
xorq.cli
7+
xorq.catalog.cli
8+
xorq.catalog.tui
9+
xorq.ibis_yaml.compiler
10+
xorq.ibis_yaml.packager
11+
xorq.ibis_yaml.pep723
12+
xorq.backends.pandas.executor
13+
xorq.common.utils.dasher
Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
"""Import every module in bare_install_modules.txt; exit non-zero on the first failure.
2+
3+
Run inside an environment holding only [project].dependencies, so it must not
4+
import anything beyond the standard library and xorq itself.
5+
"""
6+
7+
import importlib
8+
import pathlib
9+
import sys
10+
11+
12+
def main():
13+
listing = pathlib.Path(__file__).with_name("bare_install_modules.txt")
14+
lines = (line.strip() for line in listing.read_text().splitlines())
15+
names = [line for line in lines if line and not line.startswith("#")]
16+
if not names:
17+
sys.exit(f"no modules listed in {listing}")
18+
for name in names:
19+
importlib.import_module(name)
20+
print(f"ok {name}")
21+
print(f"imported {len(names)} modules with declared dependencies only")
22+
23+
24+
if __name__ == "__main__":
25+
main()
Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,124 @@
1+
"""Drive the CLI against an environment holding only the declared dependencies.
2+
3+
Covers what the static scan in ``test_declared_dependencies`` cannot: imports
4+
deferred inside a function body, which is most of the packager imports in
5+
``xorq/cli.py``.
6+
"""
7+
8+
import sys
9+
10+
import pytest
11+
12+
from xorq.common.utils.process_utils import subprocess_run
13+
14+
15+
pytestmark = pytest.mark.bare_install
16+
17+
18+
PYTHON_VERSION = f"{sys.version_info.major}.{sys.version_info.minor}"
19+
20+
EXPR_PY = """
21+
import xorq.api as xo
22+
23+
t = xo.memtable({"a": [1, 2, 3]})
24+
expr = t.filter(t.a > 1)
25+
"""
26+
27+
BARE_IMPORT_CHECK = "python/xorq/tests/check_bare_imports.py"
28+
MODULE_LISTING = "python/xorq/tests/bare_install_modules.txt"
29+
30+
31+
def run_bare(wheel, args, cwd):
32+
return subprocess_run(
33+
(
34+
"uv",
35+
"tool",
36+
"run",
37+
"--isolated",
38+
"--python",
39+
PYTHON_VERSION,
40+
"--with",
41+
str(wheel),
42+
*args,
43+
),
44+
cwd=cwd,
45+
text=True,
46+
)
47+
48+
49+
@pytest.fixture(scope="module")
50+
def wheel(root_dir, tmp_path_factory):
51+
dist = tmp_path_factory.mktemp("dist")
52+
returncode, stdout, stderr = subprocess_run(
53+
("uv", "build", "--wheel", "-o", str(dist)),
54+
cwd=root_dir,
55+
text=True,
56+
)
57+
assert returncode == 0, stderr
58+
(wheel,) = dist.glob("*.whl")
59+
return wheel
60+
61+
62+
def test_core_modules_import(wheel, root_dir):
63+
"""The static scan cannot see function-level imports; this can."""
64+
returncode, stdout, stderr = run_bare(
65+
wheel, ("python", BARE_IMPORT_CHECK), cwd=root_dir
66+
)
67+
assert returncode == 0, stderr
68+
69+
70+
def test_module_listing_covers_the_new_declarations(root_dir):
71+
"""Each declaration needs a module that imports it named in the listing.
72+
73+
pygments comes from catalog/tui.py, numpy from backends/pandas/executor.py,
74+
xxhash from common/utils/dasher. dasher is reached incidentally through
75+
xorq.api today, so name it explicitly: coverage should not depend on
76+
xorq.api continuing to import it.
77+
78+
This exercises those import paths; it does not guard the declarations.
79+
Removing numpy, pygments or xxhash from pyproject.toml leaves these tests
80+
green, because each still arrives transitively via pandas, rich/textual and
81+
xorq-dasher. test_declared_dependencies is what fails on removal.
82+
"""
83+
listing = root_dir.joinpath(MODULE_LISTING).read_text()
84+
for module in (
85+
"xorq.catalog.tui",
86+
"xorq.backends.pandas.executor",
87+
"xorq.common.utils.dasher",
88+
):
89+
assert module in listing
90+
91+
92+
def test_build_then_run_round_trip(wheel, tmp_path):
93+
"""`xorq build` succeeded even while `xorq run` was broken, so test both."""
94+
tmp_path.joinpath("expr.py").write_text(EXPR_PY)
95+
96+
returncode, stdout, stderr = run_bare(
97+
wheel,
98+
# not stdout: OTel's ConsoleSpanExporter flushes there at shutdown, after
99+
# the path is printed, whenever OTEL_EXPORTER_CONSOLE_FALLBACK is set.
100+
("xorq", "build", "expr.py", "-e", "expr", "--emit-build-path-to", "path.txt"),
101+
cwd=tmp_path,
102+
)
103+
assert returncode == 0, stderr
104+
build_path = tmp_path.joinpath("path.txt").read_text().strip()
105+
assert tmp_path.joinpath(build_path).exists()
106+
107+
returncode, stdout, stderr = run_bare(
108+
wheel, ("xorq", "run", build_path), cwd=tmp_path
109+
)
110+
assert returncode == 0, stderr
111+
112+
113+
@pytest.mark.parametrize(
114+
"args",
115+
(
116+
("xorq", "--help"),
117+
("xorq", "build", "--help"),
118+
("xorq", "run", "--help"),
119+
("xorq", "catalog", "--help"),
120+
),
121+
)
122+
def test_help_is_reachable(wheel, tmp_path, args):
123+
returncode, stdout, stderr = run_bare(wheel, args, cwd=tmp_path)
124+
assert returncode == 0, stderr

0 commit comments

Comments
 (0)