Fix send_to_device crashing on defaultdict batches - #4162
Open
mayuriphad wants to merge 1 commit into
Open
Conversation
send_to_device reconstructed Mapping instances via type(tensor)(new_mapping), but defaultdict's constructor takes default_factory as its first positional argument rather than initial data, causing "TypeError: first argument must be callable or None" whenever a DataLoader collate_fn returned a defaultdict batch (fixes huggingface#4154). Preserve the default_factory and populate the new defaultdict via update() instead.
Author
|
Hi maintainers — just checking in on this PR since it hasn't had a maintainer review yet. Happy to make any changes needed; let me know if anything's unclear or if I should split/rework it. Thanks for your time! |
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
send_to_devicereconstructsMappingbatches viatype(tensor)(new_mapping). Forcollections.defaultdict, the constructor's first positional argument is thedefault_factory, not initial data, so this raisesTypeError: first argument must be callable or Nonewhenever aDataLoadercollate function returns adefaultdictbatch.Fixes #4154.
Change
In
src/accelerate/utils/operations.py::send_to_device, when the mapping being converted is adefaultdict, construct the new instance withtype(tensor)(tensor.default_factory)and populate it via.update(new_data)instead of passing the data dict as the constructor's first argument. OtherMappingsubclasses keep the previous behavior.Test plan
tests/test_utils.py::UtilsTester::test_send_to_device_defaultdict, a regression test that reproduces the issue (fails before the fix with the exactTypeErrorfrom the report, passes after) and checks that thedefault_factoryand existing values are preserved.python -m pytest tests/test_utils.py -k send_to_device -v-- all 3 tests pass.tests/test_utils.pysuite -- same pass/fail results as before the change (one pre-existing, unrelated failure:test_convert_to_fp32, atorch._inductor/torch.compileissue in this local Windows/CPU environment, not touched by this PR).AI disclosure: AI assistance was used to help investigate the issue and draft this fix and test. I reviewed the change and test results myself.