feat: add pip installation support with optional torch-scatter dependency#28
feat: add pip installation support with optional torch-scatter dependency#28liu-687 wants to merge 2 commits into
Conversation
…ency - Move torch-scatter from core dependencies to optional extra 'scatter' - Add lazy import in grid_int.py with helpful error message for LDOS users - Add conditional pytest skip markers for scatter-dependent tests - Create requirements.txt / requirements-dev.txt / requirements-full.txt - Update CI workflow to install scatter extra - Update README and docs with pip installation instructions BREAKING CHANGE: torch-scatter is no longer installed by default. Users needing LDOS calculations should install with pip install dftio[scatter]
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthrough
ChangesMake torch-scatter an optional extra
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
test/test_grid_int.py (1)
55-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one test for the no-scatter error path.
With
@needs_scatter, the onlyintegrate()test is skipped exactly where this PR changes behavior, so the newImportErrorcontract/message is never asserted. A small test that forces the import to fail and checks the raised message would lock down the new optional-dependency behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_grid_int.py` around lines 55 - 80, The current `test_integrate` coverage only runs when scatter support is available, so it never exercises the new optional-dependency failure path in `SingleGridIntegrator.integrate`. Add a separate test that simulates scatter being unavailable, invokes `integrate()` on `SingleGridIntegrator`, and asserts that an `ImportError` is raised with the expected message. Reuse the existing `SingleGridIntegrator` setup and keep the new test focused on the no-scatter branch so the behavior is locked down even when `@needs_scatter` skips the happy-path test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dftio/op/grid_int.py`:
- Around line 36-42: The ImportError handling in the grid integration path drops
the original exception context. Update the `except ImportError` block in
`grid_int.py` to bind the exception as `err` and re-raise the new `ImportError`
from that original error so the traceback preserves the missing-dependency
cause.
- Around line 37-41: The ImportError message in grid_int.py hardcodes the
torch-2.5.0+cpu wheel hint, which sends users to the wrong install path on CUDA
setups. Update the exception text in the grid integration import guard to prefer
the existing dftio[scatter] / install guide guidance or derive the correct wheel
selector dynamically, and make sure the raised ImportError in the relevant
import block is chained with from err so the original cause is preserved.
In `@docs/installation.md`:
- Around line 52-54: The installation docs currently describe the full extra as
including both scatter and dev tools, but the actual extra definition in
pyproject.toml only maps full to dftio[scatter]. Update the wording in
docs/installation.md so it matches the package metadata, or change the full
extra in pyproject.toml if dev tools are meant to be included; use the full
extra definition and the pip install "dftio[full]" example as the references to
keep both sources consistent.
In `@install.sh`:
- Line 50: The `uv sync` invocation in `install.sh` is passing the Python path
from `$(which python)` unquoted, which can break when the path contains spaces.
Update the existing `uv sync` command to quote the Python path result while
keeping the current lookup approach, so the `--python` argument is always passed
as a single value.
In `@pyproject.toml`:
- Line 39: The `full` extra in pyproject.toml is inconsistent with the README
description because it only includes `dftio[scatter]` while the docs say it also
covers dev tools. Update the dependency definition in the `full` extra to
include `dftio[dev]` as well, or change the README wording to match the actual
`full` set; make the definition and documentation consistent using the `full`
extra symbol and the related `dev`/`scatter` extras.
In `@README.md`:
- Around line 51-53: The README install note for the full extra is inconsistent
with the package definition in pyproject.toml. Update the description next to
the dftio[full] install example to match the actual extra behavior, or change
the full extra definition in pyproject.toml if it is meant to include dev as
well; use the README install snippet and the full extra entry as the main points
of reference.
---
Nitpick comments:
In `@test/test_grid_int.py`:
- Around line 55-80: The current `test_integrate` coverage only runs when
scatter support is available, so it never exercises the new optional-dependency
failure path in `SingleGridIntegrator.integrate`. Add a separate test that
simulates scatter being unavailable, invokes `integrate()` on
`SingleGridIntegrator`, and asserts that an `ImportError` is raised with the
expected message. Reuse the existing `SingleGridIntegrator` setup and keep the
new test focused on the no-scatter branch so the behavior is locked down even
when `@needs_scatter` skips the happy-path test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ba4f618-1b45-4092-a65d-9fee62f3bc67
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.github/workflows/test.ymlREADME.mddftio/op/grid_int.pydocs/installation.mdinstall.shpyproject.tomlrequirements-dev.txtrequirements-full.txtrequirements.txttest/test_grid_int.pytest/test_ldos.py
…stency, and test coverage - Use raise ImportError(...) from err to preserve original exception context - Rework error message to avoid hardcoding CPU wheel URL; prioritize dftio[scatter] extra - Fix full extra to include both scatter and dev, matching documentation - Update requirements-full.txt to include dev dependencies - Quote /Users/liu687/anaconda3/bin/python in install.sh to handle paths with spaces - Add test_integrate_without_scatter_raises_error to cover no-scatter path
1 participant
Summary
Make dftio pip-installable by converting
torch-scatterfrom a hard dependency into an optional extra. This allowspip install dftioto work out of the box for all core functionality.Background
torch-scatterrequires pre-compiled wheels fromdata.pyg.orgthat are not reliably available on PyPI. It is only used in one file (dftio/op/grid_int.py) for LDOS (Local Density of States) calculations. All other features (parsing, data structures, CLI, band plotting) work without it.Changes
dftio/op/grid_int.py: Replace module-leveltorch_scatterimport with lazy import insideintegrate()method, with helpful error message guiding users to installdftio[scatter]pyproject.toml: Movetorch-scatterfrom coredependenciesto[project.optional-dependencies]underscatterextra; addfullanddevextras for pip usersrequirements.txt,requirements-dev.txt,requirements-full.txtfor pip-based workflowstest/test_grid_int.py&test/test_ldos.py: Addneeds_scatterpytest skip marker so scatter-dependent tests are gracefully skipped whentorch-scatteris not installed.github/workflows/test.yml: Updateuv syncto include--extra scatterso scatter tests continue to run in CIinstall.sh: Add--extra scatterflag touv synccommandREADME.md&docs/installation.md: Replace "coming soon" placeholder with actual pip installation instructionsHow to Test
Breaking Change
⚡
torch-scatteris no longer installed by default. Users who need LDOS calculations must install withpip install "dftio[scatter]"or userequirements-full.txt. When callingintegrate()without scatter installed, users will see a helpful error message with installation instructions.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
pipand requirements-file examples.