test: skip incompatible pylance / python versions in compat_test.py#7811
test: skip incompatible pylance / python versions in compat_test.py#7811westonpace wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughCompatibility test execution adds version-based skips, validates virtual environments against Lance and Python versions, improves Rust panic visibility, and reports subprocess exit or communication failures with additional detail. ChangesCompatibility execution
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@python/python/tests/compat/venv_manager.py`:
- Around line 112-121: The _skip_reason function must include a GitHub issue URL
in the message returned for every compatibility skip. Update the returned skip
text alongside the existing _COMPAT_SKIP reference to include the relevant issue
URL, while preserving the Lance and Python version details.
- Line 318: Update the environment setup in the venv manager to assign
RUST_BACKTRACE to "full" unconditionally rather than using setdefault, ensuring
inherited values such as "0" or "1" are overridden.
- Around line 315-317: Update the environment setup near env and tests_dir so
PYTHONPATH prepends tests_dir while retaining the inherited PYTHONPATH value,
including the appropriate path separator; preserve the existing behavior when no
inherited value is present.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f7e55f7c-362e-4a28-a760-b2f2ad62e61a
📒 Files selected for processing (1)
python/python/tests/compat/venv_manager.py
| def _skip_reason(lance_version: str) -> Optional[str]: | ||
| """Return a pytest.skip message if this lance/Python combo is whitelisted, else None.""" | ||
| py = sys.version_info[:2] | ||
| for max_lance, min_python in _COMPAT_SKIP: | ||
| try: | ||
| if Version(lance_version) <= Version(max_lance) and py >= min_python: | ||
| py_str = f"{py[0]}.{py[1]}" | ||
| return ( | ||
| f"Lance {lance_version} + Python {py_str}: whitelisted skip " | ||
| f"(see _COMPAT_SKIP in venv_manager.py)" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Include a GitHub issue URL in the skip reason.
The new pytest.skip() path emits only an internal code reference. Every skipped Python test must include a GitHub issue URL so this intentional compatibility exception remains traceable.
As per coding guidelines: skipped tests must include a GitHub issue URL.
🤖 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 `@python/python/tests/compat/venv_manager.py` around lines 112 - 121, The
_skip_reason function must include a GitHub issue URL in the message returned
for every compatibility skip. Update the returned skip text alongside the
existing _COMPAT_SKIP reference to include the relevant issue URL, while
preserving the Lance and Python version details.
Source: Coding guidelines
| env = os.environ.copy() | ||
| tests_dir = Path(__file__).parent.parent | ||
| env["PYTHONPATH"] = str(tests_dir) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve inherited PYTHONPATH entries.
This assignment discards any caller or CI-provided import paths. Prepend tests_dir instead of replacing the inherited value.
Proposed fix
env = os.environ.copy()
tests_dir = Path(__file__).parent.parent
- env["PYTHONPATH"] = str(tests_dir)
+ inherited = env.get("PYTHONPATH")
+ env["PYTHONPATH"] = os.pathsep.join(
+ path for path in (str(tests_dir), inherited) if path
+ )📝 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.
| env = os.environ.copy() | |
| tests_dir = Path(__file__).parent.parent | |
| env["PYTHONPATH"] = str(tests_dir) | |
| env = os.environ.copy() | |
| tests_dir = Path(__file__).parent.parent | |
| inherited = env.get("PYTHONPATH") | |
| env["PYTHONPATH"] = os.pathsep.join( | |
| path for path in (str(tests_dir), inherited) if path | |
| ) |
🤖 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 `@python/python/tests/compat/venv_manager.py` around lines 315 - 317, Update
the environment setup near env and tests_dir so PYTHONPATH prepends tests_dir
while retaining the inherited PYTHONPATH value, including the appropriate path
separator; preserve the existing behavior when no inherited value is present.
| env = os.environ.copy() | ||
| tests_dir = Path(__file__).parent.parent | ||
| env["PYTHONPATH"] = str(tests_dir) | ||
| env.setdefault("RUST_BACKTRACE", "full") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Ensure backtraces are actually configured as full.
setdefault() leaves inherited values such as RUST_BACKTRACE=0 or 1 unchanged, so this change does not guarantee the detailed traces described by the PR objective.
Proposed fix
- env.setdefault("RUST_BACKTRACE", "full")
+ env["RUST_BACKTRACE"] = "full"📝 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.
| env.setdefault("RUST_BACKTRACE", "full") | |
| env["RUST_BACKTRACE"] = "full" |
🤖 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 `@python/python/tests/compat/venv_manager.py` at line 318, Update the
environment setup in the venv manager to assign RUST_BACKTRACE to "full"
unconditionally rather than using setdefault, ensuring inherited values such as
"0" or "1" are overridden.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/python/tests/compat/venv_manager.py (1)
434-445: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winInvalidate the stale runner after communication failure.
wait(timeout=2)can leave_subprocessset, and_ensure_subprocess()will keep treating that handle as healthy. Clear it (and terminate/kill if possible) before raising so the next call starts a fresh runner, and add a regression test for the stale-handle path.🤖 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 `@python/python/tests/compat/venv_manager.py` around lines 434 - 445, Update the communication-failure handler around _subprocess in the affected runner method to terminate or kill the existing process when possible, clear _subprocess before raising RuntimeError, and preserve the captured exit/detail reporting. Add a regression test covering a stale handle after wait(timeout=2), verifying the next _ensure_subprocess() call starts a fresh runner.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@python/python/tests/compat/venv_manager.py`:
- Around line 434-445: Update the communication-failure handler around
_subprocess in the affected runner method to terminate or kill the existing
process when possible, clear _subprocess before raising RuntimeError, and
preserve the captured exit/detail reporting. Add a regression test covering a
stale handle after wait(timeout=2), verifying the next _ensure_subprocess() call
starts a fresh runner.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 61e232e9-e7b5-4ac8-8d89-1bf125787f16
📒 Files selected for processing (1)
python/python/tests/compat/venv_manager.py
2 participants
The compat_test runs older versions of pylance to generate old datasets. When running python 3.14 it was failing with a segmentation fault. It turns out the root cause is that older versions of pylance used a pyo3 library that didn't support python 3.14 (likely called functions that were since removed). This fix skips the older compat_test runs on those versions (they will still be tested for the time being on 3.13)
This should help unblock #7728
Summary by CodeRabbit