Call `set_epoch` on `Dataloader.batch_sampler` if defined #3124

Ghelfi · 2024-03-18T13:11:59Z

What does this PR do?

Proposes a technical implementation to set_epoch on either DataLoader.sampler or DataLoader.batch_sampler if it is defined.

What issue(s) does this change relate to?

Related to Set torch.utils.data.DataLoader.batch_sampler epoch if defined #3123

mvpatel2000 · 2024-03-19T18:56:01Z

Thanks for the PR!

Would you mind adding a unit test too please? You can extend test_checkpoint.py with class TestCheckpointResumption:. I would add an arg to get_trainer to add a batch dist sampler and add a new fn that tests that path instead.

If you need help, I'm happy to provide assistance for this!

When you are ready, please request review from mvpatel2000!

…ler is always defined

Bumps [coverage[toml]](https://github.com/nedbat/coveragepy) from 7.4.3 to 7.4.4. - [Release notes](https://github.com/nedbat/coveragepy/releases) - [Changelog](https://github.com/nedbat/coveragepy/blob/master/CHANGES.rst) - [Commits](nedbat/coveragepy@7.4.3...7.4.4) --- updated-dependencies: - dependency-name: coverage[toml] dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Co-authored-by: practicinginhell <oneforall1412@gmail.com>

…osaicml#3122) Updates the requirements on [packaging](https://github.com/pypa/packaging) to permit the latest version. - [Release notes](https://github.com/pypa/packaging/releases) - [Changelog](https://github.com/pypa/packaging/blob/main/CHANGELOG.rst) - [Commits](pypa/packaging@21.3...24.0) --- updated-dependencies: - dependency-name: packaging dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Mihir Patel <mihir.v.patel7@gmail.com>

composer/trainer/trainer.py

* fix documentation about which file extensions perform compression * refactor tar handling when writing checkpoint files * add CLI compressor options to checkpoint saving * update documentation * restructure CLI compressor handling. Added more tests * fix skipping test when compressor not installed * add documentation and fix some code style issues * use correct file extension for zstd * add missing full stop * fix failing tests * remove type ignore Co-authored-by: Mihir Patel <mihir.v.patel7@gmail.com> * combine imports into one * fix capitalisation Co-authored-by: Mihir Patel <mihir.v.patel7@gmail.com> --------- Co-authored-by: Mihir Patel <mihir.v.patel7@gmail.com>

* filter * remove metadata skip

* bump version to 021 * fix lint * fix docstring * remove old ones

Ghelfi · 2024-03-26T14:07:16Z

@mvpatel2000 cannot add you as reviewer to ask for a review.
I added a test and ready for a review.
I put the test where you suggested. Will it be better to put it in test_trainer.py?

…ler is always defined

…t batch_sampler

mvpatel2000

LGTM assuming tests pass! I have some minor style nits

tests/trainer/test_checkpoint.py

mvpatel2000 · 2024-04-01T17:24:36Z

@Ghelfi do you mind if I push directly to this branch? there's a few lint things and refactor in test I would prefer. I'm happy to leave as comments if you'd like, but I figure it's faster for me to directly apply. I don't want to delay you further :)

Ghelfi · 2024-04-01T18:06:45Z

@mvpatel2000 fine by me. Thanks for handling it.

* adding _get_dist_sampler function * apply _get_dist_sampler across all trainer.py * Switch condition to check if batch_sampler if filled first since sampler is always defined * adding docstring * removing antipattenr from _get_sampler and linting * removed default attrs from exception in the attrs dict (#3126) * Bump coverage[toml] from 7.4.3 to 7.4.4 (#3121) Bumps [coverage[toml]](https://github.com/nedbat/coveragepy) from 7.4.3 to 7.4.4. - [Release notes](https://github.com/nedbat/coveragepy/releases) - [Changelog](https://github.com/nedbat/coveragepy/blob/master/CHANGES.rst) - [Commits](nedbat/coveragepy@7.4.3...7.4.4) --- updated-dependencies: - dependency-name: coverage[toml] dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Refactor initialization (#3127) Co-authored-by: practicinginhell <oneforall1412@gmail.com> * Bump databricks sdk version (#3128) * Update packaging requirement from <23.3,>=21.3.0 to >=21.3.0,<24.1 (#3122) Updates the requirements on [packaging](https://github.com/pypa/packaging) to permit the latest version. - [Release notes](https://github.com/pypa/packaging/releases) - [Changelog](https://github.com/pypa/packaging/blob/main/CHANGELOG.rst) - [Commits](pypa/packaging@21.3...24.0) --- updated-dependencies: - dependency-name: packaging dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Mihir Patel <mihir.v.patel7@gmail.com> * remove rng from save weights only ckpt (#3129) * More compression options (#3118) * fix documentation about which file extensions perform compression * refactor tar handling when writing checkpoint files * add CLI compressor options to checkpoint saving * update documentation * restructure CLI compressor handling. Added more tests * fix skipping test when compressor not installed * add documentation and fix some code style issues * use correct file extension for zstd * add missing full stop * fix failing tests * remove type ignore Co-authored-by: Mihir Patel <mihir.v.patel7@gmail.com> * combine imports into one * fix capitalisation Co-authored-by: Mihir Patel <mihir.v.patel7@gmail.com> --------- Co-authored-by: Mihir Patel <mihir.v.patel7@gmail.com> * Only broadcast distcp files (#3130) * filter * remove metadata skip * Bump version to 0.21 (#3132) * bump version to 021 * fix lint * fix docstring * remove old ones * Adding a test for checking the update of epoch on batch_sampler * proper formating * adding _get_dist_sampler function * apply _get_dist_sampler across all trainer.py * Switch condition to check if batch_sampler if filled first since sampler is always defined * adding docstring * removing antipattenr from _get_sampler and linting * Adding a test for checking the update of epoch on batch_sampler * proper formating * Chang _get_sampler to _get_distributed_sampler to bypass torch default batch_sampler * Move batch_sampler instanciation for test * fix batch_sampler setting in eval_loop * trainer * fix * lint --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Jane Zhang <jane.zhang@databricks.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Practicinginhell <147235329+Practicinginhell@users.noreply.github.com> Co-authored-by: practicinginhell <oneforall1412@gmail.com> Co-authored-by: Daniel King <43149077+dakinggg@users.noreply.github.com> Co-authored-by: Mihir Patel <mihir.v.patel7@gmail.com> Co-authored-by: Evan Racah <evan@mosaicml.com> Co-authored-by: Matt Broadway <mattdbway@gmail.com> Co-authored-by: Alexandre Ghelfi <alexandre.ghelfi@helsing.ai>

Ghelfi mentioned this pull request Mar 18, 2024

Set torch.utils.data.DataLoader.batch_sampler epoch if defined #3123

Closed

Ghelfi and others added 11 commits March 21, 2024 17:43

adding _get_dist_sampler function

408179e

apply _get_dist_sampler across all trainer.py

514a8b1

Switch condition to check if batch_sampler if filled first since samp…

ef5540d

…ler is always defined

adding docstring

413c4c3

removing antipattenr from _get_sampler and linting

ed8535f

removed default attrs from exception in the attrs dict (mosaicml#3126)

ca7873f

Refactor initialization (mosaicml#3127)

9f768a4

Co-authored-by: practicinginhell <oneforall1412@gmail.com>

Bump databricks sdk version (mosaicml#3128)

06b8afd

remove rng from save weights only ckpt (mosaicml#3129)

0c42c51

Ghelfi force-pushed the set-epoch-on-batch-sampler branch from 660d7fd to 0c42c51 Compare March 21, 2024 16:43

Ghelfi requested review from a team as code owners March 21, 2024 16:43

antoinebrl reviewed Mar 22, 2024

View reviewed changes

composer/trainer/trainer.py Outdated Show resolved Hide resolved

mbway and others added 5 commits March 25, 2024 11:21

Only broadcast distcp files (mosaicml#3130)

2fdbf45

* filter * remove metadata skip

Bump version to 0.21 (mosaicml#3132)

88677c3

* bump version to 021 * fix lint * fix docstring * remove old ones

Adding a test for checking the update of epoch on batch_sampler

5161f59

proper formating

c731f67

Ghelfi force-pushed the set-epoch-on-batch-sampler branch from e496bd0 to c731f67 Compare March 25, 2024 10:21

Ghelfi requested a review from antoinebrl March 25, 2024 12:06

Ghelfi added 5 commits March 27, 2024 04:33

adding _get_dist_sampler function

570558d

apply _get_dist_sampler across all trainer.py

ab09d43

Switch condition to check if batch_sampler if filled first since samp…

df73325

…ler is always defined

adding docstring

61da1b8

removing antipattenr from _get_sampler and linting

12245cc

Ghelfi added 2 commits March 27, 2024 04:33

Adding a test for checking the update of epoch on batch_sampler

b2c147a

proper formating

68e6d9d

Ghelfi force-pushed the set-epoch-on-batch-sampler branch from c731f67 to 68e6d9d Compare March 27, 2024 03:36

fixing test

98c4e51

Ghelfi marked this pull request as draft March 27, 2024 14:33

Chang _get_sampler to _get_distributed_sampler to bypass torch defaul…

b3e6551

…t batch_sampler

Ghelfi marked this pull request as ready for review March 27, 2024 15:16

mvpatel2000 reviewed Mar 27, 2024

View reviewed changes

tests/trainer/test_checkpoint.py Outdated Show resolved Hide resolved

tests/trainer/test_checkpoint.py Outdated Show resolved Hide resolved

tests/trainer/test_checkpoint.py Show resolved Hide resolved

Ghelfi added 2 commits March 28, 2024 14:07

Move batch_sampler instanciation for test

76b888b

fix batch_sampler setting in eval_loop

dd45d3e

Ghelfi requested a review from mvpatel2000 March 29, 2024 08:25

Merge branch 'dev' into set-epoch-on-batch-sampler

14eb4f9

mvpatel2000 force-pushed the set-epoch-on-batch-sampler branch from e821428 to 2c6a3b4 Compare April 2, 2024 18:40

mvpatel2000 added 3 commits April 2, 2024 16:40

trainer

448d531

fix

522afe4

lint

fdb2efa

mvpatel2000 force-pushed the set-epoch-on-batch-sampler branch from c3d7539 to fdb2efa Compare April 2, 2024 21:41

mvpatel2000 approved these changes Apr 2, 2024

View reviewed changes

mvpatel2000 merged commit a9b9791 into mosaicml:dev Apr 2, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Call `set_epoch` on `Dataloader.batch_sampler` if defined #3124

Call `set_epoch` on `Dataloader.batch_sampler` if defined #3124

Ghelfi commented Mar 18, 2024

mvpatel2000 commented Mar 19, 2024 •

edited

Loading

Ghelfi commented Mar 26, 2024

mvpatel2000 left a comment

mvpatel2000 commented Apr 1, 2024

Ghelfi commented Apr 1, 2024

Call set_epoch on Dataloader.batch_sampler if defined #3124

Call set_epoch on Dataloader.batch_sampler if defined #3124

Conversation

Ghelfi commented Mar 18, 2024

What does this PR do?

What issue(s) does this change relate to?

mvpatel2000 commented Mar 19, 2024 • edited Loading

Ghelfi commented Mar 26, 2024

mvpatel2000 left a comment

Choose a reason for hiding this comment

mvpatel2000 commented Apr 1, 2024

Ghelfi commented Apr 1, 2024

Call `set_epoch` on `Dataloader.batch_sampler` if defined #3124

Call `set_epoch` on `Dataloader.batch_sampler` if defined #3124

mvpatel2000 commented Mar 19, 2024 •

edited

Loading