Skip to content

Fix silent GPU transfer failures disabling CPU threading fallback - #176

Open
bhuvan-somisetty wants to merge 2 commits into
OSIPI:mainfrom
bhuvan-somisetty:fix-silent-gpu-fallback-175
Open

Fix silent GPU transfer failures disabling CPU threading fallback#176
bhuvan-somisetty wants to merge 2 commits into
OSIPI:mainfrom
bhuvan-somisetty:fix-silent-gpu-fallback-175

Conversation

@bhuvan-somisetty

Copy link
Copy Markdown
Contributor

Fixes #175

to_gpu() in osipy/common/backend/array_module.py caught any exception from the actual GPU transfer and fell back to NumPy with zero logging or warning. That's already not great, a real CUDA OOM or driver error just vanishes, but BaseFitter.fit_image() (osipy/common/fitting/base.py) makes it worse: use_gpu is decided once before the transfer and never re-checked afterward, so on a silent fallback:

  • Chunk sizing still picks GPU-oriented sizing instead of the CPU chunk_size.
  • use_threading = not use_gpu and ... stays False because use_gpu is still True, so the CPU multi-threaded fallback that would normally compensate is disabled too.

End result: a GPU transfer failure silently degrades fitting to serial, single-threaded CPU execution with nothing telling you why. osipy/common/backend/batch.py already treats an analogous GPU-OOM case as worth a warnings.warn(...), this just brings to_gpu() in line with that existing pattern.

Fix

  • to_gpu() now warns (UserWarning) when the transfer fails, before falling back.
  • fit_image() re-derives use_gpu from the actual array to_gpu() returns (hasattr(..., "__cuda_array_interface__")) instead of trusting the pre-transfer decision, so chunk sizing and the CPU threading fallback reflect what's actually running.

Testing

  • Added TestToGpu::test_transfer_failure_warns_and_falls_back in tests/unit/common/backend/test_array_module.py: mocks a GPU transfer failure and confirms a UserWarning is raised and a usable NumPy array is still returned.
  • Added tests/unit/common/fitting/test_base.py (new file, mirrors the osipy/common/fitting/ package): confirms that when to_gpu() falls back, fit_image() re-enables the CPU threaded path (previously it silently stayed disabled), plus a sanity check that a successful GPU transfer still skips CPU threading as expected.
  • Verified both new tests actually fail against the pre-fix code (reverted the fix locally and re-ran, both failed with the expected assertion errors) before confirming they pass with the fix.
  • Full suite: 778 passed, 40 skipped (GPU/data-dependent), 6 xfailed, 8 failed. Those 8 are the pre-existing PackageNotFoundError CLI failures already tracked/fixed separately in [BUG] osipy CLI crashes on every invocation when package metadata isn't discoverable, not just --version #173 / PR Fix CLI crashing on every invocation when package metadata is missing #174, unrelated to this change (this branch was cut from main before that fix landed).
  • ruff check / ruff format --check clean on all changed/new files.
  • mypy on both changed source files: no new errors introduced (verified by diffing mypy output against main, same 3 pre-existing findings in array_module.py shifted by one line from the added import; fitting/base.py is clean).

to_gpu() caught any exception from the GPU transfer and fell back to
NumPy with no logging or warning at all. BaseFitter.fit_image() made
it worse: use_gpu was decided once before the transfer and never
re-checked, so after a silent fallback, chunk sizing still used
GPU-oriented sizing and the CPU multi-threaded fallback stayed
disabled (use_threading = not use_gpu evaluated False). A real GPU
failure (OOM, driver hiccup) would silently degrade fitting to
serial, single-threaded CPU with no way to tell why.

- to_gpu() now warns (UserWarning) when the transfer fails, matching
  the existing GPU-OOM warning pattern already used in batch.py.
- fit_image() re-derives use_gpu from the actual array returned by
  to_gpu() instead of trusting the pre-transfer decision, so chunk
  sizing and the CPU threading fallback reflect what's actually
  running.

Fixes OSIPI#175
@bhuvan-somisetty

Copy link
Copy Markdown
Contributor Author

cc @ltorres6 @MohamedNasser8 for review

@MohamedNasser8

Copy link
Copy Markdown
Collaborator

Hello @bhuvan-somisetty, thanks for all your work, I'll have a look soon.

@ltorres6 ltorres6 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for catching this silent fallback and your fix attempt.

I think what we actually want here is to throw an error instead of falling back if we
a) have a detected GPU
b) are not using force_cpu, and
c) transfer fails for any reason.

This will also require a change in batch.py since that deliberately falls back to numpy if we fill memory.

Can you make those changes?

Comment thread osipy/common/backend/array_module.py Outdated
except Exception:
# Fallback to NumPy if GPU transfer fails
except Exception as e:
warnings.warn(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please use the logging package to log INFO/WARN/ERRORS.

…r failure

Per review feedback: to_gpu() now raises GPUTransferError instead of
warning and silently returning a NumPy array when a GPU is detected,
force_cpu is not set, and the transfer fails for any reason. Callers
that decided GPU-vs-CPU behavior before the transfer (chunk sizing,
threading) can no longer be left assuming GPU execution that never
happened.

BatchProcessor.map() deliberately wants to keep falling back to CPU
when GPU memory fills mid-run, so it now explicitly catches
GPUTransferError from to_gpu() and continues its existing retry-on-CPU
behavior, while other exceptions from the batch function itself are
still filtered by the existing memory-error heuristic.

Also switches the transfer-failure log from warnings.warn to the
logging package, as requested.
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.

[BUG] to_gpu() silently swallows GPU transfer failures, disabling the CPU threading fallback in fit_image()

3 participants