-
Notifications
You must be signed in to change notification settings - Fork 2.3k
[cfg] refactor: make actor config more modular #2379
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
- Add TestConfigComparison class with recursive config comparison method - Test ppo_trainer.yaml matches legacy_ppo_trainer.yaml exactly - Test ppo_megatron_trainer.yaml matches legacy_ppo_megatron_trainer.yaml exactly - Use Hydra compose to properly resolve config defaults and interpolations - Tests pass when configs match and fail with detailed error messages when they differ Co-Authored-By: H <linhaibin.eric@gmail.com>
- Move TestConfigComparison from test_algo_config_on_cpu.py to test_legacy_config_on_cpu.py - Create new test_legacy_config_on_cpu.py file with config comparison tests - Remove TestConfigComparison class from test_algo_config_on_cpu.py - Both test files continue to work correctly after the move - Maintains all existing functionality while improving test organization Co-Authored-By: H <linhaibin.eric@gmail.com>
- Add comprehensive field documentation following format check instructions - Document all fields in strategy, data_loader_seed, load_weight sections - Add detailed documentation for checkpoint configuration - Document all optimizer fields with explanations and options - Add comprehensive megatron section documentation covering parallelism options - Document profiling configuration fields - Follow CI format requirements: comments above fields, blank lines between fields - Use information from legacy_ppo_megatron_trainer.yaml as reference - Maintain proper indentation for nested fields Co-Authored-By: H <linhaibin.eric@gmail.com>
- Attempt to add comprehensive comments above each field - Working to resolve CI format validation failures - YAML file loads correctly but format validation still needs work - Continuing to debug comment placement requirements Co-Authored-By: H <linhaibin.eric@gmail.com>
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.
Code Review
This PR consolidates the actor config, enhancing maintainability. A test was added for backward compatibility. A logic error was identified in the new test file that needs to be addressed.
Shall we modify other roles as well in this PR? |
There's already 2k lines of code and I'd prefer we break them up and change one by one incrementally |
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.
After all refactors are done, can ppo_trainer.yaml
and ppo_megatron_trainer.yaml
be merged into one file?
yeah i think that's possible |
What does this PR do?
As part of the refactoring efforts, this PR consolidate the actor config in ppo-trainer and ppo-megatron-trainer. The shared configs are in actor/actor.yaml, and both megatron_actor.yaml and dp_actor.yaml inherits from it. If the change is reasonable we can expand it to critic, ref, rollout, data, etc.
Backward compatibility:
ppo_trainer.yaml
with OmegaConf now need to change to the following:Checklist Before Starting
[{modules}] {type}: {description}
(This will be checked by the CI){modules}
includefsdp
,megatron
,sglang
,vllm
,rollout
,trainer
,ci
,training_utils
,recipe
,hardware
,deployment
,ray
,worker
,single_controller
,misc
,perf
,model
,algo
,env
,tool
,ckpt
,doc
,data
,
like[megatron, fsdp, doc]
{type}
is infeat
,fix
,refactor
,chore
,test
[BREAKING]
to the beginning of the title.[BREAKING][fsdp, megatron] feat: dynamic batching
Test
For backward compatibility, a copy of the original legacy configs are stored in the test folder. A test is added to make sure the loaded new config is equivalent to legacy config. This test, however, should be disabled in the future since we will only maintain and update the new modular config going forward.
Checklist Before Submitting
Important
Please check all the following items before requesting a review, otherwise the reviewer might deprioritize this PR for review.
pre-commit install && pre-commit run --all-files --show-diff-on-failure --color=always
ci-request
channel in theverl
Slack workspace.