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.

Sign up for GitHub

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

Jump to bottom

fix get dataloader size #2375

Merged
merged 2 commits into from
Jun 26, 2020
Merged

fix get dataloader size #2375

merged 2 commits into from
Jun 26, 2020

Conversation

Borda
Copy link
Member

@Borda Borda commented Jun 26, 2020 •
edited
Loading

What does this PR do?

smhow the master is still failing

Fixes #2293 #2307

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 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 🙃

@Borda Borda added bug Something isn't working priority: 0 High priority task labels Jun 26, 2020
@mergify mergify bot requested a review from a team June 26, 2020 16:49
@Borda Borda mentioned this pull request Jun 26, 2020
Merged
7 tasks
awaelchli
awaelchli reviewed Jun 26, 2020
View reviewed changes
pytorch_lightning/trainer/data_loading.py
Comment on lines +288 to +291
try:
num_batches = len(dataloader)
except (TypeError, NotImplementedError):
num_batches = float('inf')
Copy link
Member

@awaelchli awaelchli Jun 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
try:
num_batches = len(dataloader)
except (TypeError, NotImplementedError):
num_batches = float('inf')
num_batches = len(dataloader) if callable(getattr(dataloader, "__len__", None)) else float("inf")

Wouldn't that be better?

Copy link
Member

@awaelchli awaelchli Jun 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think it is bad to use exceptions for control flow like this. If the user implements len but has a type error, we catch it and the program crashes somewhere else, making it much harder for the user to debug their dataset. (#2266 )

Copy link
Member Author

Choose a reason for hiding this comment

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

seems like the suggestion did not work, so reverting it back... :(

Copy link
Member

Choose a reason for hiding this comment

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

:(

Copy link
Member

@awaelchli awaelchli Jun 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

what was the error? (we have a _has_len defined in the same file btw)

Copy link
Member

Choose a reason for hiding this comment

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

what about num_batches = len(dataloader) if _has_len(dataloader) else float('inf')?

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be a longterm solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

@awaelchli see here - https://github.com/PyTorchLightning/pytorch-lightning/runs/812000184

Copy link
Member Author

Choose a reason for hiding this comment

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

the problem is raising an exception in the __len__

Copy link
Member

Choose a reason for hiding this comment

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

yeah my orignal suggestions was wrong sorry. my new suggestion is to use the already existing _has_len method
num_batches = len(dataloader) if _has_len(dataloader) else float('inf')
but not a big deal, would just be cleaner.

tests/trainer/test_lr_finder.py
@@ -155,7 +154,7 @@ def test_accumulation_and_early_stopping(tmpdir):
'Learning rate was not altered after running learning rate finder'
assert len(lrfinder.results['lr']) == 100, \
'Early stopping for learning rate finder did not work'
assert lrfinder._total_batch_idx == 100 * 2, \
assert lrfinder._total_batch_idx == 190, \
Copy link
Member

Choose a reason for hiding this comment

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

how is this related?

Copy link
Member Author

Choose a reason for hiding this comment

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

not really but the test was skipped completely

@mergify mergify bot requested a review from a team June 26, 2020 17:00
@Borda Borda mentioned this pull request Jun 26, 2020
Merged
7 tasks
@Borda Borda requested a review from awaelchli June 26, 2020 19:08
thschaaf
thschaaf approved these changes Jun 26, 2020
View reviewed changes
Copy link
Contributor

@thschaaf thschaaf left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@mergify mergify bot requested a review from a team June 26, 2020 19:18
@Borda Borda added the ready PRs ready to be merged label Jun 26, 2020
@williamFalcon williamFalcon merged commit a5f4578 into master Jun 26, 2020
@Borda Borda deleted the bugfix/dataloaders branch June 26, 2020 19:44
@HHousen HHousen mentioned this pull request Jul 3, 2020
Closed
7 tasks
williamFalcon pushed a commit that referenced this pull request Jul 9, 2020
@HHousen
Start accumulate gradients schedule at epoch 0 (continued) (#2513)
992a7e2 
* Start accumulate gradients schedule at epoch 0

* Undo change in #2375

* Update test_trainer.py::test_gradient_accumulation_scheduling

* Fix pep8 formatting

* Remove 'Datasets/' folder

* Split args for readability

* Fix pep8 formatting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awaelchli awaelchli awaelchli approved these changes

@thschaaf thschaaf thschaaf approved these changes

@jeremyjordan jeremyjordan Awaiting requested review from jeremyjordan

@SkafteNicki SkafteNicki Awaiting requested review from SkafteNicki

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

Assignees
No one assigned
Labels
bug Something isn't working priority: 0 High priority task ready PRs ready to be merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Borda @awaelchli @thschaaf @williamFalcon