Add optional CuPy backend with auto-detection#36
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
pytalises/pytalises/backends/__init__.py
Lines 59 to 63 in 9f4ced7
Reject unusable CuPy backend when selected explicitly
has_cupy() defines CuPy availability as getDeviceCount() > 0, but get_backend("cupy") only checks whether the class was imported, so environments with CuPy installed but no visible GPU (e.g., CPU-only runners with cupy-cuda12x present) still construct CupyBackend and fail later during array ops with CUDA runtime errors. This should fail fast in backend selection (consistent with has_cupy()) instead of deferring to a harder-to-diagnose runtime crash.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| try: # optional dependency | ||
| from .cupy_backend import CupyBackend | ||
| except Exception: # pragma: no cover - depends on optional runtime |
There was a problem hiding this comment.
Catching Exception while importing cupy_backend masks real implementation bugs (not just missing optional deps) by silently removing the backend from _BACKENDS; once that happens, has_cupy() returns False immediately and CuPy tests guarded by skipif(not has_cupy()) are skipped, allowing broken CuPy backend code to pass CI undetected. Restricting this fallback to import/dependency failures would preserve fail-fast behavior for genuine regressions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d17701a800
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| except Exception: # pragma: no cover - depends on optional runtime | ||
| CupyBackend = None | ||
| else: | ||
| _BACKENDS["cupy"] = CupyBackend |
There was a problem hiding this comment.
Registering cupy in _BACKENDS immediately after module import means get_backend("cupy") succeeds even when has_cupy() is False (e.g., CuPy installed but no visible CUDA device), so users only see a CUDA runtime failure later on first array allocation instead of a clear selection-time error. This also conflicts with the new availability contract used in tests/backend_test.py and will break environments that have CuPy wheels installed on CPU-only runners.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b20a31ba4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| import numpy as np | ||
|
|
||
| import pytalises as pt |
There was a problem hiding this comment.
Running the benchmark exactly as documented (python benchmarks/backend_benchmark.py ...) fails on a fresh checkout because direct script execution sets sys.path to benchmarks/, so pytalises is not importable unless the package was already installed. This makes the new benchmark utility unusable in the common local workflow and contradicts the README invocation.
Useful? React with 👍 / 👎.
| args.json_out.parent.mkdir(parents=True, exist_ok=True) | ||
| args.json_out.write_text(json.dumps(payload, indent=2), encoding="utf-8") | ||
|
|
||
| if args.min_speedup is not None and speedup is not None and speedup < args.min_speedup: |
There was a problem hiding this comment.
The --min-speedup gate only triggers when speedup is not None, so if CuPy is unavailable the script exits successfully even though a minimum speedup was explicitly requested. That silently turns a required performance gate into a no-op in misconfigured environments (e.g., missing GPU/CuPy), which can let CI pass without validating the intended benchmark condition.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8676183f06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| dt=1e-6, | ||
| variables={"Omega": 2.0, "Delta": 1.0}, | ||
| options=pt.PropagationOptions(threads=4), | ||
| options=pt.PropagationOptions(backend="cupy", threads=4), |
There was a problem hiding this comment.
The quickstart now passes PropagationOptions(backend="cupy") while constructing Wavefunction(...) with the default backend ("auto"), so on normal CPU-only installs the wavefunction resolves to NumPy and psi.propagate(...) immediately raises the backend-mismatch ValueError from Propagator. This makes the primary README example fail in the common non-CUDA environment even though CuPy is described as optional.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d67fd22e3f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| mean_seconds=float(np.mean(arr)), | ||
| std_seconds=float(np.std(arr)), |
There was a problem hiding this comment.
When --repeats is 0 (or any input combination that leaves timings empty), np.mean/np.std produce NaN values and the script still exits successfully, which can silently bypass performance gating (e.g., --min-speedup check compares against NaN and does not fail). I reproduced this with python -m benchmarks.backend_benchmark --repeats 0 --warmup 0 --min-speedup 2, which returned exit code 0 while reporting invalid timings.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 317b9fad62
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| git fetch --depth 1 origin ${GITHUB_SHA} | ||
| git checkout --detach ${GITHUB_SHA} |
There was a problem hiding this comment.
ref input when checking out pod code
This step ignores the user-provided workflow_dispatch ref input and always tests ${GITHUB_SHA} in the RunPod VM. If someone dispatches the workflow with a non-empty ref (branch/tag/SHA), the local checkout uses that ref but the pod executes tests on a different revision, which can produce a false green result for code that was never actually validated.
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
RunPod benchmark follow-up (RTX A4500, CuPy 13.6.0) after CI hardening ✅ Workflow runs:
Headline speedups (NumPy/CuPy):
Parity remains tight across sweeps (max abs diff ~1e-14 for density and occupation). Takeaway: small 1D grids are overhead-bound on GPU, but larger grids show clear CuPy gains, including strong wins on free propagation at very large sizes. |
This reverts commit 9d3ac77.
- New gpu_backend.rst covering installation, backend selection, performance characteristics, precision control, and troubleshooting - Add to index.rst table of contents - Update v2_migration.rst with link to GPU docs
- Add .readthedocs.yaml for reproducible RTD builds - Update docs/requirements.txt with all necessary deps (sphinx, sphinx-rtd-theme, nbsphinx, pypandoc_binary) - Suppress ambiguous cross-reference warnings in conf.py (expected due to legacy module duplicating class names) - Tested: sphinx-build -W passes with no warnings
- Add animated diffraction grating GIF as hero visual - Simplify quickstart to 5 lines - Add comparison table vs QuTiP - Collapse detailed examples into <details> blocks - Add citation bibtex block - Fix badges (removed broken workflow, added GPU badge) - Include animation generation scripts in docs/assets/
- Longer propagation (200 frames, more time steps) - Taller grid extent to show diffracted orders separating - Higher initial momentum for clearer diffraction angles
- Remove QuTiP comparison (not really comparable tools) - List specific physics domains where pyTALISES excels - Cleaner, more positive framing
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ba6772bad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| safe_builtins = {"__import__": __import__} | ||
| return eval(expr, {"__builtins__": safe_builtins}, scope) |
There was a problem hiding this comment.
The CuPy evaluator intentionally exposes __import__ and then runs raw eval on user-provided expressions, which means any expression string can execute arbitrary Python (for example through imported modules) when backend="cupy"; this is a real code-execution risk whenever expressions come from configs, notebooks, or other non-hardcoded inputs, and it is materially less safe than the NumPy/numexpr path.
Useful? React with 👍 / 👎.
|
|
||
| return GpuEngine(backend) | ||
|
|
||
| raise ValueError(f"No engine registered for backend '{backend.name}'.") |
There was a problem hiding this comment.
register_backend remains part of the public API, but create_engine now hard-fails for any backend whose name is not exactly "numpy" or "cupy"; as a result, a backend that can be successfully registered and instantiated cannot be used by Wavefunction anymore because initialization now unconditionally calls this factory.
Useful? React with 👍 / 👎.
1 participant
Summary
cupybackend implementation and register it in backend discoverybackend="cupy"/backend="auto"selection paths forWavefunctionbenchmarks/backend_benchmark.py) with optional speedup gating and JSON outputNotes
gpu-cienvironment (RUNPOD_API_KEY,RUNPOD_SSH_PRIVATE_KEYexpected there)Testing
python -m pytest tests/ -v(19 passed, 5 skipped)python benchmarks/backend_benchmark.py --size 64 --steps 5 --repeats 1 --warmup 0