Fix multi-rank distributed-launch races and redundant LoRA merge in f… - #155
Open
sagarneeldubey wants to merge 1 commit into
Open
sagarneeldubey wants to merge 1 commit into
sagarneeldubey wants to merge 1 commit into
Conversation
…inetune.py
When launching with multiple ranks (torchrun/accelerate), two model-load
call sites race because each rank does the work independently:
1. snapshot_download(): all ranks call it concurrently, producing one HF
Hub API hit per rank and racing the post-download update_auto_map()
rewrite that runs only on the main process.
2. from_pretrained() with trust_remote_code=True: transformers copies the
model's custom modules (e.g. processing_prismatic.py) into a
process-local dynamic-modules cache. Concurrent ranks race on the same
destination file and a rank can import a partial write, raising:
AttributeError: module 'transformers_modules.<commit>.processing_prismatic'
has no attribute 'PrismaticProcessor'
Both are fixed by wrapping the call sites in
accelerate.PartialState.main_process_first() so rank 0 populates the
shared caches before the other ranks read them. Single-process runs are
unaffected. No behavior change beyond ordering.
Additionally, the LoRA-merge step in save_training_checkpoint() had every
rank call AutoModelForVision2Seq.from_pretrained() + PeftModel.from_pretrained()
+ merge_and_unload() on the 7B base model, even though only rank 0 ever
wrote the merged result to disk. That meant N redundant 7B loads + merges
per checkpoint on every multi-rank job. The adapter is already on disk
before this block (the preceding dist.barrier() guarantees it), so the
load + merge can be moved inside the is_main_process branch. The trailing
dist.barrier() is intentionally kept outside the guard so all ranks reach
it and stay in sync; placing it inside would deadlock.
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
vla-scripts/finetune.pyhas three multi-rank issues that I hit while fine-tuning OpenVLA withtorchrun/accelerate launchon more than one GPU on a single node. All three are fixed this PRsnapshot_download()race. All ranks call it concurrently, which produces one HF Hub API hit per rank and races the rank-0-onlyupdate_auto_map()rewrite that happens immediately after.from_pretrained(..., trust_remote_code=True)race.transformerscopies the model's custom modules (processing_prismatic.py,modeling_prismatic.py, etc.) into a process-localtransformers_modules/<commit>/cache. With N ranks importing concurrently, one rank can observe a partial file write and raise:AttributeError: module 'transformers_modules.<commit>.processing_prismatic' has no attribute 'PrismaticProcessor'This reproduces fairly reliably at ≥4 ranks on a fresh HF cache.
save_training_checkpoint(). Every rank callsAutoModelForVision2Seq.from_pretrained(...)+PeftModel.from_pretrained(...)+merge_and_unload()on the 7B base model, but only rank 0 writes the result. The other ranks buildmerged_vlaand immediately drop it on the floor. On 8 ranks that's 8× redundant 7B load + merge per checkpoint.Changes (single commit, single file)
snapshot_downloadrace: wrapped indistributed_state.main_process_first()so rank 0 populates the cache and runsupdate_auto_mapbefore other ranks read it.from_pretrainedrace: wrapped bothAutoProcessor.from_pretrainedandAutoModelForVision2Seq.from_pretrainedin the samemain_process_first()block so rank 0 finishes the dynamic-modules copy before peers import.from_pretrained+PeftModel.from_pretrained+merge_and_unload()inside the existingif distributed_state.is_main_process:branch insave_training_checkpoint. The precedingdist.barrier()already guarantees the adapter is on disk before this block runs.dist.barrier()after the merge is intentionally kept outside theis_main_processguard so all ranks still reach it and stay in sync. Putting it inside the guard would deadlock.Single-process / single-rank runs hit the same code paths but with
main_process_first()as a no-op, so behavior is unchanged.Test plan
Smoke-tested on a single AWS
p4d.24xlarge(8× A100 40GB) with 8 ranks viatorchrun --nproc_per_node=8, fine-tuningopenvla/openvla-7bonaloha_scoop_x_into_bowl. The short run exercises:snapshot_download+update_auto_mapon a cold HF cache,trust_remote_codedynamic-modules copy across all 8 ranks,Verified on the patched version:
PrismaticProcessorAttributeErrorpreviously seen onmain.Saved merged model for Step ....dist.barrier(); all ranks proceed to the next training step.Logs / screenshots
smoke test: 8 ranks past model load

smoke test: checkpoint save

Risk
Low. All changes are process-ordering / guard adjustments in
vla-scripts/finetune.py. No change to model architecture, optimizer, scheduler, dataset pipeline, training loop semantics, or saved-checkpoint format. Single-rank users see no behavior change.Notes for reviewers
finetune.py.main_process_first()pattern is already used inaccelerateexamples for exactly this kind of cache-population race, so I went with that rather than a hand-rolleddist.barrier()-based gate.