Fix command line parsing in the cuda.bindings example helpers - #2546
Open
LeSingh1 wants to merge 1 commit into
Open
Fix command line parsing in the cuda.bindings example helpers#2546LeSingh1 wants to merge 1 commit into
LeSingh1 wants to merge 1 commit into
Conversation
Both helpers unpack enumerate() backwards:
def check_cmd_line_flag(string_ref):
return any(string_ref == i and k < len(sys.argv) - 1 for i, k in enumerate(sys.argv))
enumerate() yields (index, value), so `i` is an int and `k` is a str, but the
body uses `i` as the argument text and `k` as the index. `string_ref == i`
compares str to int and is therefore always False:
$ python -c "import sys; from cuda.bindings._example_helpers import *; \
print(check_cmd_line_flag('device='), get_cmd_line_argument_int('device='))" device= 3
False 0
check_cmd_line_flag() always returns False and get_cmd_line_argument_int()
always returns 0, so every command line option in the examples is silently
ignored: device=, wA=, hA=, wB=, hB=, kernel=, help, ? and
use_generic_memory, via helper_cuda.find_cuda_device(),
find_cuda_device_drv(), global_to_shmem_async_copy.py,
simple_zero_copy.py and stream_ordered_allocation.py. The dead branch would
not have worked either: `k < len(sys.argv) - 1` is str < int (TypeError) and
`sys.argv[k + 1]` indexes with a str.
Alongside the unpacking:
- check_cmd_line_flag() no longer requires a following argument. That
condition belongs to the value lookup; requiring it would keep `help` and
`?` broken whenever they are the last argument, which is the normal way to
pass them.
- Both helpers skip sys.argv[0], matching the C samples' helper_string.h,
which scans from argv[1].
- get_cmd_line_argument_int() returns an int, as its name says and as its
callers require: helper_cuda.find_cuda_device() passes the result straight
to cudaSetDevice(), and find_cuda_device_drv() to cuDeviceGet(). Returning
sys.argv[k + 1] unchanged would hand those APIs a str. This is also the
only value the function has ever actually returned, since the literal 0
fallback was the sole reachable path.
Adds cuda_bindings/tests/test_example_helpers.py. Five of its assertions
fail against main.
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Both helpers in
cuda/bindings/_example_helpers/helper_string.pyunpackenumerate()backwards:
enumerate()yields(index, value), soiis anintandkis astr— but thebody uses
ias the argument text andkas the index.string_ref == icomparesstrto
intand is therefore alwaysFalse.So
check_cmd_line_flag()always returnsFalseandget_cmd_line_argument_int()alwaysreturns
0. Every command line option in the examples is silently ignored —device=,wA=,hA=,wB=,hB=,kernel=,help,?,use_generic_memory— reachinghelper_cuda.find_cuda_device()/find_cuda_device_drv(),3_CUDA_Features/global_to_shmem_async_copy.py,0_Introduction/simple_zero_copy.pyand
2_Concepts_and_Techniques/stream_ordered_allocation.py.Note the dead branch would not have worked either:
k < len(sys.argv) - 1isstr < int(
TypeError) andsys.argv[k + 1]indexes with astr. Nothing here has ever run.Three judgement calls, since "the current behaviour" is not a guide
Because both functions are constant today, any fix defines new behaviour. I kept it as
close to the evident intent — and to the C samples'
helper_string.h— as I could:check_cmd_line_flag()no longer requires a following argument. That conditionbelongs to the value lookup. Keeping it would leave
helpand?broken whenever theyare the last argument, which is the normal way to pass them.
sys.argv[0], matchingcheckCmdLineFlag()in the C samples,which scans from
argv[1].get_cmd_line_argument_int()returns anint, as its name says and as its callersrequire:
helper_cuda.find_cuda_device()passes the result straight tocudaSetDevice()andfind_cuda_device_drv()tocuDeviceGet(). Returningsys.argv[idx + 1]unchanged would hand those APIs astr— i.e. fixing only theunpacking would turn a silent no-op into a
TypeError. Anintis also the only valuethis function has ever actually returned, since the literal
0fallback was the solereachable path.
Happy to change any of these if you'd prefer different semantics.
Tests
New
cuda_bindings/tests/test_example_helpers.py— puresys.argvmonkeypatching, no GPUor toolkit needed. Five of its assertions fail against
upstream/main(flag present,trailing boolean flag, flag in the middle of a longer argv, and both value lookups); the
program-name and not-found cases pass either way and are there to pin the new edges down.
tests/test_examples.pyruns each example in a subprocess as[sys.executable, example],so
sys.argv[1:]is empty there and no flag can accidentally match under pytest.Verified against
upstream/mainand with the change;ruff checkandruff format --checkare clean.