Fix load_accelerator_state only restoring one RNG backend - #4217
Open
Rakshit-gen wants to merge 1 commit into
Open
Fix load_accelerator_state only restoring one RNG backend#4217Rakshit-gen wants to merge 1 commit into
Rakshit-gen wants to merge 1 commit into
Conversation
save_accelerator_state uses independent if statements, so it saves an RNG seed for every backend that reports as available. On the load side, HPU, Neuron and CUDA were chained together with elif, with CUDA as the bare else. Two problems come from that: - If more than one backend reports as available, only the first one in the chain gets its RNG state restored, so training resumes with a stale RNG state on the others. This is the bug from huggingface#3960. - If none of XPU/MLU/SDAA/MUSA/HPU/Neuron/CUDA are available, for example on a CPU only machine or a TPU/XLA setup, the else branch still runs unconditionally and tries to read a torch_cuda_manual_seed key that was never saved, raising a KeyError. That gets silently swallowed by the surrounding try/except, so on XLA the xm.set_rng_state call right after it never runs either, and the saved XLA RNG state is never restored. Switched HPU, Neuron and CUDA to independent if statements, matching save_accelerator_state, so each backend that was actually saved gets restored regardless of what else is available. huggingface#3960 already reported the first issue and huggingface#3988 proposed close to this same fix, but it went stale without a maintainer review and got closed along with the issue. Picking that back up here with the same approach plus a couple of regression tests, since the underlying bug is still present on main.
4 tasks
Author
|
@SunMarc could you take a look when you get a chance? |
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.
Fixes #3960
save_accelerator_stateuses independentifstatements for each RNG backend, so it saves a seed for every backend that reports as available. On the load side, HPU, Neuron and CUDA were chained together withelif, ending in a bareelsefor CUDA. That causes two separate problems.If more than one backend reports as available, only the first one in the
elifchain actually gets its RNG state restored onload_state, so training resumes with a stale RNG state on the others. That's the reproducibility issue reported in #3960.There's a second problem the
elif/elsestructure causes that wasn't in the original report: if none of XPU, MLU, SDAA, MUSA, HPU, Neuron or CUDA are available, for example a plain CPU machine or a TPU/XLA setup, theelsebranch still runs unconditionally and tries to read atorch_cuda_manual_seedkey thatsave_accelerator_statenever wrote (since it only writes that key whenis_cuda_available()is true). That raises aKeyError, which gets silently swallowed by the surroundingtry/except, and thexm.set_rng_statecall that comes right after in the same block never runs, so on XLA the saved XLA RNG state never actually gets restored on resume.Change
Switched HPU, Neuron and CUDA in
load_accelerator_statefrom anelifchain to independentifstatements, matching the structuresave_accelerator_statealready uses. Each backend that was actually saved gets restored now, regardless of what else is or isn't available. MLU/SDAA/MUSA stay as anelifchain sincesave_accelerator_statetreats them the same way.#3960 already reported the first half of this and #3988 proposed close to the same fix, but it went stale without a maintainer review and got closed along with the issue. The underlying bug is still on main, so picking that back up here with the same approach, plus a couple of regression tests that were missing before.
Tests
Added two tests to
tests/test_state_checkpointing.py:test_rng_state_loads_without_accelerator_specific_backend: patches every specific backend to unavailable and asserts loading a saved checkpoint logs "All random states loaded successfully" instead of silently failing.test_rng_state_loads_for_every_saved_backend: patches two backends (HPU and CUDA) as available at once and asserts both get their RNG state restored, not just the first one in the chain.Checked both against the code before this change, they fail there and pass after. Also ran the full
tests/test_state_checkpointing.pysuite (CPU only, 22 tests), all pass.