should I manually change the max_position_embeddings for training long context?

[mungg]

Hi, I'm working on training long-context models using GRPO or SFT.
I set the model_len to my desired context length, but I have a question regarding max_position_embeddings in the model's config.json.

In 360-LLaMA-Factory, it seems like even without manually changing max_position_embeddings, the value is automatically updated in the newly trained model as long as I configure the max length and RoPE settings.
However, in ms-swift, unless I explicitly modify max_position_embeddings myself, even the trained models (with RoPE scaling applied) retain the original value.

so do I need to manually change max_position_embeddings to enable proper training for long-context tasks?
Is this the correct approach?

[tastelikefeet]

Fixed by #5155
Pull the latest code please

[mungg]

@tastelikefeet coool! thank you for the fast update 🥇

[mungg]

Hi, I've checked updated code and find issues.
I think it should parse from rope_scaling from config.json of model (if I understand your intention correctly)

ms-swift/swift/llm/argument/base_args/model_args.py

Lines 112 to 115 in 710553d

	if isinstance(self.rope_scaling, str):
	self.rope_scaling = json.loads(self.rope_scaling)
	if 'factor' in self.rope_scaling and max_model_len:
	self.max_model_len = int(self.rope_scaling['factor'] * max_model_len)

I modified your code little bit and pull-requested! #5220

[tastelikefeet]

Hi, I've checked updated code and find issues. I think it should parse from rope_scaling from config.json of model (if I understand your intention correctly)

ms-swift/swift/llm/argument/base_args/model_args.py

Lines 112 to 115 in 710553d

if isinstance(self.rope_scaling, str):
self.rope_scaling = json.loads(self.rope_scaling)
if 'factor' in self.rope_scaling and max_model_len:
self.max_model_len = int(self.rope_scaling['factor'] * max_model_len)
I modified your code little bit and pull-requested! #5220

Thanks for your commitment!
But I am a little confused, because:

self.rope_scaling = json.loads(self.rope_scaling) 

This code here I want to parse rope_scaling from the arguments, like:

--rope_scaling yarn
--rope_scaling '{"type":"yarn","factor":2.0}'

model_info.rope_scaling comes from the config.json, this is the old information, I was not intended to change this.
May I ask your scenario please?

[tastelikefeet]

This is an example we suggest to use:
https://github.com/modelscope/ms-swift/blob/main/examples/train/sequence_parallel/ulysses/sequence_parallel.sh

[mungg]

Oh, I just checked the updated documentation for rope_scaling, which now supports JSON format.

Previously, my commit was made to prioritize values from config.json, but now I see that your implementation prioritizes user-defined settings (e.g., command-line input). That makes sense!

However, I still think we should support the case where users don't specify rope_scaling explicitly, but it's already defined in the model's config.json. I think this case isn't handled current version. since qwen's official instructions recommend adding rope_scaling to config.json, I assume most users will follow that and expect it to work without extra config (if they are new to ms-swift like me lol)

Also, rope_scaling can includes the original_max_position_embeddings field, we should make sure that's handled correctly too!

Suggested priority:
User-specified rope_scaling via command-line (highest priority)
Fallback to model's config.json if not specified

For handling original_max_position_embeddings, here's the expected logic:

if original_max_position_embeddings is provided:
    if factor is provided:
        max_model_len = original_max_position_embeddings * factor  # if specified
        assert self.max_model_len == max_model_len
    else:
        factor = self.max_model_len / original_max_position_embeddings

How do you think about this?

[tastelikefeet]

max_model_len

