qwen3.5 lora compatibility (needs SGLang + Megatron-Bridge patches as well) - #1112
qwen3.5 lora compatibility (needs SGLang + Megatron-Bridge patches as well) #1112artem-osmosis wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds a LoRA training script for Qwen3.5-35B-A3B and introduces logic to filter target modules for Qwen3.5 models. Key feedback includes fixing a typo in the PYTHONUNBUFFERED environment variable, adjusting the module path pattern to ensure compatibility with text-only models, and recommending a more reliable way to identify model types than string matching on file paths.
| def exclude_mtp_vision_modules(target_modules: list[str]) -> list[str]: | ||
| """Restrict Qwen3.5-VL LoRA targets to the language model.""" | ||
| return [ | ||
| target if "language_model" in target else f"*language_model.decoder.layers.*.*.{target}" |
There was a problem hiding this comment.
The prefix *language_model.decoder.layers.*.*. might be too restrictive for text-only Qwen3.5 models. In the Megatron backend (as seen in megatron_to_hf/qwen3_5.py), the module hierarchy typically starts with decoder.layers without a language_model wrapper. Including language_model in the pattern may cause LoRA adapters to fail to match any modules in non-VL models. Consider using a more general pattern like *decoder.layers.*.*. to target the main transformer layers while still excluding MTP modules (which are usually under mtp.layers).
| target if "language_model" in target else f"*language_model.decoder.layers.*.*.{target}" | |
| target if "language_model" in target else f"*decoder.layers.*.*.{target}" |
| set -ex | ||
|
|
||
| # will prevent ray from buffering stdout/stderr | ||
| export PYTHONBUFFERED=16 |
There was a problem hiding this comment.
The environment variable should be PYTHONUNBUFFERED instead of PYTHONBUFFERED. Standard Python uses PYTHONUNBUFFERED to disable stream buffering, which is what the comment above indicates is the goal. Ensure the variable name matches the exact spelling expected by the system to avoid the override being ignored.
| export PYTHONBUFFERED=16 | |
| export PYTHONUNBUFFERED=1 |
References
- Ensure environment variable names match the exact spelling expected by the system, even if it contains a typo, to avoid the override being ignored.
| target_modules = convert_target_modules_to_megatron(args.target_modules, lora_type=lora_cls) | ||
|
|
||
| model_name = args.hf_checkpoint | ||
| if "Qwen3.5" in model_name: |
There was a problem hiding this comment.
Checking for "Qwen3.5" in the full checkpoint path is fragile, as the path might contain this string for unrelated reasons (e.g., a parent directory name) or might not contain it if the model directory was renamed. It would be more robust to retrieve model parameters and architecture details from the model configuration rather than hardcoding checks against file paths.
References
- Model parameters should be retrieved from the model configuration rather than being hardcoded.
for lora + qwen3.5 + megatron bridge to work, we also need patches to both the sglang-miles fork of sglang, and the bridge branch of radixark/Megatron-Bridge
changes for sglang:
AssertionError: LoRA buffer shape torch.Size([1152, 32]) does not match weight shape torch.Size([1280, 32]).changes for Megatron-Bridge:
transformer_layerandmtp_model_layerkeys for mtp megatron modulesMegatron-Bridge and sglang PRs: