Repository navigation
CP2K support for dpgen2 - #238
Conversation
|
Warning Rate limit exceeded@Andy6M has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 31 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes introduce support for interfacing with CP2K simulations. This includes new classes and methods to handle CP2K inputs and executions, additional test cases, and updates to configuration files to integrate CP2K operations into larger workflows. The update also removes unnecessary arguments from existing methods and adds dependencies in Changes
Sequence Diagram(s)sequenceDiagram
participant Tester as TestFpOpCp2k
participant CP2KModule as dpgen2.fp.cp2k
participant Config as Configuration Files
participant Executor as Execution Engine
Tester->>+Config: Load CP2K configuration
Config-->>Tester: Configuration settings
Tester->>+CP2KModule: Prepare CP2K inputs
CP2KModule-->>Tester: Prepared inputs
Tester->>+Executor: Execute CP2K operation
Executor-->>Tester: Execution status and results
Tester->>Tester: Validate workflow execution and outputs
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
for more information, see https://pre-commit.ci
| def test_cp2k(self): | ||
| data_path = Path(__file__).parent / "data.cp2k" | ||
| print(data_path) | ||
| fp_config = { | ||
| "inputs": FpOpCp2kInputs(data_path / "input.inp"), | ||
| "run": { | ||
| "command": "cp -r %s output.log && cat %s" | ||
| % (data_path / "output.log", data_path / "output.log"), | ||
| }, | ||
| } |
There was a problem hiding this comment.
Use format specifiers instead of percent format.
Replace the percent format with format specifiers for better readability and consistency.
- "command": "cp -r %s output.log && cat %s"
- % (data_path / "output.log", data_path / "output.log"),
+ "command": "cp -r {} output.log && cat {}".format(data_path / "output.log", data_path / "output.log"),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.
| def test_cp2k(self): | |
| data_path = Path(__file__).parent / "data.cp2k" | |
| print(data_path) | |
| fp_config = { | |
| "inputs": FpOpCp2kInputs(data_path / "input.inp"), | |
| "run": { | |
| "command": "cp -r %s output.log && cat %s" | |
| % (data_path / "output.log", data_path / "output.log"), | |
| }, | |
| } | |
| def test_cp2k(self): | |
| data_path = Path(__file__).parent / "data.cp2k" | |
| print(data_path) | |
| fp_config = { | |
| "inputs": FpOpCp2kInputs(data_path / "input.inp"), | |
| "run": { | |
| "command": "cp -r {} output.log && cat {}".format(data_path / "output.log", data_path / "output.log"), | |
| }, | |
| } |
Tools
Ruff
42-43: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
| file_path = os.path.join(str(workdir), "output.log") | ||
|
|
||
| # convert the output to deepmd/npy format | ||
| with open(workdir / "input.inp", "r") as f: |
There was a problem hiding this comment.
Remove unnecessary open mode parameters.
The open mode parameter "r" is unnecessary for the open function.
- with open(workdir / "input.inp", "r") as f:
+ with open(workdir / "input.inp") as f: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.
| with open(workdir / "input.inp", "r") as f: | |
| with open(workdir / "input.inp") as f: |
Tools
Ruff
154-154: Unnecessary open mode parameters
Remove open mode parameters
(UP015)
| s["atom_types"][i] = atom_names.index(s["atom_names"][t]) # type: ignore https://github.com/microsoft/pyright/issues/5620 | ||
| s.data["atom_numbs"] = atom_numbs | ||
| s.data["atom_names"] = atom_names | ||
| target = "output/%s" % system |
There was a problem hiding this comment.
Use format specifiers instead of percent format.
Replace the percent format with format specifiers for better readability and consistency.
- target = "output/%s" % system
+ target = "output/{}".format(system)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.
| target = "output/%s" % system | |
| target = "output/{}".format(system) |
Tools
Ruff
88-88: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #238 +/- ##
==========================================
- Coverage 84.46% 83.84% -0.62%
==========================================
Files 96 97 +1
Lines 5285 5368 +83
==========================================
+ Hits 4464 4501 +37
- Misses 821 867 +46 ☔ View full report in Codecov by Sentry. |
|
|
||
| def test_cp2k(self): | ||
| data_path = Path(__file__).parent / "data.cp2k" | ||
| print(data_path) |
| data_path = Path(__file__).parent / "data.cp2k" | ||
| fp_config = { | ||
| "inputs": FpOpCp2kInputs(data_path / "input.inp"), | ||
| "run": { | ||
| "command": "cp -r %s output.log && cat %s" | ||
| % (data_path / "output.log", data_path / "output.log"), | ||
| }, |
There was a problem hiding this comment.
Use format specifiers instead of percent format.
Replace the percent format with format specifiers for better readability and consistency.
- "command": "cp -r %s output.log && cat %s"
- % (data_path / "output.log", data_path / "output.log"),
+ "command": "cp -r {} output.log && cat {}".format(data_path / "output.log", data_path / "output.log"),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.
| data_path = Path(__file__).parent / "data.cp2k" | |
| fp_config = { | |
| "inputs": FpOpCp2kInputs(data_path / "input.inp"), | |
| "run": { | |
| "command": "cp -r %s output.log && cat %s" | |
| % (data_path / "output.log", data_path / "output.log"), | |
| }, | |
| data_path = Path(__file__).parent / "data.cp2k" | |
| fp_config = { | |
| "inputs": FpOpCp2kInputs(data_path / "input.inp"), | |
| "run": { | |
| "command": "cp -r {} output.log && cat {}".format(data_path / "output.log", data_path / "output.log"), | |
| }, |
Tools
Ruff
48-49: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Could you please add an example, showcasing how to use cp2k in dpgen2?
| def execute( | ||
| self, | ||
| ip: OPIO, | ||
| ) -> OPIO: | ||
| confs = [] | ||
| # remove atom types with 0 atom from type map | ||
| # for all atom types in the type map | ||
| for p in ip["confs"]: | ||
| for f in p.rglob("type.raw"): | ||
| system = f.parent | ||
| s = dpdata.System(system, fmt="deepmd/npy") | ||
| atom_numbs = [] | ||
| atom_names = [] | ||
| for numb, name in zip(s["atom_numbs"], s["atom_names"]): # type: ignore https://github.com/microsoft/pyright/issues/5620 | ||
| if numb > 0: | ||
| atom_numbs.append(numb) | ||
| atom_names.append(name) | ||
| if atom_names != s["atom_names"]: | ||
| for i, t in enumerate(s["atom_types"]): # type: ignore https://github.com/microsoft/pyright/issues/5620 | ||
| s["atom_types"][i] = atom_names.index(s["atom_names"][t]) # type: ignore https://github.com/microsoft/pyright/issues/5620 | ||
| s.data["atom_numbs"] = atom_numbs | ||
| s.data["atom_names"] = atom_names | ||
| target = "output/%s" % system | ||
| s.to("deepmd/npy", target) | ||
| confs.append(Path(target)) | ||
| else: | ||
| confs.append(system) | ||
| op_in = OPIO( | ||
| { | ||
| "inputs": ip["config"]["inputs"], | ||
| "type_map": ip["type_map"], | ||
| "confs": confs, | ||
| "prep_image_config": ip["config"].get("prep", {}), | ||
| } | ||
| ) | ||
| op = PrepCp2k() | ||
| return op.execute(op_in) # type: ignore in the case of not importing fpop | ||
|
|
There was a problem hiding this comment.
Consider refactoring the execute method for readability and performance.
The nested loops and type manipulations can be refactored for better readability and performance. Additionally, consider handling potential exceptions that might occur during file operations or data manipulations.
def execute(self, ip: OPIO) -> OPIO:
confs = []
for p in ip["confs"]:
for f in p.rglob("type.raw"):
system = f.parent
s = dpdata.System(system, fmt="deepmd/npy")
atom_numbs = [numb for numb in s["atom_numbs"] if numb > 0]
atom_names = [name for numb, name in zip(s["atom_numbs"], s["atom_names"]) if numb > 0]
if atom_names != s["atom_names"]:
for i, t in enumerate(s["atom_types"]):
s["atom_types"][i] = atom_names.index(s["atom_names"][t])
s.data["atom_numbs"] = atom_numbs
s.data["atom_names"] = atom_names
target = f"output/{system}"
s.to("deepmd/npy", target)
confs.append(Path(target))
else:
confs.append(system)
op_in = OPIO({
"inputs": ip["config"]["inputs"],
"type_map": ip["type_map"],
"confs": confs,
"prep_image_config": ip["config"].get("prep", {}),
})
op = PrepCp2k()
return op.execute(op_in) # type: ignore in the case of not importing fpopTools
Ruff
88-88: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
| def execute( | ||
| self, | ||
| ip: OPIO, | ||
| ) -> OPIO: | ||
| run_config = ip["config"].get("run", {}) | ||
| op_in = OPIO( | ||
| { | ||
| "task_name": ip["task_name"], | ||
| "task_path": ip["task_path"], | ||
| "backward_list": [], | ||
| "log_name": "output.log", | ||
| "run_image_config": run_config, | ||
| } | ||
| ) | ||
| op = RunCp2k() | ||
| op_out = op.execute(op_in) # type: ignore in the case of not importing fpop | ||
| workdir = op_out["backward_dir"].parent | ||
|
|
||
| file_path = os.path.join(str(workdir), "output.log") | ||
|
|
||
| # convert the output to deepmd/npy format | ||
| with open(workdir / "input.inp", "r") as f: | ||
| lines = f.readlines() | ||
|
|
||
| # 获取 RUN_TYPE | ||
| run_type = get_run_type(lines) | ||
|
|
||
| if run_type == "ENERGY_FORCE": | ||
| sys = dpdata.LabeledSystem(file_path, fmt="cp2kdata/e_f") | ||
| elif run_type == "MD": | ||
| sys = dpdata.LabeledSystem( | ||
| str(workdir), cp2k_output_name="output.log", fmt="cp2kdata/md" | ||
| ) | ||
| else: | ||
| raise ValueError(f"Type of calculation {run_type} not supported") | ||
|
|
||
| # out_name = run_config.get("out", fp_default_out_data_name) | ||
| out_name = fp_default_out_data_name | ||
| sys.to("deepmd/npy", workdir / out_name) | ||
|
|
||
| return OPIO( | ||
| { | ||
| "log": workdir / "output.log", | ||
| "labeled_data": workdir / out_name, | ||
| } | ||
| ) |
There was a problem hiding this comment.
Remove unnecessary open mode parameters and use format specifiers.
- The open mode parameter "r" is unnecessary for the
openfunction. - Replace the percent format with format specifiers for better readability and consistency.
- with open(workdir / "input.inp", "r") as f:
+ with open(workdir / "input.inp") as f:
- target = "output/%s" % system
+ target = "output/{}".format(system)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.
| def execute( | |
| self, | |
| ip: OPIO, | |
| ) -> OPIO: | |
| run_config = ip["config"].get("run", {}) | |
| op_in = OPIO( | |
| { | |
| "task_name": ip["task_name"], | |
| "task_path": ip["task_path"], | |
| "backward_list": [], | |
| "log_name": "output.log", | |
| "run_image_config": run_config, | |
| } | |
| ) | |
| op = RunCp2k() | |
| op_out = op.execute(op_in) # type: ignore in the case of not importing fpop | |
| workdir = op_out["backward_dir"].parent | |
| file_path = os.path.join(str(workdir), "output.log") | |
| # convert the output to deepmd/npy format | |
| with open(workdir / "input.inp", "r") as f: | |
| lines = f.readlines() | |
| # 获取 RUN_TYPE | |
| run_type = get_run_type(lines) | |
| if run_type == "ENERGY_FORCE": | |
| sys = dpdata.LabeledSystem(file_path, fmt="cp2kdata/e_f") | |
| elif run_type == "MD": | |
| sys = dpdata.LabeledSystem( | |
| str(workdir), cp2k_output_name="output.log", fmt="cp2kdata/md" | |
| ) | |
| else: | |
| raise ValueError(f"Type of calculation {run_type} not supported") | |
| # out_name = run_config.get("out", fp_default_out_data_name) | |
| out_name = fp_default_out_data_name | |
| sys.to("deepmd/npy", workdir / out_name) | |
| return OPIO( | |
| { | |
| "log": workdir / "output.log", | |
| "labeled_data": workdir / out_name, | |
| } | |
| ) | |
| with open(workdir / "input.inp") as f: | |
| lines = f.readlines() | |
| # 获取 RUN_TYPE | |
| run_type = get_run_type(lines) | |
| if run_type == "ENERGY_FORCE": | |
| sys = dpdata.LabeledSystem(file_path, fmt="cp2kdata/e_f") | |
| elif run_type == "MD": | |
| sys = dpdata.LabeledSystem( | |
| str(workdir), cp2k_output_name="output.log", fmt="cp2kdata/md" | |
| ) | |
| else: | |
| raise ValueError(f"Type of calculation {run_type} not supported") | |
| # out_name = run_config.get("out", fp_default_out_data_name) | |
| out_name = fp_default_out_data_name | |
| sys.to("deepmd/npy", workdir / out_name) | |
| return OPIO( | |
| { | |
| "log": workdir / "output.log", | |
| "labeled_data": workdir / out_name, | |
| } | |
| ) |
Tools
Ruff
154-154: Unnecessary open mode parameters
Remove open mode parameters
(UP015)
Abacus input json file created by @zjgemi
| "bohrium_config": { | ||
| "username": "", | ||
| "password": "", | ||
| "project_id": 1, | ||
| "_comment": "all" | ||
| }, |
There was a problem hiding this comment.
Tip
Codebase Verification
Security Issue: Hardcoded Credentials Found
The file examples/calypso/input.test.json contains hardcoded credentials for "username" and "password". This poses a significant security risk.
examples/calypso/input.test.json- Line with
"username": "wzy@calypso.cn" - Line with
"password": "xxx"
- Line with
Please replace these hardcoded credentials with placeholders to ensure secure handling of sensitive information.
Analysis chain
Ensure secure handling of sensitive information.
The "bohrium_config" section contains placeholders for sensitive information like "username" and "password". Ensure these are securely managed and not hardcoded in the configuration file.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that sensitive information is not hardcoded in any configuration files.
# Test: Search for hardcoded sensitive information. Expect: No matches for "username" and "password".
rg --type json 'username|password'
Length of output: 1031
| "step_configs": { | ||
| "run_train_config": { | ||
| "template_config": { | ||
| "image": "", | ||
| "_comment": "all" | ||
| }, | ||
| "executor": { | ||
| "type": "dispatcher", | ||
| "retry_on_submission_error": 10, | ||
| "image_pull_policy": "IfNotPresent", | ||
| "machine_dict": { | ||
| "batch_type": "Bohrium", | ||
| "context_type": "Bohrium", | ||
| "remote_profile": { | ||
| "input_data": { | ||
| "job_type": "container", | ||
| "platform": "ali", | ||
| "scass_type": "1 * NVIDIA V100_16g" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "_comment": "all" | ||
| }, |
There was a problem hiding this comment.
Tip
Codebase Verification
Verify the "run_train_config" section in examples/water/input_dpgen_abacus.json.
The "run_train_config" section is missing or incorrectly specified in the examples/water/input_dpgen_abacus.json file. Ensure that this section is correctly included and properly structured.
- File:
examples/water/input_dpgen_abacus.json
Analysis chain
Verify the completeness and correctness of "run_train_config".
The "run_train_config" section contains configurations for a training step. Ensure that all necessary fields are included and correctly specified.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the completeness and correctness of "run_train_config".
# Test: Search for the usage of "run_train_config". Expect: All necessary fields are included and correctly specified.
rg --type json 'run_train_config'
Length of output: 571
Script:
#!/bin/bash
# Description: Extract and display the "run_train_config" sections from the identified files.
# Define the files containing "run_train_config"
files=(
"examples/chno/input.json"
"examples/water/input_distill.json"
"examples/water/input_multitask.json"
"examples/water/input_dpgen.json"
"examples/ch4/input_dist.json"
"examples/water/input_dpgen_abacus.json"
"examples/almg/input-v005.json"
"examples/calypso/input.test.json"
"examples/almg/input.json"
)
# Extract and display the "run_train_config" sections from each file
for file in "${files[@]}"; do
echo "File: $file"
jq '.run_train_config' "$file"
echo -e "\n"
done
Length of output: 2387
| "default_step_config": { | ||
| "template_config": { | ||
| "image": "", | ||
| "_comment": "all" | ||
| }, | ||
| "_comment": "all" | ||
| }, |
There was a problem hiding this comment.
Tip
Codebase Verification
Issues found with the "image" field in "template_config".
The "image" field in the "template_config" section is empty in several files, indicating that the correct image is not specified. This needs to be addressed to ensure the configuration is complete and functional.
examples/water/input_dpgen_abacus.jsonexamples/water/input_multitask.jsonexamples/water/input_distill.jsonexamples/water/input_dpgen.jsonexamples/ch4/input_dist.json
Please ensure that the correct image is specified in these files.
Analysis chain
Verify the usage of the "image" field.
The "default_step_config" section contains a "template_config" with an "image" field. Ensure that the correct image is specified when the configuration is in use.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the "image" field in the "template_config" is correctly specified.
# Test: Search for the usage of "default_step_config". Expect: Correct image specified.
rg --type json 'default_step_config'
Length of output: 521
Script:
#!/bin/bash
# Description: Extract and review the "image" field in the "template_config" for each occurrence of "default_step_config".
# List of files containing "default_step_config"
files=(
"examples/water/input_dpgen_abacus.json"
"examples/chno/input.json"
"examples/water/input_multitask.json"
"examples/water/input_distill.json"
"examples/water/input_dpgen.json"
"examples/ch4/input_dist.json"
"examples/calypso/input.test.json"
"examples/almg/input.json"
)
# Extract and print the "image" field from each file
for file in "${files[@]}"; do
echo "File: $file"
jq '.default_step_config.template_config.image' "$file"
done
Length of output: 1964
| "run_explore_config": { | ||
| "template_config": { | ||
| "image": "", | ||
| "_comment": "all" | ||
| }, | ||
| "continue_on_success_ratio": 0.8, | ||
| "executor": { | ||
| "type": "dispatcher", | ||
| "retry_on_submission_error": 10, | ||
| "image_pull_policy": "IfNotPresent", | ||
| "machine_dict": { | ||
| "batch_type": "Bohrium", | ||
| "context_type": "Bohrium", | ||
| "remote_profile": { | ||
| "input_data": { | ||
| "job_type": "container", | ||
| "platform": "ali", | ||
| "scass_type": "1 * NVIDIA V100_16g" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "template_slice_config": { | ||
| "group_size": 5, | ||
| "pool_size": 1 | ||
| }, | ||
| "_comment": "all" | ||
| }, |
There was a problem hiding this comment.
Tip
Codebase Verification
Incomplete run_explore_config section
The run_explore_config section in examples/water/input_dpgen_abacus.json is incomplete or incorrectly specified. For reference, compare it with the run_explore_config section in examples/almg/input-v005.json, which includes fields such as template_config, executor, and _comment.
-
examples/water/input_dpgen_abacus.json:{ "template_config": { "image": "", "_comment": "all" }, "continue_on_success_ratio": 0.8, "executor": { "type": "dispatcher", "retry_on_submission_error": 10, "image_pull_policy": "IfNotPresent", "machine_dict": { "batch_type": "Bohrium", "context_type": "Bohrium", "remote_profile": { "input_data": { "job_type": "container", "platform": "ali", "scass_type": "1 * NVIDIA V100_16g" } } } }, "template_slice_config": { "group_size": 5, "pool_size": 1 }, "_comment": "all" } -
examples/almg/input-v005.json:{ "template_config": { "image": "deepmd-kit:wanghan", "_comment": "all" }, "executor": { "type": "lebesgue_v2", "extra": { "scass_type": "c8_m32_cpu", "machine_type": "c8_m32_cpu", "platform": "paratera", "program_id": "xxxx", "job_type": "container", "region": "default" } }, "_comment": "all" }
Please ensure that all necessary fields are included and correctly specified.
Analysis chain
Verify the completeness and correctness of "run_explore_config".
The "run_explore_config" section contains configurations for an exploration step. Ensure that all necessary fields are included and correctly specified.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the completeness and correctness of "run_explore_config".
# Test: Search for the usage of "run_explore_config". Expect: All necessary fields are included and correctly specified.
rg --type json 'run_explore_config'
Length of output: 591
Script:
#!/bin/bash
# Extract and display the "run_explore_config" sections from the found JSON files for comparison.
# Define the files containing "run_explore_config"
files=(
"examples/water/input_dpgen_abacus.json"
"examples/ch4/input_dist.json"
"examples/chno/input.json"
"examples/water/input_dpgen.json"
"examples/water/input_distill.json"
"examples/water/input_multitask.json"
"examples/calypso/input.test.json"
"examples/almg/input-v005.json"
"examples/almg/input.json"
)
# Loop through each file and extract the "run_explore_config" section
for file in "${files[@]}"; do
echo "File: $file"
jq '.run_explore_config' "$file"
echo -e "\n"
done
Length of output: 2381
Thank you for your suggestion. Two example files, |
There was a problem hiding this comment.
Two example files,
examples/water/input_dpgen_abacus.jsonandexamples/water/input_dpgen_cp2k.json, have been uploaded with the assistance of @zjgemi.
Please add new example files to tests/check_examples.py.
There was a problem hiding this comment.
Thank you for your suggestion. The new example files have been added to tests/check_examples.py.
Summary by CodeRabbit
New Features
Bug Fixes
executefunction in theAbacusclass by removing an unnecessary argument.Tests
Dependencies
cp2kdatato the list of project dependencies.