Skip to content

Conversation

@cpcloud
Copy link
Contributor

@cpcloud cpcloud commented Jan 29, 2026

Small PR to refactor run_in_subprocess to avoid explicit and overly verbose Popen use.

@cpcloud
Copy link
Contributor Author

cpcloud commented Jan 29, 2026

/ok to test

@copy-pr-bot
Copy link

copy-pr-bot bot commented Jan 29, 2026

Auto-sync is disabled for ready for review pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 29, 2026

Greptile Overview

Greptile Summary

This PR successfully refactors run_in_subprocess to use the more modern subprocess.run API instead of the verbose Popen approach. The refactoring includes using text=True to automatically handle string encoding/decoding, eliminating the need for manual .decode() calls in test code.

Key changes:

  • Replaced subprocess.Popen with subprocess.run in support.py:196-217
  • Added text=True parameter to return strings instead of bytes
  • Simplified error handling with CalledProcessError exception
  • Updated test_warning.py:44 to remove .decode() call
  • Modernized test_import.py:54-65 to work with string output using .splitlines()

The refactoring is clean and maintains backward compatibility with all existing callers of run_in_subprocess.

Confidence Score: 5/5

  • This PR is safe to merge - it's a straightforward refactoring with no logic changes
  • The refactoring is well-executed, all affected call sites have been updated correctly, and the change maintains the same behavior while simplifying the code
  • No files require special attention

Important Files Changed

Filename Overview
numba_cuda/numba/cuda/tests/support.py Refactored run_in_subprocess to use subprocess.run with text=True, simplifying code and making it more Pythonic
numba_cuda/numba/cuda/tests/cudapy/test_warning.py Removed .decode() call since run_in_subprocess now returns strings instead of bytes
numba_cuda/numba/cuda/tests/nocuda/test_import.py Updated to use .splitlines() on string output and replaced assertion with simpler assert statement

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

3 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@cpcloud
Copy link
Contributor Author

cpcloud commented Jan 29, 2026

/ok to test

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

3 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
@cpcloud cpcloud enabled auto-merge (squash) January 30, 2026 12:45
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

3 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@cpcloud
Copy link
Contributor Author

cpcloud commented Jan 30, 2026

/ok to test

@cpcloud cpcloud merged commit 6fdf9a5 into NVIDIA:main Jan 30, 2026
104 checks passed
@cpcloud cpcloud deleted the refactor-run-in-subprocess branch January 30, 2026 15:23
gmarkall added a commit to gmarkall/numba-cuda that referenced this pull request Feb 5, 2026
- Add CUDA FP8 type + conversion bindings (E5M2/E4M3/E8M0), HW-accel detection, and comprehensive tests (NVIDIA#686)
- fix: fix boolean return type mismatch in C ABI wrapper (NVIDIA#770)
- Remove unused `rtapi.py`  (NVIDIA#773)
- feat: Add documentation for debugging Numba CUDA programs with CUDA GDB and VSCode (NVIDIA#665)
- Move `CallConv` from `CUDAContext` to `FunctionDescriptor` (NVIDIA#717)
- Generate line info for PHI exporters in terminator block (NVIDIA#756)
- Add `cuda-core` to `oldest` tests (NVIDIA#769)
- build(deps): bump actions/setup-python from 6.1.0 to 6.2.0 in the actions-monthly group across 1 directory (NVIDIA#768)
- Enable apt proxy caching; skip hosted Windows builds (NVIDIA#766)
- Disable automatic review trigger for Greptile (NVIDIA#743)
- test(refactor): clean up `run_in_subprocess` (NVIDIA#762)
- remove super args (NVIDIA#763)
@gmarkall gmarkall mentioned this pull request Feb 5, 2026
kkraus14 pushed a commit that referenced this pull request Feb 5, 2026
- Add CUDA FP8 type + conversion bindings (E5M2/E4M3/E8M0), HW-accel
detection, and comprehensive tests (#686)
- fix: fix boolean return type mismatch in C ABI wrapper (#770)
- Remove unused `rtapi.py`  (#773)
- feat: Add documentation for debugging Numba CUDA programs with CUDA
GDB and VSCode (#665)
- Move `CallConv` from `CUDAContext` to `FunctionDescriptor` (#717)
- Generate line info for PHI exporters in terminator block (#756)
- Add `cuda-core` to `oldest` tests (#769)
- build(deps): bump actions/setup-python from 6.1.0 to 6.2.0 in the
actions-monthly group across 1 directory (#768)
- Enable apt proxy caching; skip hosted Windows builds (#766)
- Disable automatic review trigger for Greptile (#743)
- test(refactor): clean up `run_in_subprocess` (#762)
- remove super args (#763)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants