-
Notifications
You must be signed in to change notification settings - Fork 585
chore: manage CI pinnings in pyproject.toml #5068
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## devel #5068 +/- ##
==========================================
- Coverage 84.33% 84.33% -0.01%
==========================================
Files 709 709
Lines 70435 70435
Branches 3618 3619 +1
==========================================
- Hits 59402 59401 -1
Misses 9867 9867
- Partials 1166 1167 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughThe PR consolidates Python dependency installations across multiple CI workflows by introducing versioned dependency groups in pyproject.toml and updating workflows to use these groups via the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyproject.toml (1)
139-150: Reconcile JAX version specifications across optional-dependencies and dependency-groups.The existing
jaxoptional-dependency requires>=0.4.33, while the newpin_jaxgroup pins==0.5.0. This creates two different dependency resolution paths:
- User installations:
pip install deepmd-kit[jax]→ jax>=0.4.33- CI installations:
pip install --group pin_jax→ jax==0.5.0This version divergence could result in inconsistent test results or behavior between local development and CI. Align these versions or document the intentional split if it serves a specific purpose (e.g., CI pinning for reproducibility while allowing flexible local development).
Also applies to: 173-175
🧹 Nitpick comments (2)
.github/workflows/codeql.yml (1)
46-47: Inconsistent use of retry wrapper across workflows.This workflow uses
uv pip installdirectly (line 47), while other workflows liketest_cc.ymlwrap pip calls withsource/install/uv_with_retry.shfor transient failure resilience. For consistency and reliability, consider using the retry wrapper here as well:uv_with_retry.sh pip install --system --group pin_tensorflow_cpu --group pin_pytorch_cpu --torch-backend cpu..github/workflows/copilot-setup-steps.yml (1)
52-52: Consider using retry wrapper for consistency.For operational resilience across CI workflows, consider wrapping the pip install with
uv_with_retry.sh:uv_with_retry.sh pip install --group pin_tensorflow_cpu --group pin_pytorch_cpu --torch-backend cpu. While this is a development setup workflow (not a build step), the wrapper improves reliability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/build_cc.yml(1 hunks).github/workflows/codeql.yml(1 hunks).github/workflows/copilot-setup-steps.yml(1 hunks).github/workflows/test_cc.yml(1 hunks).github/workflows/test_cuda.yml(1 hunks).github/workflows/test_python.yml(1 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-24T22:42:23.128Z
Learnt from: CR
Repo: deepmodeling/deepmd-kit PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T22:42:23.128Z
Learning: Install backend dependencies (TensorFlow, PyTorch, JAX, or Paddle) before building C++ components, and set TENSORFLOW_ROOT and PYTORCH_ROOT environment variables
Applied to files:
.github/workflows/copilot-setup-steps.yml.github/workflows/test_cuda.yml.github/workflows/codeql.yml.github/workflows/test_cc.yml.github/workflows/test_python.yml.github/workflows/build_cc.yml
📚 Learning: 2025-08-14T07:11:51.357Z
Learnt from: njzjz
Repo: deepmodeling/deepmd-kit PR: 4884
File: .github/workflows/test_cuda.yml:46-46
Timestamp: 2025-08-14T07:11:51.357Z
Learning: As of PyTorch 2.8 (August 2025), the default wheel on PyPI installed by `pip install torch` is CPU-only. CUDA-enabled wheels are available on PyPI for Linux x86 and Windows x86 platforms, but require explicit specification via index URLs or variant-aware installers. For CUDA support, use `--index-url https://download.pytorch.org/whl/cu126` (or appropriate CUDA version).
Applied to files:
.github/workflows/copilot-setup-steps.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (6)
pyproject.toml (1)
161-175: Verify PyTorch version selection for GPU environments.The dependency groups specify
torch~=2.8.0for CPU buttorch~=2.7.0for GPU. This is unusual—typically, the same PyTorch version supports both CPU and GPU modes. Clarify the rationale for this version split to ensure GPU environments receive the correct, compatible version..github/workflows/test_cc.yml (1)
31-33: Clarify JAX installation in CPU-only test workflow.Line 31 installs
--group pin_jaxin a CPU-only workflow (as evidenced by--torch-backend cpu), and line 33 then installs the package with.[cpu,test,lmp,jax]. Confirm whether JAX CPU support is intentional for this workflow or ifpin_jaxshould be removed (since thejaxfeature is already included in line 33's installation). If JAX is meant to be tested on CPU, document this design decision..github/workflows/codeql.yml (1)
46-47: Consider adding TENSORFLOW_ROOT and PYTORCH_ROOT exports after installation.The CodeQL workflow installs backend dependencies (lines 46–47) but does not export
TENSORFLOW_ROOTandPYTORCH_ROOTenvironment variables. Per the learnings, these should be set before building C++ components. Compare this totest_cc.yml(lines 32–33), which exports these variables after installing dependencies. If CodeQL's build script (build_cc.sh, line 58) requires these variables, add the exports..github/workflows/build_cc.yml (1)
38-38: Verify TENSORFLOW_ROOT and PYTORCH_ROOT environment variables.The backend dependencies are installed (line 38), but
TENSORFLOW_ROOTandPYTORCH_ROOTare not explicitly exported before the build step (line 61). According to learnings, these should be set before building C++ components. Check ifbuild_cc.shauto-discovers these paths or if explicit exports are needed (compare totest_cc.ymllines 32–33)..github/workflows/test_cuda.yml (1)
46-46: Clarify duplicate JAX specifications in grouped install.Line 46 installs both
--group pin_jax(which specifiesjax==0.5.0) and the explicit"jax[cuda12]"package. This could result in conflicting specifications or unexpected behavior. Verify that:
- The
pin_jaxgroup is needed alongside the explicitjax[cuda12]specification.- The syntax correctly applies both the group and the explicit extras.
If the intent is solely to get JAX with CUDA 12 extras, consider removing
--group pin_jaxand relying on the explicitjax[cuda12]specification, which already pins the version through the pin_jax group if desired..github/workflows/test_python.yml (1)
34-34: Verify command syntax and clarify JAX dependency handling.Line 34 combines multiple installation targets: editable package install (
-e .), features (.[test,jax]), direct packages (mpi4py), and a grouped dependency (--group pin_jax). This is complex, and the placement of--group pin_jaxafter the package specifier is unconventional. Verify:
- That
uv pip install --system -e .[test,jax] mpi4py --group pin_jaxexecutes as expected (groups typically precede or sit alongside package specs).- Whether both
.[test,jax]and--group pin_jaxare needed, or if one is redundant.If JAX is already provided via the
[jax]feature, the--group pin_jaxmay not be necessary.
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Streamlined CI/CD pipeline dependency management by consolidating installation commands across workflows. * Introduced modular dependency groups to enable flexible CPU and GPU environment configurations. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.