Fix type annotations in pylibucdf hybrid scan - #22118
Conversation
This changes the `list[Span]` type annotations in the pylibucdf hybrid scan module to `Sequence[Span]`. It's tested by adding type annotations to the hybrid scan tests, which surfaced the error reported in the original issue. Closes NVIDIA#22117
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test 7f6dc45 |
mroeschke
left a comment
There was a problem hiding this comment.
non-blocking: The docs in python/pylibcudf/pylibcudf/io/experimental/hybrid_scan.pyx generally still state list[Span] so it would be nice to update those.
Also the Cython annotations do use list for these arguments, but I don't think this annotation change directly conflicts with this new annotation
I'll look into this. I wasn't sure offhand whether cython does something special for |
vyasr
left a comment
There was a problem hiding this comment.
It does matter for Cython. If you type a function in Cython with a built-in type like list, it will strictly validate at runtime, so Sequence and list are not equivalent.

I think your best option here is to use a cast instead. Either that, or change the type annotation in the pyi to be a generic list rather than list[T] so that you give up verification of the type inside the list and just validate the top level of the list.
Per vyasr's review, Cython validates list at runtime, so Sequence is not equivalent. Change the .pyi annotations from Sequence[Span] to bare list to preserve Cython's runtime behavior while avoiding mypy invariance issues with list[T]. Also update docstrings in the .pyx file to match (per mroeschke's suggestion).
|
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 (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughSeven ChangesHybridScanReader type annotation relaxation
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ❌ 5❌ Failed checks (5 warnings)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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/pylibcudf/tests/io/test_experimental_hybrid_scan.py (1)
671-708:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing type annotations on test function parameters and return type.
The test function
test_hybrid_scan_construct_row_group_passes(lines 671–708) lacks type annotations on its parameters and return type, while all other test functions in this module have been explicitly annotated. This is inconsistent with the PR objective to add explicit type annotations to all test functions.🔧 Proposed fix
def test_hybrid_scan_construct_row_group_passes( - simple_hybrid_scan_reader, - simple_parquet_options, -): + simple_hybrid_scan_reader: HybridScanReader, + simple_parquet_options: plc.io.parquet.ParquetReaderOptions, +) -> None:🤖 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/pylibcudf/tests/io/test_experimental_hybrid_scan.py` around lines 671 - 708, The test function `test_hybrid_scan_construct_row_group_passes` is missing type annotations on its parameters and return type annotation. Add type annotations to the function parameters `simple_hybrid_scan_reader` and `simple_parquet_options`, and add a return type annotation (which should be `None` for test functions). This aligns the function with the type annotation standards applied to other test functions in the module.
🤖 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/pylibcudf/tests/io/test_experimental_hybrid_scan.py`:
- Around line 671-708: The test function
`test_hybrid_scan_construct_row_group_passes` is missing type annotations on its
parameters and return type annotation. Add type annotations to the function
parameters `simple_hybrid_scan_reader` and `simple_parquet_options`, and add a
return type annotation (which should be `None` for test functions). This aligns
the function with the type annotation standards applied to other test functions
in the module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b792fc51-54b3-4ea9-a0e8-f1347d723bac
📒 Files selected for processing (3)
python/pylibcudf/pylibcudf/io/experimental/hybrid_scan.pyipython/pylibcudf/pylibcudf/io/experimental/hybrid_scan.pyxpython/pylibcudf/tests/io/test_experimental_hybrid_scan.py
Update copyright headers to canonical form required by rapidsai/pre-commit-hooks v1.5.1. Add [tool.cython-lint] config to pyproject.toml to accommodate longer copyright lines.
|
|
||
| [tool.cython-lint] | ||
| max-line-length = 120 |
There was a problem hiding this comment.
This change is necessary because of #22905. The copyright notice used by the latest version of our copyright hook (rapidsai/pre-commit-hooks#120) is too long for cython-lint, but this is the first PR that is using that hook. We already ignore E501 for ruff in Python code (although in that case we're better off because ruff format will still try to fix the line lengths) so I'm accepting this change as well for now. We'll need to look at the copyright and see if there's anything we can do to improve this situation going forward.
|
/merge |
Description
This changes the
list[Span]type annotations in the pylibucdf hybrid scan module toSequence[Span]. It's tested by adding type annotations to the hybrid scan tests, which surfaced the error reported in the original issue:Closes #22117