feat: merged mode for caly evo step #224
Conversation
|
Warning Rate Limit Exceeded@wangzyphysics has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 35 minutes and 43 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. WalkthroughThe recent changes introduce a new class Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant Submit
participant CalyEvoStepMerge
participant CalyEvoStep
User->>Submit: Trigger make_concurrent_learning_op
Submit->>Submit: Check expl_mode
alt expl_mode is "merge"
Submit->>CalyEvoStepMerge: Create instance
CalyEvoStepMerge-->>Submit: Instance created
else expl_mode is "default"
Submit->>CalyEvoStep: Create instance
CalyEvoStep-->>Submit: Instance created
end
Submit-->>User: Return instance based on expl_mode
sequenceDiagram
participant User
participant PrepRunCaly
participant WiseExecutor
participant CalyEvoStepMerge
participant CalyEvoStep
User->>PrepRunCaly: Call _prep_run_caly with expl_mode
PrepRunCaly->>WiseExecutor: Determine executor based on expl_mode
alt expl_mode is "merge"
WiseExecutor->>PrepRunCaly: Return None
PrepRunCaly->>CalyEvoStepMerge: Execute with None executor
CalyEvoStepMerge-->>PrepRunCaly: Execution result
else expl_mode is "default"
WiseExecutor->>PrepRunCaly: Return initialized executor
PrepRunCaly->>CalyEvoStep: Execute with initialized executor
CalyEvoStep-->>PrepRunCaly: Execution result
end
PrepRunCaly-->>User: Return execution result
Tip Early Access Features
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #224 +/- ##
==========================================
- Coverage 84.63% 84.23% -0.40%
==========================================
Files 94 95 +1
Lines 5140 5214 +74
==========================================
+ Hits 4350 4392 +42
- Misses 790 822 +32 ☔ View full report in Codecov by Sentry. |
dpgen2/superop/prep_run_calypso.py
Outdated
| "qhull_input": temp_value, | ||
| }, | ||
| key=step_keys["caly-evo-step-{{item}}"], | ||
| executor=caly_evo_step_executor, |
There was a problem hiding this comment.
For super OP, dispatcher executor is invalid.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (17)
dpgen2/superop/__init__.py (1)
Line range hint
2-14: Consider addressing the unused imports or adding them to__all__if they are meant to be public.- from .block import ( - ConcurrentLearningBlock, - ) - from .prep_run_calypso import ( - PrepRunCaly, - ) - from .prep_run_dp_train import ( - PrepRunDPTrain, - ) - from .prep_run_fp import ( - PrepRunFp, - ) - from .prep_run_lmp import ( - PrepRunLmp, - )tests/op/test_prep_caly_dp_optim.py (1)
Line range hint
1-1: Remove unused imports to clean up the code.- import os - import numpy as np - from dflow.python.OP import OP - from dflow.python.Artifact import Artifact - from dflow.python.OPIOSign import OPIOSign - from dflow.python.TransientError import TransientError - from mock import call, mock, patch - from .context import dpgen2 - from dpgen2.constants import model_name_pattern - from dpgen2.utils.BinaryFileInput import BinaryFileInputAlso applies to: 8-8, 10-10, 12-14, 17-19, 24-24, 28-28, 34-34
dpgen2/op/prep_caly_dp_optim.py (1)
Line range hint
1-4: Remove unused imports to clean up the code.- import json - import logging - import pickle - import shutil - from typing import Tuple - from dflow.python.BigParameter import BigParameter - from dflow.python.TransientError import TransientError - from dpgen2.constants import calypso_check_opt_file, calypso_opt_dir_name, calypso_run_opt_file, model_name_pattern - from dpgen2.exploration.task.ExplorationTaskGroup import ExplorationTaskGroup - from dpgen2.utils.BinaryFileInput import BinaryFileInput - from dpgen2.utils.run_command.run_command import run_commandAlso applies to: 10-10, 17-17, 20-20, 24-27, 30-30, 33-33, 37-37
tests/test_prep_run_caly.py (1)
Line range hint
1-3: Remove unused imports to clean up the code.- import json - import os - import pickle - from typing import List, Set - import jsonpickle - import numpy as np - from dflow import InputArtifact, InputParameter, Inputs, OutputArtifact, OutputParameter, Outputs, S3Artifact, Steps - from dflow.argo_range import argo_range - from dflow.download_artifact import download_artifact - from dflow.python import OP, OPIO, Artifact, OPIOSign, PythonOPTemplate - from dpgen2.constants import calypso_index_pattern - from dpgen2.op.run_caly_model_devi import RunCalyModelDeviAlso applies to: 11-12, 15-16, 18-24, 26-26, 28-29, 33-37, 42-42, 85-85
dpgen2/superop/caly_evo_step.py (1)
Line range hint
6-6: Remove unused imports to clean up the code.- from pathlib import Path - from typing import Set - from dflow import OutputParameter, Workflow, argo_len, argo_range, argo_sequence, download_artifact, upload_artifact - from dflow.python import OPIO, Artifact, OPIOSign - from dpgen2.constants import calypso_index_patternAlso applies to: 11-11, 20-20, 24-25, 26-28, 30-30, 34-36, 42-42
dpgen2/superop/prep_run_calypso.py (1)
Line range hint
6-6: Remove unused imports to clean up the code and improve readability.- from pathlib import Path - from dflow.OPTemplate import OPTemplate - from dflow.OutputParameter import OutputParameter - from dflow.Workflow import Workflow - from dflow.argo_len import argo_len - from dflow.argo_range import argo_range - from dflow.argo_sequence import argo_sequence - from dflow.download_artifact import download_artifact - from dflow.upload_artifact import upload_artifact - from dflow.python.OPIO import OPIO - from dflow.python.Artifact import Artifact - from dflow.python.OPIOSign import OPIOSign - from dpgen2.constants.calypso_index_pattern import calypso_index_patternAlso applies to: 20-20, 22-22, 26-26, 27-27, 28-28, 29-29, 30-30, 35-35, 36-36, 37-37, 43-43
tests/test_caly_evo_step.py (1)
Line range hint
1-1: Remove unused imports to clean up the code and improve readability.- import json - import pickle - from typing import List, Set - import jsonpickle - import numpy as np - from dflow import InputArtifact, InputParameter, Inputs, OutputArtifact, OutputParameter, Outputs, S3Artifact, Steps - from dflow.argo_range import argo_range - from dflow.python import OP, Artifact, OPIOSign, PythonOPTemplate - from context.dpgen2 import dpgen2; consider using importlib.util.find_spec to test for availability - from mocked_ops.MockedPrepCalyDPOptim import MockedPrepCalyDPOptim - from dpgen2.constants import lmp_conf_name, lmp_input_name, lmp_log_name, lmp_model_devi_name, lmp_task_pattern, lmp_traj_name, model_name_pattern, train_log_name, train_script_name, train_task_pattern - from dpgen2.exploration.task import ExplorationTask, ExplorationTaskGroup - from dpgen2.op import RunCalyDPOptim - from dpgen2.op.collect_run_caly import CollRunCaly - from dpgen2.op.prep_caly_input import PrepCalyInputAlso applies to: 3-3, 11-11, 12-12, 15-15, 16-16, 18-18, 19-19, 20-20, 21-21, 22-22, 23-23, 24-24, 26-26, 28-28, 33-33, 35-35, 36-36, 37-37, 49-49, 63-63, 69-69, 70-70, 71-71, 72-72, 73-73, 74-74, 75-75, 76-76, 77-77, 78-78, 81-81, 82-82, 86-86, 89-89, 92-92, 95-95
dpgen2/entrypoint/submit.py (10)
Line range hint
2-2: Remove unused importglob.- import glob
Line range hint
6-6: Remove unused importpickle.- import pickle
Line range hint
16-16: Remove unused importsTypeandUnionfromtyping.- from typing import ( - Dict, - List, - Optional, - Tuple, - Type, - Union, - ) + from typing import ( + Dict, + List, + Optional, + Tuple, + )Also applies to: 17-17
Line range hint
20-20: Remove unused importdpdata.- import dpdata
Line range hint
23-45: Remove unused imports fromdflow.- from dflow import ( - ArgoStep, - InputArtifact, - InputParameter, - Inputs, - OutputArtifact, - OutputParameter, - Outputs, - S3Artifact, - Step, - Steps, - Workflow, - argo_range, - download_artifact, - upload_artifact, - ) - from dflow.python import ( - OP, - OPIO, - Artifact, - FatalError, - OPIOSign, - PythonOPTemplate, - TransientError, - upload_packages, - ) + from dflow import ( + ArgoStep, + S3Artifact, + Step, + Workflow, + upload_artifact, + )
Line range hint
52-52: Remove unused importdefault_hostfromdpgen2.constants.- from dpgen2.constants import ( - default_host, - default_image, - ) + from dpgen2.constants import ( + default_image, + )
Line range hint
65-80: Remove unused imports fromdpgen2.exploration.task.- from dpgen2.exploration.task import ( - CustomizedLmpTemplateTaskGroup, - ExplorationStage, - ExplorationTask, - LmpTemplateTaskGroup, - NPTTaskGroup, - caly_normalize, - make_calypso_task_group_from_config, - make_lmp_task_group_from_config, - normalize_lmp_task_group_config, - ) + from dpgen2.exploration.task import ( + ExplorationStage, + caly_normalize, + make_calypso_task_group_from_config, + make_lmp_task_group_from_config, + normalize_lmp_task_group_config, + )
Line range hint
121-128: Remove unused imports fromdpgen2.utils.- from dpgen2.utils import ( - BinaryFileInput, - bohrium_config_from_dict, - dump_object_to_file, - get_subkey, - load_object_from_file, - matched_step_key, - print_keys_in_nice_format, - sort_slice_ops, - workflow_config_from_dict, - ) + from dpgen2.utils import ( + BinaryFileInput, + get_subkey, + matched_step_key, + print_keys_in_nice_format, + sort_slice_ops, + )
Line range hint
454-454: Remove the unused local variabledefault_config.- default_config = normalize_step_dict( - { - "template_config": { - "image": default_image, - } - } - )
Line range hint
552-552: Remove unnecessary f-string.- logging.warning( - "No scheduler found in the workflow, " "does not do any replacement." - ) + logging.warning( + "No scheduler found in the workflow, does not do any replacement." + )
dpgen2/superop/prep_run_calypso.py
Outdated
| @@ -147,7 +143,7 @@ | |||
| prep_run_caly_steps: Steps, | |||
| step_keys: Dict[str, Any], | |||
| prep_caly_input_op: Type[OP], | |||
| caly_evo_step_op: OPTemplate, | |||
| caly_evo_step_op, | |||
There was a problem hiding this comment.
The type for caly_evo_step_op should be specified here as well for consistency and clarity.
- caly_evo_step_op,
+ caly_evo_step_op: Type[OP],Committable suggestion was skipped due low confidence.
dpgen2/superop/prep_run_calypso.py
Outdated
|
|
||
| class PrepRunCaly(Steps): | ||
| def __init__( | ||
| self, | ||
| name: str, | ||
| prep_caly_input_op: Type[OP], | ||
| caly_evo_step_op: OPTemplate, | ||
| caly_evo_step_op, |
There was a problem hiding this comment.
Consider specifying the type for caly_evo_step_op to maintain consistency and type safety.
- caly_evo_step_op,
+ caly_evo_step_op: Type[OP],Committable suggestion was skipped due low confidence.
There was a problem hiding this comment.
@wanghan-iapcm, the suggestion to remove the type hint for caly_evo_step_op was made to ensure consistency and type safety. However, after reviewing the context, it appears that the type hint OPTemplate is indeed appropriate and should be retained to maintain clarity and type safety in the code.
Here's the corrected version with the type hint retained:
caly_evo_step_op: OPTemplate,Thank you for pointing this out. I will make sure to keep the type hint for better code quality.
New Learnings Added
User: wanghan-iapcm
PR: deepmodeling/dpgen2#224
File: dpgen2/superop/prep_run_calypso.py:56-56
Timestamp: 2024-05-28T14:41:41.272Z
Learning: The type hint `OPTemplate` for `caly_evo_step_op` in the `PrepRunCaly` class constructor should be retained for consistency and type safety.
Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure review instructions.
| @@ -0,0 +1,350 @@ | |||
| import json | |||
There was a problem hiding this comment.
Remove unused imports to clean up the code and improve readability.
- import json
- import pickle
- from typing import List, Set
- import jsonpickle
- import numpy as np
- from dflow import InputArtifact, InputParameter, Inputs, OutputArtifact, OutputParameter, Outputs, S3Artifact, Steps
- from dflow.argo_range import argo_range
- from dflow.python import OP, Artifact, OPIOSign, PythonOPTemplate
- from context.dpgen2 import dpgen2; consider using importlib.util.find_spec to test for availability
- from mocked_ops.MockedPrepCalyDPOptim import MockedPrepCalyDPOptim
- from dpgen2.constants import lmp_conf_name, lmp_input_name, lmp_log_name, lmp_model_devi_name, lmp_task_pattern, lmp_traj_name, model_name_pattern, train_log_name, train_script_name, train_task_pattern
- from dpgen2.exploration.task import ExplorationTask, ExplorationTaskGroup
- from dpgen2.op import RunCalyDPOptim
- from dpgen2.op.collect_run_caly import CollRunCaly
- from dpgen2.op.prep_caly_input import PrepCalyInput
- from dpgen2.superop.caly_evo_step import CalyEvoStepAlso applies to: 3-3, 11-11, 12-12, 15-15, 16-16, 18-18, 19-19, 20-20, 21-21, 22-22, 23-23, 24-24, 26-26, 28-28, 33-33, 35-35, 36-36, 49-49, 63-63, 69-69, 70-70, 71-71, 72-72, 73-73, 74-74, 75-75, 76-76, 77-77, 78-78, 81-81, 82-82, 86-86, 92-92, 95-95, 98-98
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.
| import json |
dpgen2/op/caly_evo_step_merge.py
Outdated
| k: upload_artifact(ip[k]) if ip[k] is not None else None | ||
| for k in steps.inputs.artifacts | ||
| }, | ||
| with_param=range(1), |
There was a problem hiding this comment.
The with_param argument expects a string or list, but a range object is provided, which is incompatible.
- with_param=range(1),
+ with_param=["1"], # Assuming you want to iterate once, adjust as necessaryCommittable 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.
| with_param=range(1), | |
| with_param=["1"], # Assuming you want to iterate once, adjust as necessary |
| import json | ||
| import logging | ||
| import pickle | ||
| import shutil |
There was a problem hiding this comment.
Remove unused imports to clean up the code.
- import json
- import logging
- import pickle
- import shutil
- from typing import Tuple
- from dflow.python.TransientError import TransientError
- from dpgen2.constants import calypso_check_opt_file, calypso_run_opt_file
- from dpgen2.exploration.task import ExplorationTaskGroup
- from dpgen2.utils import BinaryFileInput, set_directory
- from dpgen2.utils.run_command import run_commandAlso applies to: 10-10, 27-27, 34-35, 38-38, 44-45, 48-48
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.
| import json | |
| import logging | |
| import pickle | |
| import shutil |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (4)
tests/op/test_prep_caly_input.py (1)
Line range hint
1-1: Remove unused imports to clean up the code.- import os - import numpy as np - from dflow.python.OP import OP - from dflow.python.Artifact import Artifact - from dflow.python.OPIOSign import OPIOSign - from dflow.python.TransientError import TransientError - from mock.call import call - from mock.mock import mock - from mock.patch import patch - from dpgen2.utils.BinaryFileInput import BinaryFileInputAlso applies to: 8-8, 10-10, 12-12, 13-13, 14-14, 17-17, 18-18, 19-19, 31-31
tests/test_prep_run_caly.py (1)
Line range hint
1-1: Remove unused imports to clean up the code.- import json - import os - import pickle - from typing import List - from typing import Set - import jsonpickle - import numpy as np - from dflow.InputArtifact import InputArtifact - from dflow.InputParameter import InputParameter - from dflow.Inputs import Inputs - from dflow.OutputArtifact import OutputArtifact - from dflow.OutputParameter import OutputParameter - from dflow.Outputs import Outputs - from dflow.S3Artifact import S3Artifact - from dflow.Steps import Steps - from dflow.argo_range import argo_range - from dflow.download_artifact import download_artifact - from dflow.python.OP import OP - from dflow.python.OPIO import OPIO - from dflow.python.Artifact import Artifact - from dflow.python.OPIOSign import OPIOSign - from dflow.python.PythonOPTemplate import PythonOPTemplate - from dpgen2.op.run_caly_model_devi.RunCalyModelDevi import RunCalyModelDeviAlso applies to: 2-2, 3-3, 11-11, 12-12, 15-15, 16-16, 18-18, 19-19, 20-20, 21-21, 22-22, 23-23, 24-24, 26-26, 28-28, 29-29, 33-33, 34-34, 35-35, 36-36, 37-37, 85-85
dpgen2/superop/prep_run_calypso.py (1)
Line range hint
6-6: Remove unused imports to clean up the code.- from pathlib import Path - from dflow.OPTemplate import OPTemplate - from dflow.OutputParameter import OutputParameter - from dflow.Workflow import Workflow - from dflow.argo_len import argo_len - from dflow.argo_sequence import argo_sequence - from dflow.download_artifact import download_artifact - from dflow.upload_artifact import upload_artifact - from dflow.python.OPIO import OPIO - from dflow.python.Artifact import Artifact - from dflow.python.OPIOSign import OPIOSign - from dpgen2.constants.calypso_index_pattern import calypso_index_pattern - Local variable `caly_evo_step_executor` is assigned to but never usedAlso applies to: 20-20, 22-22, 26-26, 27-27, 29-29, 30-30, 31-31, 35-35, 36-36, 37-37, 43-43, 186-186
dpgen2/op/prep_caly_input.py (1)
Line range hint
1-1: Remove unused imports to clean up the code.- import json - import pickle - from typing import Tuple - from dpgen2.constants.model_name_pattern import model_name_pattern - from dpgen2.exploration.task.ExplorationTaskGroup import ExplorationTaskGroup - from dpgen2.utils.set_directory import set_directoryAlso applies to: 2-2, 8-8, 25-25, 29-29, 32-32
| @@ -0,0 +1,129 @@ | |||
| import json | |||
There was a problem hiding this comment.
Remove unused imports to clean up the code.
- import json
- import logging
- import pickle
- import shutil
- from typing import Tuple
- from dflow.python.TransientError import TransientError
- from dpgen2.constants.calypso_check_opt_file import calypso_check_opt_file
- from dpgen2.constants.calypso_run_opt_file import calypso_run_opt_file
- from dpgen2.exploration.task.ExplorationTaskGroup import ExplorationTaskGroup
- from dpgen2.utils.BinaryFileInput import BinaryFileInput
- from dpgen2.utils.set_directory import set_directory
- from dpgen2.utils.run_command.run_command import run_commandAlso applies to: 2-2, 3-3, 4-4, 10-10, 27-27, 34-34, 35-35, 38-38, 44-44, 45-45, 48-48
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.
| import json |
dpgen2/superop/prep_run_calypso.py
Outdated
|
|
||
| class PrepRunCaly(Steps): | ||
| def __init__( | ||
| self, | ||
| name: str, | ||
| prep_caly_input_op: Type[OP], | ||
| caly_evo_step_op: OPTemplate, | ||
| caly_evo_step_op, |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
dpgen2/op/collect_run_caly.py (2)
Line range hint
1-1: Remove unused imports to clean up the code.- import json - import os - import random - import re - from typing import Optional, Set, Tuple - from dargs import ArgumentEncoder, Variant, dargs - from dflow.python.FatalError - from dpgen2.utils.BinaryFileInputAlso applies to: 3-5, 12-14, 19-21, 28-28, 38-38
62-62: Ensure thetask_nameparameter is correctly documented.The comment for
task_nameinget_input_signmethod could be more descriptive. It currently just repeats the variable name with a type.
Signed-off-by: zjgemi <[email protected]>
change task_name to BigParameter
one can choose mode from
defaultandmergein run_explore_config/modeSummary by CodeRabbit
New Features
CalyEvoStepMergeclass to handle merging results from evolutionary steps.Improvements
input.test.jsonto include new image versions and refined atom configurations.Bug Fixes
prep_caly_dp_optim.pyto correct directory structure.Tests
CalyEvoStepMergeand other workflow operations.