Let drop_last modify gather_for_metrics
#2048
Conversation
|
The documentation is not available anymore as the PR was closed or merged. |
src/accelerate/test_utils/scripts/external_deps/test_metrics.py
Outdated
Show resolved
Hide resolved
src/accelerate/test_utils/scripts/external_deps/test_metrics.py
Outdated
Show resolved
Hide resolved
| rng_types=None, | ||
| synchronized_generator=None, | ||
| skip_batches=0, | ||
| _drop_last: bool = False, |
There was a problem hiding this comment.
Why use an underscore for the argument name? That's quite unusual. drop_last on the dataloader doesn't use an underscore. If the attribute should be marked as "private", it can still use an underscore, but for the argument name, I'd go without (i.e. self._drop_last = drop_last).
There was a problem hiding this comment.
Because it is indeed private, and to be consistent with the other dataloader arg which has it as private too. Because we control it, the user shouldn't ever pass in _drop_last (but we need to on the backend)
There was a problem hiding this comment.
I see. It's a bit strange to the eye, but as you said, it should not be visible to the user.
Co-authored-by: Benjamin Bossan <[email protected]>
| rng_types=None, | ||
| synchronized_generator=None, | ||
| skip_batches=0, | ||
| _drop_last: bool = False, |
There was a problem hiding this comment.
I see. It's a bit strange to the eye, but as you said, it should not be visible to the user.
What does this PR do?
Ensures that if
drop_last=True, the default remainder for theGradientStateis utilized since we are just dropping the non-fitting batch anyways instead.Fixes # (issue)
closes #2007
Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.
@BenjaminBossan @LysandreJik