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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add early support for
torchdata.stateful_dataloader.StatefulDataLoader
within theAccelerator
#2895Add early support for
torchdata.stateful_dataloader.StatefulDataLoader
within theAccelerator
#2895Changes from 67 commits
79a8fa2
efa1e7d
8dc107d
f342f4c
065849a
1e3fad1
a41cf38
8831488
140f1e6
727afeb
73683b4
32c318e
57c6f57
ed612d1
511050e
8dbc1a3
df43960
c778e32
4e00055
0471fe3
3036b7f
9ade2e9
ba0f5c6
b774291
fde597d
f273abc
8a46eb6
e4e1cac
8bf2fe2
ca4338d
c38f317
17a2a19
b39a606
d1e82e0
d99d734
39b2866
f58f609
f2119cf
7adec94
8850af3
6ff0f68
4f28d2e
a9b637d
0384543
0e0515d
809aca0
5145c2d
ca74ff2
a8f8bf3
59738f4
d264939
8f04c1e
45db4b9
0ffc64b
03a7774
8d2c6c3
6bfe871
7a344e4
6ff997e
4739524
4de9159
abf815a
06597d4
4142c7f
f02f18c
35977ca
597e910
419f607
4188d4c
51377a4
f4b6bb5
d02dfcc
21bc420
74e2f53
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me just bring up (again) that another solution could be monkey-patching
__instancecheck__
onDataLoader
. Not saying that it's less hacky, just wanted to raise awareness :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kinda looks dangerous. For example, this skips
@property
, is that intended? We could instead use__getattr__
to dispatch toself.base_dataloader
.If we want to stick this this, more succinct code could be:
self.__dict__.update(self.base_loader.__dict__)
orvars(self).update(self.base_loader.__dict__)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kinda agree with you, but all dynamic reflection looks dangerous to me.
I did write up an alternative which avoids the wizardry and just duplicates all the code required over here in: byi8220/accelerate@stateful-dataloader...byi8220:accelerate:stateful-dataloader-2
That code is messier and involves way more duplication, but much more explicit in what it does. If enough people feel the reflection approach is way too hacky and this feature doesn't justify it, I'm fine with doing that instead.
I updated the PR to do that instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, the name is not quite fitting, isn't it more like
update_state_dict
or so? Also, maybe we can avoid this all by not having a staticself.dl_state_dict
attribute but instead thestate_dict
method just returnsself.base_dataloader.state_dict()
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to
_update_state_dict
I'm not sure if we can. The base dataloader's state dict is one ahead of what we're yielding, so we couldn't do a passthrough. Some additional context in the comments of a6e192c#r1704736815
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a comment here when this needs to be called and with the context on why it's required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment here, kinda clunky though.