More miscellaneous cuDF Python testing cleanups - #22713
Conversation
|
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
WalkthroughRemoves the ChangesTesting Infrastructure Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ 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.
Actionable comments posted: 1
🤖 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/cudf/cudf/testing/_utils.py`:
- Around line 75-79: set_random_null_mask_inplace currently removed the seed
parameter which breaks callers; restore a seed=None parameter in the function
signature and mark it as deprecated: accept seed (default None), if seed is not
None create rng = np.random.default_rng(seed) else use rng =
np.random.default_rng() so behavior is deterministic only when seed is provided;
emit a warnings.warn(..., DeprecationWarning) when seed is used to indicate it
will be removed in a future release and update the function docstring to mention
the deprecated seed argument; keep the rest of the logic (mask generation with
rng.choice) unchanged and reference the function name
set_random_null_mask_inplace in the change.
🪄 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: CHILL
Plan: Enterprise
Run ID: 597e459e-c2ab-4e90-a659-1620c6ca20cf
📒 Files selected for processing (2)
python/cudf/cudf/testing/_utils.pypython/cudf/cudf/tests/test_doctests.py
💤 Files with no reviewable changes (1)
- python/cudf/cudf/tests/test_doctests.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/cudf_polars/tests/conftest.py`:
- Around line 409-410: The ThreadPoolExecutor created in the `with
concurrent.futures.ThreadPoolExecutor() as executor:` block is being returned
while the context manager immediately shuts it down; change the `return
executor` to `yield executor` so the executor stays alive for the duration of
the test (i.e., use `yield` in the fixture instead of `return`), ensuring
`executor.submit()` can be called, and keep the surrounding `with` so shutdown
happens after the fixture completes.
🪄 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: CHILL
Plan: Enterprise
Run ID: 7cce789e-d9dd-46a7-8eef-fa5d7dedc795
📒 Files selected for processing (1)
python/cudf_polars/tests/conftest.py
|
/merge |
Description
FutureWarningfrom Python doctestsThreadPoolExecutorinconftest.pyis eventually shut downChecklist