Skip to content

Validate gpu dispatch flag as bool, fail closed (#2819)#2823

Merged
brendancol merged 3 commits into
mainfrom
issue-2819
Jun 2, 2026
Merged

Validate gpu dispatch flag as bool, fail closed (#2819)#2823
brendancol merged 3 commits into
mainfrom
issue-2819

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2819

What changed

gpu selected the GPU backend by truthiness on both entry points, so a non-bool like gpu="False" was truthy and silently routed to the GPU path instead of raising. bool is also an int subclass, so gpu=1 / gpu=0 slipped through.

  • Added _validate_gpu_arg in _validation.py. It rejects non-bool gpu with TypeError before any branch reads the flag.
  • Read path (open_geotiff): validation runs inside _validate_dispatch_kwargs, so the direct dask / gpu / vrt backends inherit the same check. gpu must be a bool (default False).
  • Write path (to_geotiff): validated at the top of the function, before use_gpu resolution. None stays valid as the auto-detect-from-data sentinel.
  • np.bool_ is accepted alongside Python bool, matching how nodata validation already treats (bool, np.bool_).

Backend coverage

Dispatch-flag validation, backend-agnostic. The read-path check is shared across numpy / dask / cupy / dask+cupy through _validate_dispatch_kwargs; the write-path check runs once at the to_geotiff entry point.

Test plan

  • gpu="False", gpu="True", gpu=1, gpu=0, gpu=1.0, gpu=[True] raise TypeError on both read and write paths
  • gpu=True / gpu=False still dispatch as before; write path gpu=None auto-detects; np.bool_ accepted
  • Read-path validation fires before file I/O (missing-source path still surfaces the gpu TypeError)
  • Full geotiff unit / read / write suite green (3155 passed, 6 GPU skips)

The gpu flag selected the GPU backend by truthiness on both the read
(open_geotiff) and write (to_geotiff) paths. A non-bool like
gpu="False" is truthy, so it routed to the GPU path instead of
raising. bool is also an int subclass, so gpu=1 / gpu=0 slipped past
int-style guards.

Add a shared _validate_gpu_arg helper that rejects non-bool gpu with
TypeError before any branch reads it. The read path requires a bool
(default False); the write path also accepts None, its auto-detect
sentinel. np.bool_ is accepted alongside Python bool, matching the
existing nodata validation.

Read-path validation lives in _validate_dispatch_kwargs, so the direct
dask / gpu / vrt backends inherit the same check.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label Jun 2, 2026
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

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

PR Review: Validate gpu dispatch flag as bool, fail closed (#2819)

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

None.

Nits (optional improvements)

  • xrspatial/geotiff/_validation.py: the read-path call passes gpu straight from open_geotiff's gpu: bool = False default, so the new check also runs for the three direct backends (read_geotiff_dask, read_geotiff_gpu, read_vrt) that route through _validate_dispatch_kwargs. read_geotiff_gpu hardcodes gpu=True into the validator, so its own deprecated string parameter (gpu='strict'/'auto'/'loose', the legacy on_gpu_failure alias) is untouched. A one-line note in the helper docstring saying the validated gpu is the dispatch bool, not that legacy alias, would save a future reader from conflating the two. Optional.

What looks good

  • The check lands before any branch reads gpu on both paths: read path inside _validate_dispatch_kwargs, ahead of the on_gpu_failure and file-like guards; write path at the top of to_geotiff, ahead of the use_gpu resolution. I confirmed no earlier gpu read exists in either function.
  • allow_none models the real signature difference. The read path is bool only; the write path is bool | None with None as the auto-detect sentinel.
  • Accepting np.bool_ next to bool matches the existing nodata validation, so the two type guards stay consistent.
  • Tests cover the important cases: parametrized non-bool rejection on both paths, the None-rejected-on-read vs None-valid-on-write asymmetry, validation firing before file I/O on read, and the positive cases (True / False / np.bool_) still dispatching.

Checklist

  • Algorithm matches reference/paper: n/a (input validation)
  • All implemented backends consistent: yes, read-path check shared via _validate_dispatch_kwargs
  • NaN handling correct: n/a
  • Edge cases covered by tests: yes
  • Dask chunk boundaries handled correctly: n/a
  • No premature materialization or unnecessary copies: confirmed
  • Benchmark exists or not needed: not needed
  • README feature matrix updated: not needed, no new function
  • Docstrings present and accurate: yes

Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

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

PR Review (follow-up): Validate gpu dispatch flag as bool, fail closed (#2819)

The one Nit from the previous review is addressed in 4cfc329: the _validate_dispatch_kwargs docstring now spells out that its gpu parameter is the dispatch bool (type-checked via _validate_gpu_arg), distinct from read_geotiff_gpu's deprecated gpu='strict'/'auto'/'loose' string alias, which never reaches this helper.

No new findings. No Blockers, no Suggestions.

What looks good

  • gpu validation fires before any branch reads the flag on both the read and write paths.
  • np.bool_ accepted alongside bool, consistent with the existing nodata guard.
  • Test coverage spans non-bool rejection on both paths, the read/write None asymmetry, validation-before-IO on read, and the positive True/False/np.bool_ cases.

Checklist

  • Algorithm matches reference/paper: n/a (input validation)
  • All implemented backends consistent: yes
  • NaN handling correct: n/a
  • Edge cases covered by tests: yes
  • No premature materialization or unnecessary copies: confirmed
  • Benchmark exists or not needed: not needed
  • README feature matrix updated: not needed
  • Docstrings present and accurate: yes

@brendancol brendancol merged commit 212d1c4 into main Jun 2, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance PR touches performance-sensitive code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

gpu dispatch flag is not fail-closed: non-bool values like gpu="False" silently select the GPU path

1 participant