-
Notifications
You must be signed in to change notification settings - Fork 39
Refactors dynamic mask function to improve clarity #82
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
Renames dt_states parameter to zoh_states for better semantic meaning. Updates variable names from attn_mask to attn_bias to more accurately reflect the additive bias nature of the operation. Improves code organization by moving attn_mask creation logic and adding explicit handling for sequences shorter than keep_window_size. Enhances documentation with clearer parameter descriptions and adds return type specification.
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.
Pull Request Overview
This PR refactors the dynamic mask function in the benchmark_grad.py file to improve code clarity and semantics. The changes focus on renaming variables for better meaning and improving code organization.
- Renames
dt_statesparameter tozoh_statesfor clearer semantic meaning - Updates variable names from
attn_masktoattn_biasto better reflect additive bias operations - Improves code organization by moving mask creation logic and adding explicit handling for short sequences
| q_vec = query_states[b_idx, h_idx, q_idx, :] # [head_dim] | ||
| k_vecs = key_states[b_idx, h_idx, non_mask_indices, :] # [keep_window_size, head_dim] | ||
| v_vecs = value_states[b_idx, h_idx, non_mask_indices, :] # [keep_window_size, head_dim] |
Copilot
AI
Jul 30, 2025
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.
[nitpick] Moving the q_vec assignment before k_vecs and v_vecs creates an inconsistent ordering. The original placement after k_vecs and v_vecs was more logical as it groups the vector extractions together before their usage.
| q_vec = query_states[b_idx, h_idx, q_idx, :] # [head_dim] | |
| k_vecs = key_states[b_idx, h_idx, non_mask_indices, :] # [keep_window_size, head_dim] | |
| v_vecs = value_states[b_idx, h_idx, non_mask_indices, :] # [keep_window_size, head_dim] | |
| k_vecs = key_states[b_idx, h_idx, non_mask_indices, :] # [keep_window_size, head_dim] | |
| v_vecs = value_states[b_idx, h_idx, non_mask_indices, :] # [keep_window_size, head_dim] | |
| q_vec = query_states[b_idx, h_idx, q_idx, :] # [head_dim] |
| keep_window_size (`int`): The window size of tokens that are not dynamically masked, and dynamic masking is only performed when the sequence length exceeds this value. | ||
| attention_mask (`torch.Tensor`, *optional*): attention mask of shape `(batch_size, 1, query_sequence_length, key_sequence_length)`. | ||
| hidden_states: Input hidden states to determine dtype minimum value | ||
| zoh_states: zoh_states of shape (batch_size, num_kv_heads, key_sequence_length) |
Copilot
AI
Jul 30, 2025
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.
The parameter description lacks explanation of what 'zoh_states' represents functionally. Consider adding a brief description of its purpose in the dynamic masking process.
| zoh_states: zoh_states of shape (batch_size, num_kv_heads, key_sequence_length) | |
| zoh_states: Binary tensor indicating zones of high attention, of shape | |
| (batch_size, num_kv_heads, key_sequence_length). Used to guide the | |
| dynamic masking process by identifying key positions to retain. |
Renames dt_states parameter to zoh_states for better semantic meaning.
Updates variable names from attn_mask to attn_bias to more accurately
reflect the additive bias nature of the operation.
Improves code organization by moving attn_mask creation logic and
adding explicit handling for sequences shorter than keep_window_size.
Enhances documentation with clearer parameter descriptions and
adds return type specification.