ci: add package sanity check and stop shipping test files (#958)#1261
ci: add package sanity check and stop shipping test files (#958)#1261hassanzafarr wants to merge 1 commit into
Conversation
✅ Deploy Preview for poetic-froyo-8baba7 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughAdds Poetry exclusions for test-only modules, introduces a script that scans wheel and source distribution contents for test files and forbidden pytest imports, and configures a GitHub Actions workflow to build the package and run the checker on pushes and pull requests. Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/package-sanity.yml (1)
12-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider updating pinned GitHub Actions to current major versions.
Both
actions/checkout(v2.7.0) andactions/setup-python(v2.3.4) are several major versions behind current (v4 and v5 respectively). While SHA-pinning ensures reproducibility, these older versions miss security patches and improvements. Updating to the latest major versions (still SHA-pinned) is low-risk and keeps the workflow on supported ground.♻️ Suggested version updates
- uses: actions/checkout@<latest-v4-sha> # v4.2.2 - name: Set up Python uses: actions/setup-python@<latest-v5-sha> # v5.3.0 with: python-version: "3.10"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/package-sanity.yml around lines 12 - 17, Update the pinned actions in the package-sanity workflow: replace actions/checkout v2.7.0 with the current v4 release SHA and actions/setup-python v2.3.4 with the current v5 release SHA, preserving SHA pinning and updating the inline version comments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/package-sanity.yml:
- Around line 12-17: Update the pinned actions in the package-sanity workflow:
replace actions/checkout v2.7.0 with the current v4 release SHA and
actions/setup-python v2.3.4 with the current v5 release SHA, preserving SHA
pinning and updating the inline version comments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ce93f9a-f178-47a5-aa59-f761d40ff99e
📒 Files selected for processing (3)
.github/workflows/package-sanity.ymlpyproject.tomltools/check_package_contents.py
The published wheel and sdist included test modules under qdrant_client/ (qdrant_client/local/tests/* and qdrant_client/hybrid/test_reranking.py), three of which import pytest at module level. That made pytest an effective import-time dependency of the package and led to broken releases where a clean `pip install` failed. Exclude test-only files from the built distribution via the poetry `exclude` key. They stay in the source tree and are still collected by pytest, so nothing changes for local development or CI test runs; they just no longer ship to end users. Add a `package-sanity` CI workflow plus tools/check_package_contents.py that builds the package and fails if any shipped module is a test file or imports pytest, so this cannot silently regress before a release. Fixes qdrant#958
hassanzafarr
force-pushed
the
ci/package-sanity-check
branch
from
e70d81f to
d15fdf3
Compare
There was a problem hiding this comment.
Pull request overview
This PR prevents test-only modules (and accidental pytest import-time dependencies) from being included in published qdrant-client distributions, and adds a CI guard to catch future regressions.
Changes:
- Adds Poetry packaging excludes to prevent
qdrant_client/**/testsandqdrant_client/**/test_*.pyfrom shipping in wheels/sdists. - Introduces a stdlib-only checker script to scan built artifacts for shipped test modules or forbidden imports (
pytest,_pytest). - Adds a GitHub Actions workflow that builds the package and runs the checker on pushes and PRs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tools/check_package_contents.py | New script that inspects built wheels/sdists for test modules and forbidden imports. |
| pyproject.toml | Adds packaging excludes to prevent test code from being included in release artifacts. |
| .github/workflows/package-sanity.yml | New CI job that runs poetry build then validates artifact contents via the new checker. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| exclude = [ | ||
| "qdrant_client/**/tests", | ||
| "qdrant_client/**/test_*.py", | ||
| ] |
| path = Path(target) | ||
| if path.is_dir(): | ||
| dists.extend(sorted(path.glob("*.whl"))) | ||
| dists.extend(sorted(path.glob("*.tar.gz"))) | ||
| elif path.is_file(): | ||
| dists.append(path) | ||
| else: | ||
| raise SystemExit(f"error: no such file or directory: {target}") | ||
| return dists |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/package-sanity.yml (1)
12-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider upgrading pinned action versions.
actions/checkout@v2.7.0andactions/setup-python@v2.3.4are significantly outdated (current versions are v4.x and v5.x respectively). While pinning to commit hashes is good practice, these older versions may miss security patches and performance improvements.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/package-sanity.yml around lines 12 - 15, Upgrade the pinned actions in the workflow: replace the outdated checkout action referenced by the checkout step with a current v4 commit SHA, and replace the setup-python action referenced by the “Set up Python” step with a current v5 commit SHA; update the version comments to match.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/package-sanity.yml:
- Line 12: Update the actions/checkout step in the workflow to set
persist-credentials: false, ensuring the GITHUB_TOKEN is not stored in the
repository’s Git configuration after checkout.
In `@tools/check_package_contents.py`:
- Around line 55-68: _iter_python_files silently accepts unsupported
distribution types, allowing check_distribution to report success without
inspection. Add an explicit error or warning for paths that are neither wheel
files nor .tar.gz archives, using the existing check_distribution/main
error-reporting flow so the command cannot report “passed” for an uninspected
file.
---
Nitpick comments:
In @.github/workflows/package-sanity.yml:
- Around line 12-15: Upgrade the pinned actions in the workflow: replace the
outdated checkout action referenced by the checkout step with a current v4
commit SHA, and replace the setup-python action referenced by the “Set up
Python” step with a current v5 commit SHA; update the version comments to match.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 00b6cf12-b3b2-4ba1-ab46-f1913cc073f6
📒 Files selected for processing (3)
.github/workflows/package-sanity.ymlpyproject.tomltools/check_package_contents.py
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
| name: Check built package contents | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@ee0669bd1cc54295c223e0bb666b733df41de1c5 # v2.7.0 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Set persist-credentials: false on checkout.
The actions/checkout action defaults to persist-credentials: true, which stores the GITHUB_TOKEN in .git/config. While this workflow doesn't upload artifacts, the token isn't needed after checkout, so disabling persistence is a simple security hardening.
🔒️ Proposed fix
- uses: actions/checkout@ee0669bd1cc54295c223e0bb666b733df41de1c5 # v2.7.0
+ with:
+ persist-credentials: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: actions/checkout@ee0669bd1cc54295c223e0bb666b733df41de1c5 # v2.7.0 | |
| - uses: actions/checkout@ee0669bd1cc54295c223e0bb666b733df41de1c5 # v2.7.0 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 12-12: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/package-sanity.yml at line 12, Update the actions/checkout
step in the workflow to set persist-credentials: false, ensuring the
GITHUB_TOKEN is not stored in the repository’s Git configuration after checkout.
Source: Linters/SAST tools
| def _iter_python_files(dist: Path) -> Iterator[tuple[str, str]]: | ||
| """Yield ``(member_path, source)`` for every ``.py`` file in a distribution.""" | ||
| if dist.suffix == ".whl": | ||
| with zipfile.ZipFile(dist) as archive: | ||
| for name in archive.namelist(): | ||
| if name.endswith(".py"): | ||
| yield name, archive.read(name).decode("utf-8", "replace") | ||
| elif dist.name.endswith(".tar.gz"): | ||
| with tarfile.open(dist, "r:gz") as archive: | ||
| for member in archive.getmembers(): | ||
| if member.isfile() and member.name.endswith(".py"): | ||
| handle = archive.extractfile(member) | ||
| if handle is not None: | ||
| yield member.name, handle.read().decode("utf-8", "replace") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Warn on unsupported distribution file types.
_iter_python_files silently yields nothing for files that aren't .whl or .tar.gz. If a user passes such a file directly, check_distribution returns an empty problem list and main reports "passed" without actually inspecting the file, giving a false sense of security.
🛡️ Proposed fix
def _iter_python_files(dist: Path) -> Iterator[tuple[str, str]]:
"""Yield ``(member_path, source)`` for every ``.py`` file in a distribution."""
if dist.suffix == ".whl":
with zipfile.ZipFile(dist) as archive:
for name in archive.namelist():
if name.endswith(".py"):
yield name, archive.read(name).decode("utf-8", "replace")
elif dist.name.endswith(".tar.gz"):
with tarfile.open(dist, "r:gz") as archive:
for member in archive.getmembers():
if member.isfile() and member.name.endswith(".py"):
handle = archive.extractfile(member)
if handle is not None:
yield member.name, handle.read().decode("utf-8", "replace")
+ else:
+ raise SystemExit(f"error: unsupported distribution format: {dist.name}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _iter_python_files(dist: Path) -> Iterator[tuple[str, str]]: | |
| """Yield ``(member_path, source)`` for every ``.py`` file in a distribution.""" | |
| if dist.suffix == ".whl": | |
| with zipfile.ZipFile(dist) as archive: | |
| for name in archive.namelist(): | |
| if name.endswith(".py"): | |
| yield name, archive.read(name).decode("utf-8", "replace") | |
| elif dist.name.endswith(".tar.gz"): | |
| with tarfile.open(dist, "r:gz") as archive: | |
| for member in archive.getmembers(): | |
| if member.isfile() and member.name.endswith(".py"): | |
| handle = archive.extractfile(member) | |
| if handle is not None: | |
| yield member.name, handle.read().decode("utf-8", "replace") | |
| def _iter_python_files(dist: Path) -> Iterator[tuple[str, str]]: | |
| """Yield ``(member_path, source)`` for every ``.py`` file in a distribution.""" | |
| if dist.suffix == ".whl": | |
| with zipfile.ZipFile(dist) as archive: | |
| for name in archive.namelist(): | |
| if name.endswith(".py"): | |
| yield name, archive.read(name).decode("utf-8", "replace") | |
| elif dist.name.endswith(".tar.gz"): | |
| with tarfile.open(dist, "r:gz") as archive: | |
| for member in archive.getmembers(): | |
| if member.isfile() and member.name.endswith(".py"): | |
| handle = archive.extractfile(member) | |
| if handle is not None: | |
| yield member.name, handle.read().decode("utf-8", "replace") | |
| else: | |
| raise SystemExit(f"error: unsupported distribution format: {dist.name}") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/check_package_contents.py` around lines 55 - 68, _iter_python_files
silently accepts unsupported distribution types, allowing check_distribution to
report success without inspection. Add an explicit error or warning for paths
that are neither wheel files nor .tar.gz archives, using the existing
check_distribution/main error-reporting flow so the command cannot report
“passed” for an uninspected file.
Closes #958.
The published wheel/sdist currently ship test modules that live inside the
qdrant_clientpackage. Building1.18.0locally and inspecting the wheel, I see:qdrant_client/local/tests/*(7 files)qdrant_client/hybrid/test_reranking.pyThree of these
import pytestat module level, which is what makespytestan effective import-time dependency of the package and causes the broken releases mentioned in the issue.Root cause:
packages = [{include = "qdrant_client"}]includes the whole tree, and there's noexclude, so anything named like a test that happens to live underqdrant_client/gets packaged.Changes
Stop shipping test-only code — added an
excludeto[tool.poetry]inpyproject.tomlforqdrant_client/**/testsandqdrant_client/**/test_*.py. This only affects the built artifacts; the files stay in the source tree and are still collected/run bypytest, so nothing changes for local dev or CI test runs.Guard against regressions — the issue asks for a CI check, so I added:
tools/check_package_contents.py— stdlib-only, builds nothing itself; it inspects the wheel + sdist and fails if any shipped module is a test file or importspytest/_pytest..github/workflows/package-sanity.yml— runspoetry buildand then the check on push/PR.A check on its own would have gone red immediately because the leak is live today, so both parts are needed together.
Testing
poetry build, then ran the checker against the resultingdist/:qdrant_client/hybrid/fusion.py, theqdrant_client/localimplementation) still ship — 91 Python modules in the wheel, only the test files removed.qdrant_client/hybrid/test_reranking.py, which sits outsidelocal/tests; the broader exclude pattern covers it too.Happy to adjust the exclude patterns or the workflow triggers to match your preferences.