-
Notifications
You must be signed in to change notification settings - Fork 173
refactor: refactor env and data processor & add nemotron super 49b recipes #1506
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
base: main
Are you sure you want to change the base?
Conversation
75f3d5c to
5ebbc73
Compare
c9335d4 to
a872ed6
Compare
b7fedb9 to
9078e33
Compare
c0bfaa6 to
ab0ac80
Compare
… processors. Added raw_dataset.py and path.py for improved dataset processing. Updated project-includes in pyrefly.toml and modified grpo.md to reflect new task-dataset mapping. Cleaned up unused code and configurations in various YAML files. Signed-off-by: ruit <[email protected]>
…or handling
- Introduced documentation for the new Code Jaccard Environment, detailing its functionality, usage, and configuration.
- Updated RawDataset class to provide a default processor if none is specified in the data configuration.
- Enhanced test coverage for the helpsteer3 data processor to ensure correct functionality and output.
Signed-off-by: ruit <[email protected]>
Signed-off-by: ruit <[email protected]>
- Updated CLEVRCoGenTDataset, OpenAIFormatDataset, and SquadDataset to inherit from the RawDataset class for improved dataset handling. - Added necessary imports for RawDataset in the respective files. Signed-off-by: ruit <[email protected]>
…up for vlm grpo - Added `env_name` to `vlm_grpo_3B_megatron.yaml` and `vlm_grpo_3B.yaml` for environment specification. - Modified `setup_data` function in `run_vlm_grpo.py` to use `env_name` for environment configuration, enhancing flexibility in dataset processing. Signed-off-by: ruit <[email protected]>
6e4393e to
7e1566c
Compare
|
7e1566c to
3fb0ec3
Compare
|
…tion Signed-off-by: ruit <[email protected]>
3fb0ec3 to
a24ba76
Compare
|
Signed-off-by: ruit <[email protected]>
|
Signed-off-by: Yuki Huang <[email protected]>
|
| def __init__(self, data_config: dict, seed: int = 42): | ||
| self.data_config: dict = data_config | ||
| self.seed: int = seed | ||
| self.processor: TaskDataProcessFnCallable | None = None | ||
| self.task_spec: TaskDataSpec | None = None | ||
| raise NotImplementedError("__init__ is not implemented") |
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.
why not using something like this then do super().__init__(...) in the sub-class?
then self.data_config = data_config at line39, set_processor and set_task_spec in nemo_rl/data/datasets/response_datasets/__init__.py can be removed.
| def __init__(self, data_config: dict, seed: int = 42): | |
| self.data_config: dict = data_config | |
| self.seed: int = seed | |
| self.processor: TaskDataProcessFnCallable | None = None | |
| self.task_spec: TaskDataSpec | None = None | |
| raise NotImplementedError("__init__ is not implemented") | |
| def __init__(self, data_config: dict, seed: int = 42): | |
| self.data_config: dict = data_config | |
| self.seed: int = seed | |
| self.processor: TaskDataProcessFnCallable | None = self.get_processor() | |
| self.task_spec: TaskDataSpec | None = self.get_task_spec() |
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.
If use super().__init__(...), we need pass data_config when initialize dataset, then all the unit test and other place using dataset need to be modified. It will lead to a huger PR. This part will be done in future PR for ISSUE #1552
| from nemo_rl.utils.logger import get_next_experiment_dir | ||
|
|
||
| OmegaConf.register_new_resolver("mul", lambda a, b: a * b) | ||
| OmegaConf.register_new_resolver("max", lambda a, b: max(a, b)) |
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.
P1, we should move customized op into a common place in the future.
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.
ACK. I can move it to a new place in the following PR to resolve #1552 .
Follow up of #1472. Thanks @nv-mmanohara for adding this!
run_grpo.py, will [Refactor] Clearrun_grpo_math.pyandrun_grpo_rm.py#1572 in a subsequent PR.Test Result
grpo math before and after refactor
nemotron 49B
Known Issue
Summary by CodeRabbit
New Features
Documentation
Tests