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

Sequential CombinedLoader to flatten the eval and predict loops #16726

Merged
merged 54 commits into from
Feb 17, 2023

Conversation

carmocca
Copy link
Contributor

@carmocca carmocca commented Feb 11, 2023

What does this PR do?

  • Removes the _EvaluationEpochLoop
  • Removes the _PredictionEpochLoop
  • Removes the _DataLoaderLoop
  • Moves the _EvaluationLoop out of the dataloader directory
  • Moves the _PredictionLoop out of the dataloader directory
  • Adds a loop.setup_data() method to the top-level loops. This method is called at the beginning of loop.run(). This replaces the trainer.reset_*_dataloader methods.
  • Removes the trainer.reset_*_dataloader methods and related trainer.py simplifications
  • The top-level loops now own their _DataLoaderSource and CombinedLoader
  • The trainer.*_dataloader properties now return what the user returned in their Module.*_dataloader() hook. Nit: validation dataloaders are casted to list, see follow-up section
  • Adds support for predict_step(dataloader_iter). this is not an important objective of the PR, but it's implemented for free.

Fixes #10696
Fixes #11845

Follow-ups:

cc @justusschock @awaelchli @Borda @carmocca

@carmocca carmocca self-assigned this Feb 11, 2023
@github-actions github-actions bot added the pl Generic label for PyTorch Lightning package label Feb 11, 2023
@github-actions github-actions bot added the fabric lightning.fabric.Fabric label Feb 13, 2023
@mergify mergify bot removed the has conflicts label Feb 16, 2023
@Borda Borda changed the title Sequential CombinedLoader to flatten the evaluation and prediction loops Sequential CombinedLoader to flatten the evaluation and prediction loops Feb 17, 2023
src/lightning/pytorch/loops/fit_loop.py Outdated Show resolved Hide resolved
src/lightning/pytorch/loops/prediction_loop.py Outdated Show resolved Hide resolved
@mergify mergify bot removed the has conflicts label Feb 17, 2023
Copy link
Member

@justusschock justusschock left a comment

Choose a reason for hiding this comment

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

That's a monster to review but seems pretty good. Only one question I might have missed due to the massive amount of changes I just tried to follow and a few mypy nits :)

A strong LGTM from me!

@mergify mergify bot added the ready PRs ready to be merged label Feb 17, 2023
Copy link
Member

@awaelchli awaelchli left a comment

Choose a reason for hiding this comment

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

LGTM. This looks very promising.

tests/tests_pytorch/trainer/test_trainer.py Outdated Show resolved Hide resolved
src/lightning/pytorch/loops/fit_loop.py Show resolved Hide resolved
@awaelchli awaelchli added this to the 2.0 milestone Feb 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Includes a breaking change data handling Generic data-related topic loops Related to the Loop API pl Generic label for PyTorch Lightning package ready PRs ready to be merged refactor
Projects
None yet
4 participants