Rename read_geotiff_gpu gpu kwarg to on_gpu_failure (#1560)#1590
Merged
Conversation
The kwarg shared a name with the boolean gpu= on open_geotiff /
to_geotiff / read_vrt but carried a different value space ('auto' or
'strict'). Calling read_geotiff_gpu(path, gpu=True) -- the natural
mental model after using open_geotiff(path, gpu=True) -- raised
"gpu must be 'auto' or 'strict', got True", obscuring the rename and
forcing readers to dig through the source to learn the correct values.
Rename to on_gpu_failure and keep gpu= as a deprecation shim:
- on_gpu_failure: canonical name, same {'auto', 'strict'} value space.
- gpu: still accepted; emits DeprecationWarning that points to the new
name and explains why the rename happened.
- Passing both raises TypeError to avoid silently picking one.
- Validation error message names on_gpu_failure so the rename is
discoverable from the traceback.
Existing test_gpu_strict_fallback_1516.py keeps using gpu='strict' via
the shim (with DeprecationWarning, as expected). One assertion that
hardcoded the old error string is updated to the new wording.
Module docstring updated to reference on_gpu_failure='strict' as the
example.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR resolves API confusion in xrspatial.geotiff.read_geotiff_gpu by renaming its gpu={'auto','strict'} failure-policy keyword to on_gpu_failure, while keeping gpu= as a deprecated alias to preserve backward compatibility (issue #1560).
Changes:
- Rename
read_geotiff_gpu(..., gpu=...)(failure policy) toread_geotiff_gpu(..., on_gpu_failure=...)and add a deprecation shim forgpu=. - Update validation errors to reference
on_gpu_failureto make the rename discoverable from tracebacks. - Add/adjust tests covering the rename, deprecation warning behavior, and updated validation messaging.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
xrspatial/geotiff/__init__.py |
Introduces on_gpu_failure kwarg, keeps gpu= as deprecated alias, updates docs/error messages. |
xrspatial/geotiff/tests/test_gpu_strict_fallback_1516.py |
Updates existing test to expect the new validation error text while suppressing deprecation warnings. |
xrspatial/geotiff/tests/test_gpu_kwarg_rename_1560.py |
Adds regression tests for the rename/deprecation/type error and improved validation messaging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1883
to
+1887
| # refuse rather than silently picking one. | ||
| if on_gpu_failure != 'auto': | ||
| raise TypeError( | ||
| "read_geotiff_gpu: pass either 'on_gpu_failure' or the " | ||
| "deprecated 'gpu' alias, not both.") |
Comment on lines
+55
to
+61
| with warnings.catch_warnings(): | ||
| # Suppress the deprecation noise; we only care that the value | ||
| # passes validation and the call proceeds to the file-read stage | ||
| # (which will fail with a missing-file error, not a ValueError). | ||
| warnings.simplefilter("ignore", DeprecationWarning) | ||
| with pytest.raises((FileNotFoundError, OSError, ValueError)) as exc_info: | ||
| read_geotiff_gpu("/nonexistent.tif", gpu='strict') |
Copilot caught a hole in the both-supplied check: the old guard ``if on_gpu_failure != 'auto'`` only fired when the new kwarg was non-default, so calls like ``on_gpu_failure='auto', gpu='strict'`` (or ``on_gpu_failure='auto', gpu='auto'``) silently accepted the deprecated value, contradicting the documented contract. Use a sentinel for both kwargs so the both-supplied case is rejected regardless of values. Add parametrized regression tests for the three previously-silent value combinations. Also widen the accepted-exceptions tuple in test_gpu_alias_accepts_old_values_without_validation_error to include ImportError, so it stays meaningful in CPU-only CI (where ``import cupy`` raises after validation passes).
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.
Summary
read_geotiff_gpu'sgpukwarg shares a name with the booleangpu=onopen_geotiff/to_geotiff/read_vrtbut accepts{'auto', 'strict'}— it controls failure policy on the GPU path, not whether to use the GPU. Callingread_geotiff_gpu(path, gpu=True)raised the unhelpfulValueError: gpu must be 'auto' or 'strict', got True.Rename to
on_gpu_failure, keepgpu=as a deprecation shim:on_gpu_failure='auto' | 'strict'is the canonical name.gpu=still works, emitsDeprecationWarningthat names the new kwarg and explains the rename.TypeError.on_gpu_failureso the rename is discoverable from the traceback.Closes #1560.
Test plan
xrspatial/geotiff/tests/test_gpu_kwarg_rename_1560.pycover: invalid value, deprecation warning ongpu=, TypeError when both passed,gpu=Trueno longer raises misleading "got True" errortest_gpu_strict_fallback_1516.pystill passes — usesgpu='strict'via the shim (emits DeprecationWarning as expected)xrspatial/geotiff/tests/suite: 357 passing (one matplotlib palette test fails identically onmain— unrelated)