Add pre-commit check for stale cudf_polars version-compat flags - #23133
Add pre-commit check for stale cudf_polars version-compat flags#23133nethum529 wants to merge 4 commits into
Conversation
cudf_polars/utils/versions.py defines POLARS_VERSION_LT_* flags that gate behavior on the installed polars version. Once the minimum polars version pinned in cudf_polars/pyproject.toml passes one of these thresholds, the flag can never evaluate true again and the code path it guards is dead, but nothing currently reminds maintainers to clean it up when bumping the pin. Add ci/checks/versions_compat.py, a standalone script (no cudf import) that parses the pinned minimum polars version from pyproject.toml and flags any POLARS_VERSION_LT_* threshold that has fallen at or below it. It fails loudly (rather than silently skipping) if it finds a version-compat flag assignment in a form it doesn't recognize, since a missed match would otherwise make the check falsely report "clean". Wire it into .pre-commit-config.yaml as a local script hook, triggered when cudf_polars/pyproject.toml or utils/versions.py change, following the existing cudf-polars-ir-signatures hook as a style precedent. Fixes rapidsai#16736 Signed-off-by: nethum529 <nethumweerasinghe.nw@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds a version-compatibility checker for stale ChangesVersion Compat Checker
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ci/checks/test_versions_compat.py (1)
14-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNote: dynamic module loading assumes
spec_from_file_locationsucceeds.
importlib.util.spec_from_file_locationcan returnNoneif the target file can't be located, which would cause a confusingAttributeErroron_SPEC.loaderrather than a clear failure message. Given this only runs against a known sibling file in the same directory, this is a low-risk edge case.🤖 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 `@ci/checks/test_versions_compat.py` around lines 14 - 19, The dynamic import setup in test_versions_compat.py assumes importlib.util.spec_from_file_location always returns a valid spec, but _SPEC can be None and make _SPEC.loader fail with an unclear AttributeError. Update the module-loading block around _SPEC, versions_compat, and _SPEC.loader.exec_module to explicitly handle a missing spec (or missing loader) with a clear failure before module execution.ci/checks/versions_compat.py (1)
130-135: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBlind
except Exceptionflagged by Ruff (BLE001).The catch-all is intentional here (fail loudly with a message rather than a raw traceback), but Ruff's static analysis flagged it. Since this repo's pre-commit runs Ruff on Python files, this could trip CI lint if
BLErules are selected in the project's ruff config.🔧 Suggested fix: silence with explicit rationale
try: minimum = minimum_polars_version(args.pyproject) stale = find_stale_flags(args.versions_file, minimum) - except Exception as e: + except Exception as e: # noqa: BLE001 - CLI: report any failure, don't crash with a traceback print(f"Error: {e}", file=sys.stderr) return 1🤖 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 `@ci/checks/versions_compat.py` around lines 130 - 135, The try/except in versions_compat.py is intentionally catching all exceptions, but Ruff flags the blind Exception handler in the versions_compat main flow. Update the exception handling around minimum_polars_version and find_stale_flags to explicitly document that this broad catch is intentional, and add the appropriate Ruff suppression on the except block in a localized way so CI lint does not fail while still preserving the user-friendly stderr message and exit code.Source: Linters/SAST tools
🤖 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 `@ci/checks/test_versions_compat.py`:
- Around line 14-19: The dynamic import setup in test_versions_compat.py assumes
importlib.util.spec_from_file_location always returns a valid spec, but _SPEC
can be None and make _SPEC.loader fail with an unclear AttributeError. Update
the module-loading block around _SPEC, versions_compat, and
_SPEC.loader.exec_module to explicitly handle a missing spec (or missing loader)
with a clear failure before module execution.
In `@ci/checks/versions_compat.py`:
- Around line 130-135: The try/except in versions_compat.py is intentionally
catching all exceptions, but Ruff flags the blind Exception handler in the
versions_compat main flow. Update the exception handling around
minimum_polars_version and find_stale_flags to explicitly document that this
broad catch is intentional, and add the appropriate Ruff suppression on the
except block in a localized way so CI lint does not fail while still preserving
the user-friendly stderr message and exit code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b7ce619c-f514-4893-9a75-1e636fe68dca
📒 Files selected for processing (3)
.pre-commit-config.yamlci/checks/test_versions_compat.pyci/checks/versions_compat.py
|
@nethum529 it looks like your new hook is failing to run. Can you please fix it? |
Will do, currently at work. Will have to fix it after 5:00 PM today. |
Signed-off-by: nethum529 <nethumweerasinghe.nw@gmail.com>
Signed-off-by: nethum529 <nethumweerasinghe.nw@gmail.com>
cudf_polars/utils/versions.pydefinesPOLARS_VERSION_LT_*flags that gate behavior on the installed polars version. Once the minimum polars version pinned incudf_polars/pyproject.tomlpasses one of these thresholds, the flag can never evaluate true again and the code path it guards is dead, but nothing currently reminds maintainers to clean it up when bumping the pin.Add
ci/checks/versions_compat.py, a standalone script (no cudf import) that parses the pinned minimum polars version frompyproject.tomland flags anyPOLARS_VERSION_LT_*threshold that has fallen at or below it. It fails loudly (rather than silently skipping) if it finds a version-compat flag assignment in a form it doesn't recognize, since a missed match would otherwise make the check falsely report "clean".Wire it into
.pre-commit-config.yamlas a local script hook, triggered whencudf_polars/pyproject.tomlorutils/versions.pychange, following the existingcudf-polars-ir-signatureshook as a style precedent.Ships with a standalone unit-test module for the checker's own logic.
Fixes #16736