feat(dev): setup devcontainer for developers #4263
Conversation
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces a comprehensive setup for a development container tailored for the DeePMD-kit project. It includes a Dockerfile for C++ development on Ubuntu 24.04, a README detailing the environment setup, and several shell scripts for building C++ and Python components, downloading libraries, and configuring debugging. Additionally, it updates the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Outside diff range and nitpick comments (5)
.devcontainer/download_libtorch.sh (1)
1-8: Add script documentation.The script would benefit from a header comment explaining its purpose and usage.
Add this at the beginning of the file:
#!/bin/bash +# Download and extract LibTorch for C++ development +# +# This script downloads the CPU version of LibTorch with C++11 ABI and shared dependencies. +# Environment variables: +# LIBTORCH_VERSION: Override the default LibTorch version (default: 2.5.0) + set -ev🧰 Tools
🪛 Shellcheck
[warning] 4-4: Quote this to prevent word splitting.
(SC2046)
.devcontainer/lmp (1)
1-9: Add script documentation and usage information.The script would benefit from a header comment block explaining its purpose and requirements.
Add this documentation at the start of the script:
#!/bin/bash +# Script: lmp +# Purpose: Wrapper script to run LAMMPS with DeepMD-kit integration +# +# Requirements: +# - Python with TensorFlow installed +# - libtorch directory in ../libtorch +# - DeepMD-kit installed in ../dp +# - LAMMPS installed in ../.venv +# +# Usage: ./lmp [LAMMPS arguments].devcontainer/gdb_lmp (1)
1-1: Add script documentation and debug logging.Consider adding documentation and debug logging to make the script more maintainable and easier to troubleshoot.
#!/bin/bash + +# gdb_lmp: Debug LAMMPS with DeePMD-kit integration +# +# Usage: ./gdb_lmp [gdb_args...] +# +# This script sets up the necessary environment variables and paths +# to debug LAMMPS with DeePMD-kit integration using GDB. + +# Enable debug logging if DEBUG is set +if [ "${DEBUG:-}" = "1" ]; then + set -x +fi.devcontainer/devcontainer.json (1)
3-5: Consider adding version tags for build reproducibility.The Dockerfile reference is correct, but consider adding a version tag to ensure consistent builds across different environments and time periods.
"build": { - "dockerfile": "Dockerfile" + "dockerfile": "Dockerfile", + "tag": "1.0.0" }.devcontainer/build_cxx.sh (1)
1-20: Consider enhancing developer experience with additional features.As this script is part of the devcontainer setup, consider adding these developer-friendly features:
- Add a
--cleanflag to force clean builds- Add a
--verboseflag to control CMake output verbosity- Consider caching build artifacts in a volume for faster rebuilds
- Add support for different build types (Release, RelWithDebInfo)
Would you like me to provide an enhanced version of the script with these features?
🧰 Tools
🪛 Shellcheck
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- .devcontainer/Dockerfile (1 hunks)
- .devcontainer/READMD.md (1 hunks)
- .devcontainer/build_cxx.sh (1 hunks)
- .devcontainer/build_py.sh (1 hunks)
- .devcontainer/devcontainer.json (1 hunks)
- .devcontainer/download_libtorch.sh (1 hunks)
- .devcontainer/gdb_lmp (1 hunks)
- .devcontainer/lmp (1 hunks)
- .gitignore (1 hunks)
- pyproject.toml (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .devcontainer/Dockerfile
🧰 Additional context used
🪛 LanguageTool
.devcontainer/READMD.md
[grammar] ~4-~4: The word “setup” is a noun. The verb is spelled with a space.
Context: ...ironment to develop DeePMD-kit. One can setup locally or use [GitHub Codespaces](http...(NOUN_VERB_CONFUSION)
[typographical] ~30-~30: Consider adding a comma after ‘Usually’ for more clarity.
Context: ... -.devcontainer/gdb_lmp## Rebuild Usually the Python package does not need to rei...(RB_LY_COMMA)
🪛 Shellcheck
.devcontainer/build_cxx.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
.devcontainer/build_py.sh
[warning] 4-4: Quote this to prevent word splitting.
(SC2046)
.devcontainer/download_libtorch.sh
[warning] 4-4: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (8)
.devcontainer/build_py.sh (1)
1-8: Consider adding logging and status reporting for CI/CD integration.As this script is part of the devcontainer setup, it would be beneficial to:
- Add structured logging to help with CI/CD pipeline debugging
- Generate a status file or environment variables indicating successful completion
- Consider adding a cleanup function for failed installations
Let's verify if this script is referenced in CI workflows:
🧰 Tools
🪛 Shellcheck
[warning] 4-4: Quote this to prevent word splitting.
(SC2046)
.devcontainer/lmp (1)
1-9: Verify devcontainer integration and documentation.Please ensure this script is properly documented in
.devcontainer/README.mdand that the paths align with the devcontainer setup.✅ Verification successful
Documentation exists but filename has a typo
The script is properly documented in
.devcontainer/READMD.md(note the typo in filename), and the paths align with the devcontainer setup. Specifically:
- The
.venvdirectory is configured inremoteEnv.PATHlibtorchis downloaded and set up bydownload_libtorch.sh- The
dp/libpath is created during C++ build process inbuild_cxx.sh- The script usage is explicitly documented in the README: "When calling and debuging LAMMPS with DeePMD-kit, use the following scripts instead of the regular
lmp:.devcontainer/lmp"Two minor issues to address:
- Rename
READMD.mdtoREADME.mdto follow standard conventions- Consider adding error handling for the case when required directories don't exist
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify documentation and path references echo "Checking documentation references..." rg -l "lmp" .devcontainer/README.md || echo "Script not documented in README.md" echo "Checking devcontainer setup..." # Verify if the required paths are configured in devcontainer setup rg -l "libtorch|\.venv|dp/lib" .devcontainer/devcontainer.json || echo "Paths may not be configured in devcontainer"Length of output: 429
Script:
#!/bin/bash # Let's check the content of devcontainer.json to understand the configuration echo "Content of devcontainer.json:" cat .devcontainer/devcontainer.json # Check if there are any other documentation files in .devcontainer echo -e "\nListing files in .devcontainer:" ls -la .devcontainer/ # Check if the required directories exist echo -e "\nChecking referenced directories:" ls -la libtorch dp/lib .venv 2>/dev/null || echo "Some directories not found"Length of output: 1537
Script:
#!/bin/bash # Let's check the content of READMD.md (note the typo) to see if it contains documentation echo "Content of .devcontainer/READMD.md:" cat .devcontainer/READMD.md # Let's also check the build scripts to understand the directory structure echo -e "\nContent of build scripts:" echo "=== build_cxx.sh ===" cat .devcontainer/build_cxx.sh echo -e "\n=== build_py.sh ===" cat .devcontainer/build_py.sh echo -e "\n=== download_libtorch.sh ===" cat .devcontainer/download_libtorch.shLength of output: 2706
.gitignore (2)
48-50: LGTM! Standard ignore patterns added.The additions of
libtorch/,uv.lock,buildcxx/, and*.bib.originalare appropriate for ignoring build artifacts, downloaded dependencies, and temporary files.Also applies to: 52-52
51-51: Verify the need for node_modules/ ignore pattern.Could you clarify if there are Node.js dependencies in the project? This might be for documentation or development tools, but it would be helpful to understand the specific use case.
✅ Verification successful
Let me check the contents of the package.json file to better understand the Node.js dependencies.
The
node_modules/ignore pattern is necessary and correctly placedThe project contains Node.js components in the
source/nodejsdirectory with:
- A valid
package.jsonwithnode-gypdependency for native module compilation- JavaScript files including
index.jsand test files- Native module build support indicated by
gypfile: trueThe
node_modules/directory will be created when installing dependencies, so it should be ignored by git.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for Node.js related files to understand the context # Test: Look for package.json or other Node.js related files echo "Searching for Node.js related files..." fd -t f "package.json|package-lock.json|npm" # Test: Check if any documentation tools use Node.js echo -e "\nSearching for documentation configuration files..." fd -t f "docusaurus|vuepress|docsify|mkdocs"Length of output: 324
Script:
#!/bin/bash # Check contents of package.json cat source/nodejs/package.json # Look for any JavaScript/TypeScript files echo -e "\nSearching for JS/TS files..." fd -e js -e ts -e jsx -e tsxLength of output: 996
.devcontainer/devcontainer.json (1)
9-9: Verify the existence and permissions of referenced scripts.The configuration references several shell scripts. Let's verify they exist and have proper permissions.
✅ Verification successful
All referenced scripts exist and have proper executable permissions ✓
The verification confirms that all three scripts referenced in the
postCreateCommandexist in the.devcontainerdirectory and have the required executable permissions:
.devcontainer/build_py.sh.devcontainer/download_libtorch.sh.devcontainer/build_cxx.sh🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and permissions of required scripts # Expected: All scripts should exist and be executable echo "Checking required scripts..." for script in build_py.sh download_libtorch.sh build_cxx.sh; do script_path=".devcontainer/${script}" if [ -f "$script_path" ]; then echo "✓ Found: ${script}" if [ -x "$script_path" ]; then echo "✓ Executable: ${script}" else echo "✗ Not executable: ${script}" fi else echo "✗ Missing: ${script}" fi doneLength of output: 1012
pyproject.toml (3)
154-159: LGTM! Development dependencies are well-defined.The development dependency group includes essential tools for building and maintaining the project:
- pre-commit for code quality checks
- cmake for building C++ components
- mpich for MPI support
444-450: LGTM! UV source configuration is properly set up.The configuration correctly sets up a dedicated source for mpich packages using mpi4py's PyPI mirror. The
explicit = trueflag ensures deterministic package resolution.
139-146: Track UV workaround for future cleanup.The duplicate entries for jax and flax dependencies are added as a workaround for uv issue #8601. Consider adding a follow-up task to remove these duplicates once the issue is resolved.
✅ Verification successful
Workaround for UV issue #8601 is still necessary
The UV issue #8601 is still open and actively tracked. The duplicate entries for jax, flax, and orbax-checkpoint dependencies are currently required as a workaround. Keep these duplicates until the issue is resolved.
- The issue specifically affects dependencies using
python_version>=constraints- Current workaround ensures proper dependency resolution
- Affects lines 139-146 in pyproject.toml
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the UV issue is still open gh issue view 8601 --repo astral-sh/uv || echo "Issue not found or closed"Length of output: 1819
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .devcontainer/build_cxx.sh (1 hunks)
🧰 Additional context used
🪛 Shellcheck
.devcontainer/build_cxx.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## devel #4263 +/- ##
==========================================
+ Coverage 84.34% 84.35% +0.01%
==========================================
Files 549 549
Lines 51463 51463
Branches 3051 3051
==========================================
+ Hits 43404 43410 +6
+ Misses 7097 7095 -2
+ Partials 962 958 -4 ☔ View full report in Codecov by Sentry. |
See
.devcontainer/READMD.mdfor details.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
.gitignoreto prevent unnecessary files from being tracked.Chores
pyproject.tomlfor improved organization and clarity.