Skip to content
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

limit auto scaling batch size to the size of the training dataset #3271

Merged
merged 11 commits into from
Sep 9, 2020

Conversation

awaelchli
Copy link
Contributor

@awaelchli awaelchli commented Aug 30, 2020

What does this PR do?

Fixes #3259
and releated to #3266

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?
  • Did you verify new and existing tests pass locally with your changes?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

Adrian Wälchli and others added 2 commits August 30, 2020 16:59
@awaelchli awaelchli added the bug Something isn't working label Aug 30, 2020
@awaelchli awaelchli changed the title limit auto scaling batch size to the size of the training dataset [blocked by #3266] limit auto scaling batch size to the size of the training dataset Aug 30, 2020
@codecov
Copy link

codecov bot commented Aug 30, 2020

Codecov Report

Merging #3271 into master will increase coverage by 0%.
The diff coverage is 82%.

@@          Coverage Diff           @@
##           master   #3271   +/-   ##
======================================
  Coverage      90%     90%           
======================================
  Files          98      98           
  Lines        8069    8072    +3     
======================================
+ Hits         7279    7282    +3     
  Misses        790     790           

@awaelchli awaelchli changed the title [blocked by #3266] limit auto scaling batch size to the size of the training dataset limit auto scaling batch size to the size of the training dataset Sep 3, 2020
@awaelchli awaelchli marked this pull request as ready for review September 3, 2020 20:32
@mergify mergify bot requested a review from a team September 3, 2020 20:32
@mergify mergify bot requested a review from a team September 3, 2020 23:32
@mergify mergify bot requested a review from a team September 4, 2020 09:21
@mergify
Copy link
Contributor

mergify bot commented Sep 7, 2020

This pull request is now in conflict... :(

@awaelchli awaelchli mentioned this pull request Sep 7, 2020
7 tasks
Copy link
Member

@SkafteNicki SkafteNicki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@SkafteNicki SkafteNicki added the ready PRs ready to be merged label Sep 8, 2020
@mergify mergify bot requested a review from a team September 8, 2020 11:07
@Borda Borda force-pushed the bugfix/batchsize-finder-limit branch from 00e73c1 to 2e0218c Compare September 8, 2020 22:24
@Borda
Copy link
Member

Borda commented Sep 8, 2020

@awaelchli maybe I made a mistake in rebase, mind push your local branch before sync and resolve conflicts...

@mergify
Copy link
Contributor

mergify bot commented Sep 9, 2020

This pull request is now in conflict... :(

@awaelchli awaelchli force-pushed the bugfix/batchsize-finder-limit branch from 2e0218c to 00e73c1 Compare September 9, 2020 06:14
@awaelchli
Copy link
Contributor Author

awaelchli commented Sep 9, 2020

@Borda I had to redo all the changes again because the batch size scaling was all moved to a new place and now there appears to be a tuner class? I hope this bugfix can be merged before any further modifications and refactoring are made to the tuner.

Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Borda Borda merged commit e245065 into master Sep 9, 2020
@Borda Borda deleted the bugfix/batchsize-finder-limit branch September 9, 2020 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cap batch size by number of training samples when using auto_scale_batch_size
4 participants