I see your point! This makes sense!
May you please update your PR(#5220) to contain all the cases we talked about?
If you have a time problem, I can start a new PR(derived from your code) to fix this, thanks again!

[mungg]

okay, I will update my PR! ping you when it's ready :D

[mungg]

I updated my commit, but I found another issue when using vLLM :0
It seems that vLLM updates model length based only on the rope setting in config.json, not the one provided via the command line. Even if I set vllm_max_model_len, it throws an error unless rope is defined in the config. So when rope scaling is passed through the command line, max_model_len may not align with vllm_max_model_length, which might not be the intended behavior.
Let me know if I understood everything correctly! And if you'd prefer to update the PR yourself or open a new one, feel free to do whatever works best for you ;D

---

This is script what I used:

CUDA_VISIBLE_DEVICES=0,1,2,3,4,5,6,7
NPROC_PER_NODE=8
swift rlhf
--rlhf_type grpo
--model Qwen/Qwen3-8B
--reward_funcs format
--train_type full
--loss_type bnpo
--torch_dtype bfloat16
--dataset DATASET
--rope_scaling '{"type":"yarn","factor":3.0}'
--max_model_len 122880
--max_length 120000
--max_completion_length 2048
--num_train_epochs 1
--seed 42
--per_device_train_batch_size 1
--per_device_eval_batch_size 8
--gradient_accumulation_steps 1
--learning_rate 1e-6
--lr_scheduler_type="constant_with_warmup"
--temperature 1.0
--top_p 1.0
--dynamic_sample true
--warmup_ratio 0.05
--max_grad_norm 0.2
--beta 0.0
--save_strategy="steps"
--save_steps 50
--save_total_limit 20
--logging_steps 1
--output_dir output/test
--dataloader_num_workers 4
--num_generations 8
--deepspeed zero3_offload
--log_completions true
--report_to wandb
--num_iterations 1
--use_hf 1
--split_dataset_ratio 0
--weight_decay 0.0
--adam_beta2 0.999
--sequence_parallel_size 8
--use_vllm true
--vllm_mode colocate
--vllm_gpu_memory_utilization 0.5
--vllm_max_model_len 122880
--vllm_tensor_parallel_size 4
--add_version false

[tastelikefeet]

I updated my commit, but I found another issue when using vLLM :0 It seems that vLLM updates model length based only on the rope setting in config.json, not the one provided via the command line. Even if I set vllm_max_model_len, it throws an error unless rope is defined in the config. So when rope scaling is passed through the command line, max_model_len may not align with vllm_max_model_length, which might not be the intended behavior. Let me know if I understood everything correctly! And if you'd prefer to update the PR yourself or open a new one, feel free to do whatever works best for you ;D

This is script what I used:

CUDA_VISIBLE_DEVICES=0,1,2,3,4,5,6,7 NPROC_PER_NODE=8 swift rlhf --rlhf_type grpo --model Qwen/Qwen3-8B --reward_funcs format --train_type full --loss_type bnpo --torch_dtype bfloat16 --dataset DATASET --rope_scaling '{"type":"yarn","factor":3.0}' --max_model_len 122880 --max_length 120000 --max_completion_length 2048 --num_train_epochs 1 --seed 42 --per_device_train_batch_size 1 --per_device_eval_batch_size 8 --gradient_accumulation_steps 1 --learning_rate 1e-6 --lr_scheduler_type="constant_with_warmup" --temperature 1.0 --top_p 1.0 --dynamic_sample true --warmup_ratio 0.05 --max_grad_norm 0.2 --beta 0.0 --save_strategy="steps" --save_steps 50 --save_total_limit 20 --logging_steps 1 --output_dir output/test --dataloader_num_workers 4 --num_generations 8 --deepspeed zero3_offload --log_completions true --report_to wandb --num_iterations 1 --use_hf 1 --split_dataset_ratio 0 --weight_decay 0.0 --adam_beta2 0.999 --sequence_parallel_size 8 --use_vllm true --vllm_mode colocate --vllm_gpu_memory_utilization 0.5 --vllm_max_model_len 122880 --vllm_tensor_parallel_size 4 --add_version false

I commented the PR, there are some code need to discuss

Let me check the vLLM infer and GRPO training

[mungg]

thank you! I updated my commit.
Also I saw there are performance degradation on grpo without changing max_position_embeddings in config.json based on my experience. I appreciate your hard work 👍

[tastelikefeet]

thank you! I updated my commit. Also I saw there are performance degradation on grpo without changing max_position_embeddings in config.json based on my experience. I appreciate your hard work 👍

I updated the comment of PR, there is still one case not clear.
On the other hand, the vLLM needs a rope_scaling config of config.json, this case is still under talking, because it's hard to implement:
If we change the original config.json, it will affect other cases
If we don't change it, we need to monkey patch the vLLM, or just hard link the folder to another place on the disk
If you have any other suggestions, we are happy to accept it.