fix(train): initialize DeepSpeed mode from top-level config - #3471
Conversation
LauraGPT
left a comment
There was a problem hiding this comment.
Thanks for fixing the DeepSpeed mode propagation. There is one blocking compatibility issue before this can merge.
All current DeepSpeed training examples put both settings under train_conf, for example:
++train_conf.use_deepspeed=false
++train_conf.deepspeed_config=...
This patch also passes use_deepspeed= and deepspeed_config= explicitly and then expands **kwargs.get("train_conf"). Python therefore raises before entering Trainer:
TypeError: Trainer() got multiple values for keyword argument use_deepspeed
Please normalize the effective values once, with top-level values taking precedence and train_conf retained as a backwards-compatible fallback. Then pass a copied train_conf with the consumed distributed-mode keys removed, so use_ddp, use_fsdp, use_deepspeed, and deepspeed_config cannot be supplied twice.
Please also add focused regression coverage for:
- the existing nested
train_confform; - the new top-level form overriding nested defaults;
- rejecting simultaneous FSDP and DeepSpeed.
compileall passes because this is a call-time configuration failure, so a behavioral test is needed.
LauraGPT
left a comment
There was a problem hiding this comment.
Re-reviewed head 241dea6. The blocking duplicate-key path is resolved.
Verified:
- existing nested train_conf DeepSpeed settings remain supported
- top-level values override nested defaults
- consumed distributed keys are removed before Trainer receives **trainer_conf
- FSDP plus DeepSpeed is rejected
- DDP is disabled when either FSDP or DeepSpeed is selected
- focused tests: 3 passed
- additional precedence/invalid-combination matrix passed
- py_compile and diff checks passed
- synthetic merge with current main d410a56 is clean and limited to the intended two files
Approved.
Summary
Type of change
Validation
python -m compileall funasr examples testsUser impact
Notes for reviewers