feat(archon): enable PP > 1 for RL training with XCCL weight sync#877
Conversation
Summary of ChangesHello @rchardx, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the distributed training capabilities by enabling and stabilizing pipeline parallelism (PP > 1) for RL training. It achieves this by fundamentally restructuring how parallel groups are defined and utilized, introducing fine-grained, pipeline-stage-aware weight synchronization, and implementing critical locking mechanisms to maintain data integrity. The changes ensure that complex distributed setups can operate correctly and efficiently, while also preventing known incompatible configurations. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request enables pipeline parallelism (PP > 1) for RL training with XCCL weight synchronization. The changes include updating process group meshes to incorporate the pipeline parallel dimension, creating per-PP-stage weight update groups, and adding a distributed lock to serialize NCCL group initialization. Additionally, a validation check for the unsupported combination of pipeline parallelism and weight tying has been added.
My review identified a critical issue in three separate locations where a distributed lock is acquired. The current implementation does not guarantee that the lock will be released if an exception occurs within the locked code block, which could lead to deadlocks. I've provided suggestions to use try...finally blocks to ensure the lock is always released, making the implementation more robust.
There was a problem hiding this comment.
Pull request overview
This pull request enables pipeline parallelism (PP > 1) to work correctly with RL training by fixing data broadcast and weight synchronization issues when using XCCL for weight updates.
Changes:
- Replaced
cp_tpmesh withpp_cp_tpmesh to include PP dimension in the context and model parallel group, enabling proper data broadcast across PP stages - Added per-PP-stage weight update groups with DistributedLock to serialize NCCL group initialization and prevent race conditions
- Added validation to prevent PP with weight_tying, which is an unsupported combination due to shared weights residing on different GPUs
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| areal/experimental/models/archon/parallel_dims.py | Updated mesh definition from cp_tp to pp_cp_tp to include PP dimension for proper context and model parallel grouping |
| areal/experimental/engine/archon_engine.py | Added PP rank tracking, per-stage weight update groups, DistributedLock for NCCL serialization, and PP+weight_tying validation |
| areal/engine/megatron_engine.py | Added DistributedLock usage for NCCL group initialization consistency with ArchonEngine |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Enable pipeline parallelism (PP > 1) to work correctly with RL training by fixing data broadcast and weight synchronization issues. Key changes: - Replace cp_tp mesh with pp_cp_tp mesh to include PP in context_and_model_parallel_group - Add per-PP-stage weight update groups for XCCL weight sync - Add DistributedLock to serialize NCCL group initialization - Add PP + weight_tying validation (unsupported combination) - Fix MegatronEngine missing engine_lock in init
28e2c9b to
125f2a1
Compare
…eal-project#877) Enable pipeline parallelism (PP > 1) to work correctly with RL training by fixing data broadcast and weight synchronization issues. Key changes: - Replace cp_tp mesh with pp_cp_tp mesh to include PP in context_and_model_parallel_group - Add per-PP-stage weight update groups for XCCL weight sync - Add DistributedLock to serialize NCCL group initialization - Add PP + weight_tying validation (unsupported combination) - Fix MegatronEngine missing engine_lock in init
…eal-project#877) Enable pipeline parallelism (PP > 1) to work correctly with RL training by fixing data broadcast and weight synchronization issues. Key changes: - Replace cp_tp mesh with pp_cp_tp mesh to include PP in context_and_model_parallel_group - Add per-PP-stage weight update groups for XCCL weight sync - Add DistributedLock to serialize NCCL group initialization - Add PP + weight_tying validation (unsupported combination) - Fix MegatronEngine missing engine_lock in init
Description
Enable pipeline parallelism (PP > 1) to work correctly with RL training by fixing data broadcast and weight synchronization issues.
Key changes:
Type of Change
work as expected)
Checklist
jb build docs/gemini review)