Skip to content

fix apply_rotary_pos_emb_thd missing cp_group#146

Merged
hjh0119 merged 1 commit into
modelscope:mainfrom
hjh0119:fix-0710
Jul 11, 2026
Merged

fix apply_rotary_pos_emb_thd missing cp_group#146
hjh0119 merged 1 commit into
modelscope:mainfrom
hjh0119:fix-0710

Conversation

@hjh0119

@hjh0119 hjh0119 commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates _apply_rotary_pos_emb_thd in src/mcore_bridge/patcher.py to retrieve and pass the context parallel group (cp_group) when calling _origin_apply_rotary_pos_emb_thd. However, retrieving the context parallel group unconditionally when context parallel is not initialized (e.g., when cp_size == 1) will cause an AssertionError and crash the execution. It is recommended to retrieve the group conditionally only when cp_size > 1.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines 203 to +204
cp_size = mpu.get_context_parallel_world_size()
cp_group = mpu.get_context_parallel_group()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

Calling mpu.get_context_parallel_group() unconditionally when context parallel is not initialized (which is the case when cp_size == 1) will raise an AssertionError: context parallel group is not initialized in Megatron-LM. This will cause any run without context parallel to crash.\n\nWe should only retrieve the context parallel group if cp_size > 1.

            cp_size = mpu.get_context_parallel_world_size()\n            cp_group = mpu.get_context_parallel_group() if cp_size > 1 else None

@hjh0119
hjh0119 merged commit 2220f77 into modelscope:main Jul 11, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants