[feat] Support Python 3.13 and Torch 2.12 dependency installs#344
Conversation
|
This advisory review plan was generated from changed file names using trusted base-branch code. DeePTB PR Review Plan / DeePTB PR 审查计划Risk / 风险等级: Medium (中) · Changed files / 变更文件: 15 Why / 风险来源
Recommended Review / 建议审查重点
Detailed risk areas
Human review focus
Local commands and hold conditionsSuggested local commands:
Hold conditions:
Advisory only. / 仅作为审查辅助。 |
📝 WalkthroughWalkthroughThis PR rewrites ChangesDependency Compatibility Tooling and Installer Overhaul
Sequence Diagram(s)sequenceDiagram
participant User
participant install.sh
participant uv
participant PyG as PyG Wheel Index
participant Verify as Post-Install Verification
User->>install.sh: ./install.sh [cpu|auto|gpu|cu128|cu130] [--extra ...] [--test]
install.sh->>install.sh: Enforce Python 3.10-3.13
install.sh->>install.sh: Detect CUDA via nvidia-smi, map backend
install.sh->>install.sh: Pin torch / torch-scatter / torch-geometric versions
install.sh->>PyG: Compute find-links URL for backend
install.sh->>uv: Create .venv
install.sh->>uv: pip install -e with --overrides --only-binary constraints
uv-->>install.sh: Packages installed
install.sh->>Verify: Read torch-scatter WHEEL metadata
Verify-->>install.sh: Platform wheel confirmed (or fail)
install.sh->>Verify: python -c "torch.cuda.is_available()"
Verify-->>install.sh: CUDA available (or fail for GPU backend)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🧹 Nitpick comments (1)
tools/compat/test_matrix.py (1)
246-264: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLocal
scatter_versionshadows the module-levelscatter_version()function.The local variable on Line 246 reuses the name of the function defined at Line 74. It works today because the function isn't called later in
run_job, but it's a refactoring hazard. Consider renaming to e.g.scatter_installed_version.🤖 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 `@tools/compat/test_matrix.py` around lines 246 - 264, The local variable in run_job is shadowing the module-level scatter_version() helper, which can create refactoring hazards and confusion. Rename the local scatter_version binding in the torch-scatter package probe to a distinct name such as scatter_installed_version, and update the subsequent result["torch_scatter_binary_wheel"] assignment to use the new identifier while leaving the scatter_version() function unchanged.
🤖 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 `@install.sh`:
- Around line 82-85: The backend selection in select_backend is returning torch
backend values like cu121, cu124, and cu132 that are later passed directly into
uv pip install --torch-backend, but uv only accepts a limited set of backend
names. Update the backend mapping so the CUDA 12.1/12.4/13.2 cases translate to
a uv-supported backend before installation, while preserving the existing
PyG-specific label used by find_links_url; use select_backend and the install
path that builds the uv command to apply the fix.
In `@tools/compat/test_matrix.py`:
- Line 270: The test command selection in main() is treating an explicit empty
list from --test-command '' as if no value was provided, so it falls back to
matrix["default_test_command"]. Update the logic around args.test_command and
matrix["default_test_command"] to distinguish None from an intentionally empty
list, and keep the skip path when --test-command is set to an empty string while
preserving the default only when no override is supplied.
---
Nitpick comments:
In `@tools/compat/test_matrix.py`:
- Around line 246-264: The local variable in run_job is shadowing the
module-level scatter_version() helper, which can create refactoring hazards and
confusion. Rename the local scatter_version binding in the torch-scatter package
probe to a distinct name such as scatter_installed_version, and update the
subsequent result["torch_scatter_binary_wheel"] assignment to use the new
identifier while leaving the scatter_version() function unchanged.
🪄 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: bca19d0d-5b14-46d7-a7df-5186e83c08f1
📒 Files selected for processing (9)
docs/maintenance/dependency_compatibility.mddocs/maintenance/dependency_upgrade_test_plan.mddptb/tests/test_energy_feast.pydptb/tests/test_feast_wrapper.pydptb/tests/test_pardisoeig.pyinstall.shpyproject.tomltools/compat/matrix.jsontools/compat/test_matrix.py
| elif cuda >= (12, 4): | ||
| print("cu124") | ||
| elif cuda >= (12, 1): | ||
| print("cu121") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🌐 Web query:
What values does uv's uv pip install --torch-backend flag accept in the latest uv release?
💡 Result:
The --torch-backend flag in uv accepts auto as a special value for automatic detection, or specific CUDA/ROCm version strings (e.g., cu130, cu121, cu118) corresponding to the PyTorch index variants [1]. When set to auto, uv queries the system for installed CUDA drivers, AMD GPU versions, or Intel GPU presence and automatically selects the most compatible PyTorch index for packages like torch and torchvision [1]. If no supported GPU is detected, it defaults to the CPU-only index [1]. Alternatively, you can manually specify a target backend to force the use of a specific PyTorch index (e.g., --torch-backend=cu130) [1]. This flag is currently available specifically within the uv pip interface [1]. Functionality can also be controlled using the UV_TORCH_BACKEND environment variable [1].
Citations:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files install.sh
wc -l install.sh
sed -n '70,230p' install.sh
rg -n "torch-backend|cuda_to_backend|cu121|cu124|cu132|find_links_url" install.shRepository: deepmodeling/DeePTB
Length of output: 5616
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files install.sh
wc -l install.sh
sed -n '70,230p' install.sh
rg -n "torch-backend|cuda_to_backend|cu121|cu124|cu132|find_links_url" install.shRepository: deepmodeling/DeePTB
Length of output: 5616
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files install.sh
wc -l install.sh
sed -n '70,230p' install.sh
rg -n "torch-backend|cuda_to_backend|cu121|cu124|cu132|find_links_url" install.shRepository: deepmodeling/DeePTB
Length of output: 5616
🌐 Web query:
Latest uv docs accepted values for uv pip install --torch-backend cu121 cu124 cu126 cu128 cu130 cpu auto rocm6 xpu
💡 Result:
The uv pip install --torch-backend flag is used to select the appropriate PyTorch index for installation [1][2]. Based on official documentation and community-verified technical resources, the accepted values for this option include [2][3]: - auto: Automatically detects the available hardware (CUDA, AMD/ROCm, or Intel/XPU) and selects the most compatible PyTorch index [1][2]. If no compatible GPU is found, it defaults to the CPU-only index [1][2]. - cpu: Selects the CPU-only PyTorch index [2][3]. - cu118: Selects the PyTorch index for CUDA 11.8 [2]. - cu126: Selects the PyTorch index for CUDA 12.6 [2][3]. - cu128: Selects the PyTorch index for CUDA 12.8 [2]. - cu130: Selects the PyTorch index for CUDA 13.0 [1][2]. - rocm6: Selects the PyTorch index for ROCm 6 [2]. - xpu: Selects the PyTorch index for Intel XPU [2]. Important usage notes: - Availability: The --torch-backend flag is currently only supported in uv pip commands [1][2][3]. It does not work with uv lock, uv sync, or uv run [2]. - Environment Variable: You can also set these values using the UV_TORCH_BACKEND environment variable [1][2]. - Preview Status: This feature is considered to be in preview and may change in future releases [3]. Always consult the official uv documentation for the most current information regarding supported versions [1][3].
Citations:
- 1: https://docs.astral.sh/uv/guides/integration/pytorch/
- 2: https://pydevtools.com/handbook/how-to/how-to-install-pytorch-with-uv/
- 3: https://docs.astral.sh/uv/reference/settings/
Use a uv-supported backend name before passing --torch-backend. select_backend can still return cu121, cu124, and cu132, and those values are forwarded directly to uv pip install --torch-backend at line 205. uv only accepts auto, cpu, cu118, cu126, cu128, cu130, rocm6, and xpu, so the 12.1/12.4/13.2 paths will fail. Map those cases to a supported uv backend and keep the separate PyG label for find_links_url.
🤖 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 `@install.sh` around lines 82 - 85, The backend selection in select_backend is
returning torch backend values like cu121, cu124, and cu132 that are later
passed directly into uv pip install --torch-backend, but uv only accepts a
limited set of backend names. Update the backend mapping so the CUDA
12.1/12.4/13.2 cases translate to a uv-supported backend before installation,
while preserving the existing PyG-specific label used by find_links_url; use
select_backend and the install path that builds the uv command to apply the fix.
QG-phy
force-pushed
the
codex/dependency-compat-install
branch
2 times, most recently
from
fe6b0eb to
d1990dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@tools/compat/test_matrix.py`:
- Around line 162-178: The smoke probe in smoke_probe_code only uses CPU
tensors, so GPU smoke jobs never validate the CUDA path for torch_scatter.
Update the probe to create tensors on CUDA when a GPU is available (while
preserving a CPU fallback if needed) and run scatter_add on those CUDA tensors
so the probe exercises the CUDA wheel in GPU jobs. Keep the change localized to
smoke_probe_code and its tensor creation/scatter_add call so the same import and
assertion flow still works.
- Around line 241-268: The version probe in test_matrix.py records
torch_backend.cuda_available but never enforces it, so GPU jobs can pass on
CPU-only execution; update the matrix validation flow around
version_probe_code()/parse_probe_json() to assert CUDA is actually available for
+cu* jobs, and fail early before proceeding to the torch-scatter binary wheel
and import-and-scatter-smoke steps when cuda_available is false.
- Around line 211-275: The matrix subprocesses in run_job() are inheriting
DPTB_TEST_MKL_FEAST from the parent environment, which can accidentally
re-enable the native FEAST path. Build an explicit child environment before
invoking run() for the venv/install/version-probe/tests steps, unset
DPTB_TEST_MKL_FEAST by default, and only preserve it when a specific job
configuration explicitly opts in. Keep the change localized around run_job(),
the install_cmd/run calls, and the job/environment setup used for spawned
compatibility jobs.
🪄 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: 3d57668c-a05d-4e48-b655-273e8ddc9052
📒 Files selected for processing (11)
.github/workflows/unit_test.ymldocs/maintenance/dependency_compatibility.mddocs/maintenance/dependency_upgrade_test_plan.mddptb/tests/test_energy_feast.pydptb/tests/test_feast_wrapper.pydptb/tests/test_pardisoeig.pyinstall.shpyproject.tomltools/compat/matrix.jsontools/compat/test_matrix.pyut.sh
💤 Files with no reviewable changes (1)
- .github/workflows/unit_test.yml
✅ Files skipped from review due to trivial changes (1)
- docs/maintenance/dependency_compatibility.md
🚧 Files skipped from review as they are similar to previous changes (6)
- tools/compat/matrix.json
- dptb/tests/test_energy_feast.py
- dptb/tests/test_pardisoeig.py
- dptb/tests/test_feast_wrapper.py
- pyproject.toml
- install.sh
QG-phy
force-pushed
the
codex/dependency-compat-install
branch
2 times, most recently
from
98be238 to
0aa11ca
Compare
QG-phy
force-pushed
the
codex/dependency-compat-install
branch
from
0aa11ca to
23f10af
Compare
QG-phy
changed the title
QG-phy
changed the title
QG-phy
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/gpu_compat.yml (2)
30-33: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDisable credential persistence on the self-hosted runner checkout.
On a long-lived self-hosted GPU runner,
actions/checkoutpersists the auth token in.git/configby default, leaving it on disk for later steps/jobs. Since this workflow doesn't push, setpersist-credentials: false.🔒 Proposed fix
- name: Checkout uses: actions/checkout@v6 with: fetch-depth: 0 + persist-credentials: false🤖 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 @.github/workflows/gpu_compat.yml around lines 30 - 33, The Checkout step in the gpu_compat workflow currently leaves the GitHub token persisted on the self-hosted runner; update the actions/checkout usage to disable credential persistence by setting persist-credentials to false. Use the Checkout step in the workflow as the target for this change so the token is not written to .git/config for later steps or jobs.Source: Linters/SAST tools
31-31: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valuePin
actions/checkoutto a commit SHA.A blanket policy flags the floating
@v6tag. Pin to the full commit hash (optionally with a# v6.x.xcomment) for supply-chain integrity.🤖 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 @.github/workflows/gpu_compat.yml at line 31, The GPU compatibility workflow is using the floating actions/checkout@v6 reference, which should be pinned for supply-chain integrity. Update the checkout step in the workflow to use the full commit SHA for actions/checkout instead of the version tag, and optionally keep a brief v6.x.x comment for readability.Source: Linters/SAST tools
tools/compat/test_matrix.py (1)
187-193: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
parse_probe_jsonbrace-scanning is fragile if surrounding output contains braces.
run()merges stderr into stdout, so warnings printed before/after the JSON block (e.g. Torch/PyG deprecation messages) can contain{/}.find("{")/rfind("}")would then span past the real JSON object and fail to parse. A delimited sentinel around the JSON payload inversion_probe_code()would make extraction deterministic.♻️ Sentinel-based extraction
In
version_probe_code():-print(json.dumps(result, indent=2, sort_keys=True)) +print("<<<PROBE_JSON>>>") +print(json.dumps(result, indent=2, sort_keys=True)) +print("<<<END_PROBE_JSON>>>")In
parse_probe_json:- start = output.find("{") - end = output.rfind("}") - if start == -1 or end == -1 or end < start: - raise ValueError("probe output did not contain a JSON object") - return json.loads(output[start : end + 1]) + marker_start = output.find("<<<PROBE_JSON>>>") + marker_end = output.find("<<<END_PROBE_JSON>>>") + if marker_start == -1 or marker_end == -1: + raise ValueError("probe output did not contain a JSON object") + payload = output[marker_start + len("<<<PROBE_JSON>>>") : marker_end] + return json.loads(payload)🤖 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 `@tools/compat/test_matrix.py` around lines 187 - 193, `parse_probe_json` currently scans for the first and last brace, which can break when `run()` merges warning output that also contains braces. Update `version_probe_code()` to wrap the JSON payload with unique sentinel markers, then change `parse_probe_json` to extract only the text between those markers before calling `json.loads`. Use the existing `version_probe_code()` and `parse_probe_json` symbols to keep the extraction deterministic even when stderr warnings are present.Source: Linters/SAST tools
🤖 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 @.github/workflows/gpu_compat.yml:
- Around line 67-70: The Run pytest step in the workflow is vulnerable to
shell/template injection because `pytest_mark` is interpolated directly into the
`run` command. Update the `Run pytest` step to pass `inputs.pytest_mark` through
an environment variable and reference that variable inside the shell command
instead of using direct workflow expression expansion. Keep the fix localized to
the `Run pytest` job step in `gpu_compat.yml`, preserving the existing
`inputs.run_pytest` condition while ensuring the marker value is treated as
plain data.
---
Nitpick comments:
In @.github/workflows/gpu_compat.yml:
- Around line 30-33: The Checkout step in the gpu_compat workflow currently
leaves the GitHub token persisted on the self-hosted runner; update the
actions/checkout usage to disable credential persistence by setting
persist-credentials to false. Use the Checkout step in the workflow as the
target for this change so the token is not written to .git/config for later
steps or jobs.
- Line 31: The GPU compatibility workflow is using the floating
actions/checkout@v6 reference, which should be pinned for supply-chain
integrity. Update the checkout step in the workflow to use the full commit SHA
for actions/checkout instead of the version tag, and optionally keep a brief
v6.x.x comment for readability.
In `@tools/compat/test_matrix.py`:
- Around line 187-193: `parse_probe_json` currently scans for the first and last
brace, which can break when `run()` merges warning output that also contains
braces. Update `version_probe_code()` to wrap the JSON payload with unique
sentinel markers, then change `parse_probe_json` to extract only the text
between those markers before calling `json.loads`. Use the existing
`version_probe_code()` and `parse_probe_json` symbols to keep the extraction
deterministic even when stderr warnings are present.
🪄 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: 8c6c4550-78f8-4b31-939e-9c2686061fb9
📒 Files selected for processing (15)
.github/workflows/gpu_compat.yml.github/workflows/unit_test.ymlREADME.mddocs/maintenance/dependency_compatibility.mddocs/maintenance/dependency_upgrade_test_plan.mddocs/quick_start/easy_install.mddptb/tests/test_energy_feast.pydptb/tests/test_export.pydptb/tests/test_feast_wrapper.pydptb/tests/test_pardisoeig.pyinstall.shpyproject.tomltools/compat/matrix.jsontools/compat/test_matrix.pyut.sh
💤 Files with no reviewable changes (1)
- .github/workflows/unit_test.yml
✅ Files skipped from review due to trivial changes (2)
- dptb/tests/test_pardisoeig.py
- README.md
🚧 Files skipped from review as they are similar to previous changes (7)
- ut.sh
- dptb/tests/test_feast_wrapper.py
- dptb/tests/test_energy_feast.py
- tools/compat/matrix.json
- docs/maintenance/dependency_compatibility.md
- install.sh
- pyproject.toml
| - name: Run pytest | ||
| if: inputs.run_pytest | ||
| run: | | ||
| .venv/bin/python -m pytest dptb/tests -m "${{ inputs.pytest_mark }}" -q |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Template injection: pass pytest_mark through an env var, not direct interpolation.
pytest_mark is a free-text workflow_dispatch input expanded directly into the run: shell, so a malicious marker value can break out of the pytest argument and execute arbitrary commands on the self-hosted GPU runner. Unlike backend (a choice constrained to cu128/cu130), this value is unconstrained. Bind it to an environment variable and reference it as a shell variable so it is never evaluated as workflow expression code.
🔒 Proposed fix
- name: Run pytest
if: inputs.run_pytest
+ env:
+ PYTEST_MARK: ${{ inputs.pytest_mark }}
run: |
- .venv/bin/python -m pytest dptb/tests -m "${{ inputs.pytest_mark }}" -q
+ .venv/bin/python -m pytest dptb/tests -m "$PYTEST_MARK" -q📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Run pytest | |
| if: inputs.run_pytest | |
| run: | | |
| .venv/bin/python -m pytest dptb/tests -m "${{ inputs.pytest_mark }}" -q | |
| - name: Run pytest | |
| if: inputs.run_pytest | |
| env: | |
| PYTEST_MARK: ${{ inputs.pytest_mark }} | |
| run: | | |
| .venv/bin/python -m pytest dptb/tests -m "$PYTEST_MARK" -q |
🧰 Tools
🪛 zizmor (1.26.1)
[error] 70-70: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 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 @.github/workflows/gpu_compat.yml around lines 67 - 70, The Run pytest step
in the workflow is vulnerable to shell/template injection because `pytest_mark`
is interpolated directly into the `run` command. Update the `Run pytest` step to
pass `inputs.pytest_mark` through an environment variable and reference that
variable inside the shell command instead of using direct workflow expression
expansion. Keep the fix localized to the `Run pytest` job step in
`gpu_compat.yml`, preserving the existing `inputs.run_pytest` condition while
ensuring the marker value is treated as plain data.
Source: Linters/SAST tools
1 participant
Summary
This PR updates DeePTB's tested dependency support and installation path around the Python / PyTorch / PyG stack.
Key changes:
3.10-3.13inpyproject.tomland the tested installer; Python 3.14 remains out of scope because TorchScript needs a separate migration.torch>=2.5.1,<=2.12.1and update the tested CPU path to Torch2.12.1with matching PyG /torch-scatterbinary wheels.install.shpath that selects tested CPU/GPU backend combinations and refuses unsupported or source-build fallback paths.Python 3.12/3.13 + torch 2.10.0+cu128 + torch-scatter 2.1.2+pt210cu128;torch 2.12.x+cu130is documented as requiring a newer driver.install.shfor new machines and keepuv syncas the developer-managed path.DPTB_TEST_MKL_FEAST=1because the FEAST native solver issue is separate from dependency compatibility.Validation
Local / remote compatibility checks:
uv lock --dry-runpython tools/compat/test_matrix.py --job py312-torch2121-cpu --job py313-torch2121-cpu-modern-cdeps --no-tests --results-dir tools/compat/results_depcheckpython tools/compat/test_matrix.py --job py313-torch2121-cpu-modern-cdeps --skip-install --results-dir tools/compat/results_depcheck --test-command python -m pytest dptb/tests -m "not slow" -qgpu-py312-torch210-cu128andgpu-py313-torch210-cu128-modern-cdepsFinal CPU
not slowresult:445 passed, 30 skipped, 34 deselected, 249 warnings in 109.67sFinal RTX 5090 GPU
not slowresults:gpu-py312-torch210-cu128:444 passed, 31 skipped, 34 deselectedgpu-py313-torch210-cu128-modern-cdeps:444 passed, 31 skipped, 34 deselectedCI status at the time of this update: