Skip to content

TEST: Merge import subprocess checks - #24010

Merged
rapids-bot[bot] merged 3 commits into
NVIDIA:mainfrom
vyasr:codex/merge-import-subprocess-checks
Sep 8, 2026
Merged

TEST: Merge import subprocess checks#24010
rapids-bot[bot] merged 3 commits into
NVIDIA:mainfrom
vyasr:codex/merge-import-subprocess-checks

Conversation

@vyasr

@vyasr vyasr commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Description

Merge the two isolated import subprocess checks. The existing no-device subprocess now also asserts that importing cuDF does not import pyarrow._s3fs.

Both contracts are still verified after the same cold, isolated cuDF import, so this removes only a redundant Python startup and not any assertion.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@vyasr
vyasr requested a review from a team as a code owner September 6, 2026 23:41
@vyasr vyasr added tests Unit testing for project Python Affects Python cuDF API. Performance Performance related issue labels Sep 6, 2026
@vyasr
vyasr requested a review from Matt711 September 6, 2026 23:41
@vyasr vyasr added tests Unit testing for project Python Affects Python cuDF API. Performance Performance related issue labels Sep 6, 2026
@coderabbitai

coderabbitai Bot commented Sep 6, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 591024ef-2d71-4826-a99d-46d84aa685d9

📥 Commits

Reviewing files that changed from the base of the PR and between 623f961 and f920201.

📒 Files selected for processing (1)
  • python/cudf/cudf/tests/test_no_device.py

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


📝 Summary

Summary by CodeRabbit

  • Tests
    • Updated import validation to confirm cuDF does not load S3 filesystem components during import.
    • Removed a redundant S3-related import test and cleaned up unused test dependencies.
    • Updated no-device test coverage to verify expected import behavior.
  • Chores
    • Updated copyright notices with current attribution details and “All rights reserved.”

Walkthrough

The pull request removes an obsolete S3 import test and extends the no-device cuDF import test to verify that pyarrow._s3fs is not loaded. It also updates SPDX attributions.

Changes

cuDF import test validation

Layer / File(s) Summary
No-device import validation
python/cudf/cudf/tests/test_no_device.py, python/cudf/cudf/tests/input_output/test_s3.py
The subprocess test checks that pyarrow._s3fs is absent from sys.modules. The obsolete S3 import test and unused imports are removed. SPDX attributions are updated.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Merge Risk: ⚪ Minimal · up to f9202

This change consolidates cuDF import-invariant coverage while preserving the no-device check and verifying that importing cuDF does not load pyarrow._s3fs. No current merge-blocking risk remains.

Suggested reviewers: mroeschke

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: merging two import subprocess checks.
Description check ✅ Passed The description directly explains the merged subprocess checks, retained assertions, and reduced interpreter startup cost.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@vyasr vyasr added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Sep 7, 2026

@bdice bdice left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this test take a lot of time? I agree these can be combined but we are really testing two different things (s3fs and no devices) that now appear under a single test name.

@vyasr

vyasr commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Does this test take a lot of time? I agree these can be combined but we are really testing two different things (s3fs and no devices) that now appear under a single test name.

This is a pretty tiny improvement yeah. I added it since at this point I'm taking every win I can, but it's miniscule on the scale of everything else. These tests take 3-4s each, so combining them shaves that off. Since this PR is already up, I'll just add some helpful comments and rename the test for clarity, but if we find it odd later I'm happy to split it back out.

@vyasr

vyasr commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

/merge

@rapids-bot
rapids-bot Bot merged commit 5471150 into NVIDIA:main Sep 8, 2026
102 checks passed
@github-project-automation github-project-automation Bot moved this from Todo to Done in cuDF Python Sep 8, 2026
@vyasr
vyasr deleted the codex/merge-import-subprocess-checks branch September 8, 2026 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Performance Performance related issue Python Affects Python cuDF API. tests Unit testing for project

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants