Repository navigation
Conversation
Signed-off-by: Runkai Tao <rt572@physics.rutgers.edu>
There was a problem hiding this comment.
Code Review
This pull request enables tunable control for naive_block_assignment in the LoRA Fused MoE layer, allowing the strategy to be specified directly in the operator configurations. While the logic for falling back to the heuristic is sound, there is a critical bug in how the configuration keys are accessed. Because the configuration dictionaries are normalized to uppercase keys before use, the current code fails to find the lowercase "naive_block_assignment" key, effectively rendering the tunable control feature non-functional. I have provided suggestions to fix this key mismatch in both the activation and summation decorators.
| shrink_naive = shrink_config.pop("naive_block_assignment", None) | ||
| expand_naive = expand_config.pop("naive_block_assignment", None) |
There was a problem hiding this comment.
The keys in shrink_config and expand_config are normalized to uppercase by _normalize_keys (called within _get_lora_moe_configs at line 188). Consequently, popping the lowercase "naive_block_assignment" will always return None, causing the code to always fall back to the heuristic and ignoring any tuned values. You should use the normalized key "NAIVE_BLOCK_ASSIGNMENT" instead.
| shrink_naive = shrink_config.pop("naive_block_assignment", None) | |
| expand_naive = expand_config.pop("naive_block_assignment", None) | |
| shrink_naive = shrink_config.pop("NAIVE_BLOCK_ASSIGNMENT", None) | |
| expand_naive = expand_config.pop("NAIVE_BLOCK_ASSIGNMENT", None) |
| shrink_config.pop("naive_block_assignment", None) | ||
| expand_config.pop("naive_block_assignment", None) |
There was a problem hiding this comment.
Similar to the issue in the activation decorator, the keys here are normalized to uppercase. Failing to pop the correct key means "NAIVE_BLOCK_ASSIGNMENT" remains in the configuration dictionaries, which may cause issues if passed down to underlying functions that do not expect it. Use the uppercase key name here as well.
| shrink_config.pop("naive_block_assignment", None) | |
| expand_config.pop("naive_block_assignment", None) | |
| shrink_config.pop("NAIVE_BLOCK_ASSIGNMENT", None) | |
| expand_config.pop("NAIVE_BLOCK_ASSIGNMENT", None) |
Signed-off-by: Runkai Tao <rt572@physics.rutgers.edu>
|
This pull request has been automatically marked as stale because it has not had any activity within 90 days. It will be automatically closed if no further activity occurs within 30 days. Leave a comment if you feel this pull request should remain open. Thank you! |
|
This pull request has been automatically closed due to inactivity. Please feel free to reopen if you intend to continue working on it. Thank you! |
Purpose
In #32655, we used heuristic condition to decide whether turning on unpermute or not. In this PR, we also support unpermute control from configurations directly. We can see some improvement when adding
naive_block_assignmentin the search space during tuning.Test Result
gpt-oss 120b
1600 input, 600 output, max_loras = 8, concurrency = 8
before
after
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.