Repository navigation
[BugFix] Keep writes to an unpickled consolidated tensordict in its next pickle - #2516
Open
peterdsharpe wants to merge 2 commits into
Open
peterdsharpe wants to merge 2 commits into
peterdsharpe wants to merge 2 commits into
Conversation
…ext pickle An unpickled consolidated tensordict is not locked but keeps the consolidated storage and metadata it was rebuilt from. Writes that replaced, added or deleted an entry did not reach that storage, and the next pickle rebuilt the tensordict from it, so the writes were lost. TensorDict now marks its consolidated state stale when such a write replaces, adds or deletes an entry (_set_str, the fast path of update, del_, and in-place writes of non-tensor values), as memmap_() and non-tensor writes do since pytorch#2441. The next pickle then consolidates again. The rebuilt sub-tensordicts link their state to their parent's, so a nested write marks the root stale too. The copy stays unlocked, and a copy that is not written to stays consolidated. Closes pytorch#2503.
…ckle of an unpickled consolidated tensordict select(..., inplace=True), exclude(..., inplace=True) and popitem change the dict of entries directly, without going through _set_str or del_, so they did not mark the consolidated state of an unpickled copy stale. The next pickle rebuilt the copy from the old storage and brought the deleted keys back. They now mark it stale, as the other writes do. Part of the fix for pytorch#2503.
This branch has not been deployed
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.
Closes #2503.
A consolidated tensordict that goes through pickle,
torch.saveor a multiprocessing queue comes back unlocked but still marked as consolidated. Writes to that copy work, but the next pickle rebuilds the tensordict from the old consolidated storage, so the writes are silently lost. Any pipeline that receives a consolidated tensordict, edits it and sends it on hits this. With this PR the writes reach the next pickle, the copy stays unlocked, and a copy without writes still pickles from its consolidated storage.What was wrong
On the original
td, the same writes needunlock_()first, andunlock_()drops the consolidated state._rebuild_tensordict_files_consolidatedsets_consolidatedon the copy but does not lock it, becauseconsolidate()recordsis_lockedbefore it locks the result._reduce_tdthen trusts that state on the next pickle.The fix
TensorDictmarks its consolidated state stale when a write replaces, adds or deletes an entry, with_mark_consolidated_stale()from #2441:_set_strwhen it sets a new value (td[key] = value,set,set_non_tensor, nested keys);update, which writes into_tensordictdirectly;del_(alsopopandrename_key_, which go throughdel_and_set_str);popitem, andselect(..., inplace=True)andexclude(..., inplace=True), which change_tensordictdirectly (at each nested level they touch, too);set_("s", "bye")), which does not reach the storage either. [BugFix] Keep pickles of a consolidated tensordict in sync after memmap_() and non-tensor writes #2441 covered this for locked tensordicts only._reduce_tdalready consolidates again when the state is stale, andis_consolidated()returnsFalse, soconsolidate()andto(device)don't use the old storage either. In-place tensor writes (td["x"].add_(1),set_of a tensor) write into the storage, so they don't mark it.An unlocked copy has no lock parents, so a write to
r["n"]could not reach the root._rebuild_tensordict_files_consolidatedalready gives every rebuilt sub-tensordict its own_consolidated. It now adds a"parent"entry pointing to the state of the tensordict that holds it, and_mark_consolidated_stalefollows that chain. The readers of_consolidated(_reduce_td,to(),_ucxx.py,_base/distributed.py,_base/shape.py) only read"storage","metadata"and"stale", so the new key does not affect them.Changing the batch size or the names of an unpickled copy (
r.batch_size = ...,r.names = ...) is still lost on the next pickle here, as on main. #2471 (open) fixes that: its setters update the recorded metadata, and with itr.batch_size = [3, 4]andr.names = ["t"]on an unpickled copy survive the next pickle. The two PRs touch different code.The issue lists two other fixes. Locking the copy would make writes that work today raise. Not marking the copy as consolidated would keep the writes, but every unpickled copy would lose the one-buffer pickle and the one-copy
to(device). TorchRL'sParallelEnvwithconsolidate=Truesends consolidated tensordicts to its workers, which move them to the env device. This PR keeps both paths for copies without writes.The check is a dict lookup,
"_consolidated" in self.__dict__, on each write that sets a new value. Timed alone it costs about 17 ns, against about 900 ns fortd[key] = tensor. Timings oftd[key] = tensor,set_,updateand a nestedsetagainst main were within the noise of this shared machine (plus or minus 15%).Behaviour changes
After such a write,
is_consolidated()on the unpickled copy returnsFalse, and the next pickle consolidates the tensordict again. On main it returnedTrueand pickled the old content.The test
TestGeneric.test_consolidated_unpickled_write_pickleintest/tensordict/test_generic.pyunpickles a consolidated tensordict with a tensor, a string and a nested tensordict, writes to it in twelve ways (replace, add, replace a string,set_a string, nested write,del,update,selectandexcludewithinplace=Trueat the top level and on a nested key,popitem), and checks that the copy is no longer consolidated and that a second pickle keeps the write. It also checks that a copy without writes stays consolidated. All twelve cases fail on main and pass here.What I ran
All runs were local and CPU-only, with the environment variables CI uses.
test/tensordict/test_generic.py,test_nontensor.py,test_methods.py,test_lazy.pyandtest/tensorclass/test_tensorclass.py, torch 2.14.1 on Python 3.12: 11832 passed, 390 skipped, 2 failed. The two failures,TestNonTensorData::test_shared_copyandtest_where_non_tensor_data, fail the same way on main on this machine. (test_misc.pyalso passed on the first commit.)test/distributed/test_distributed.py,test_ucxx.py,test/memmap/test_zarr.py,test/utils/test_utils.pyandtest/tensorclass/test_compatibility.pywith-k "consolid or pickle": 16 passed.test/tensordict/test_generic.pywith torch 2.13: 898 passed, 12 skipped.import tensordictwith-W error::DeprecationWarning: no output.