Attach data refactor and tuner bugs [4/n] by carmocca · Pull Request #7258 · Lightning-AI/pytorch-lightning

carmocca · 2021-04-28T15:59:18Z

What does this PR do?

Attach data refactor:

Each of the trainer functions is now responsible for calling attach_data to register their DataLoaders/DataModule.
No longer need to pass DataLoaders to trainer._launch()

Tuner bugs:

trainer.tuner.lr_find() and trainer.tuner.scale_batch_size() now calls trainer.tune() internally. This solves a bug where the trainer state was not set properly as we set it in trainer.tune()
trainer.tune() now has arguments for the different tuning functions.
trainer.tune() now returns the tuning result. The result is a {'scale_batch_size': Optional[int], 'lr_find': Optional[_LRFinder]}

The tuning changes are tied to the attach_data changes because the tuner relied on the setup_fit function which has been removed in this PR.

Before submitting

Was this discussed/approved via a GitHub issue? (not for typos and docs)
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?
Did you make sure to update the documentation with your changes? (if necessary)
[n/a] Did you write any new necessary tests? (not for typos and docs)
Did you verify new and existing tests pass locally with your changes?
Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Is this pull request ready for review? (if not, please submit in draft mode)
Check that all items from Before submitting are resolved
Make sure the title is self-explanatory and the description concisely explains the PR
Add labels and milestones (and optionally projects) to the PR so it can be classified

pep8speaks · 2021-04-28T15:59:22Z

Hello @carmocca! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-04-30 13:34:56 UTC

codecov · 2021-04-28T16:00:41Z

Codecov Report

Merging #7258 (70a16ea) into master (ea2287e) will decrease coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #7258   +/-   ##
======================================
- Coverage      91%     91%   -0%     
======================================
  Files         199     199           
  Lines       12808   12807    -1     
======================================
- Hits        11688   11686    -2     
- Misses       1120    1121    +1

kaushikb11 · 2021-04-28T21:44:22Z

@carmocca

Why is this one PR doing so many things?

Non-exhaustive typing
Moved _validate_data_hooks into ConfigValidator
Each of the trainer functions is now responsible of calling attach_data to register their DataLoaders/DataModule.
No longer need to pass DataLoaders to _launch
Deprecate trainer.tuner.lr_find() and trainer.tuner.scale_batch_size() in favor of trainer.tune(). This solves a bug where the trainer state was not set properly as we set it in trainer.tune()
trainer.tune() now has arguments for the different tuning functions.
trainer.tune() now returns the tuning result.
Fixed bug where scale_batch_size would fail if the number of trials was 0

carmocca · 2021-04-28T21:49:12Z

Why is this one PR doing so many things?

All of those things are tied together. I just wrote the list of changes so it's easier for reviewers to follow.

edit: clarified the change list at the top and opened #7262 with parts of this PR

Co-authored-by: Nicki Skafte <skaftenicki@gmail.com>

awaelchli · 2021-04-30T00:58:43Z

+        # links data to the trainer
+        self.data_connector.attach_data(model, val_dataloaders=val_dataloaders, datamodule=datamodule)


Side question: In trainer.fit we allow trainer.fit(datamodule) but not for test and validate. Do you know why? Did we decide explicit naming by keyword is better?

Good eye. I actually plan to open a PR adding this.

Not doing it yet as it would give myself conflicts

kaushikb11 · 2021-04-30T12:28:11Z

+
+            max_lr: maximum learning rate to investigate
+
+            num_training: number of learning rates to test


Not related to this PR. num_training param doesn't seem intuitive.

cc: @SkafteNicki

I'm just moving the docstring around here

kaushikb11

LGTM!

Co-authored-by: Kaushik B <45285388+kaushikb11@users.noreply.github.com>

tchaton

LGMT !

carmocca added 10 commits April 27, 2021 14:13

_fit_impl refactor and types

395ce2a

Fix return

65aa690

Remove return docstring

b3f3314

Fixes

e7c3657

Fixes

8060424

Merge branch 'master' into use-fit-impl

c9a9ca9

Undo results change

67d5ca8

Revert changes for a separate PR

d7fbc5d

WIP

de7937e

Merge branch 'master' into attach-data-refactor

449041c

carmocca added refactor tuner labels Apr 28, 2021

carmocca added this to the v1.3 milestone Apr 28, 2021

carmocca self-assigned this Apr 28, 2021

Progress

405082b

carmocca changed the title ~~[WIP] Attach data refactor and tuner bugs.~~ [WIP] Attach data refactor and tuner bugs [3/n] Apr 28, 2021

carmocca added 3 commits April 28, 2021 19:50

Deprecation messages

c255388

Merge branch 'master' into attach-data-refactor

abc024f

Fixes

d2a54d6

carmocca added 2 commits April 29, 2021 00:11

Move copy_trainer_model_properties

8641504

Code cleaning in preparation for 7258

d8b0bf5

carmocca changed the title ~~[WIP] Attach data refactor and tuner bugs [3/n]~~ [WIP] Attach data refactor and tuner bugs [4/n] Apr 28, 2021

carmocca mentioned this pull request Apr 28, 2021

Code cleaning in preparation for #7258 [3/n] #7262

Merged

10 tasks

carmocca added 3 commits April 29, 2021 00:45

Update CHANGELOG

1eeedac

Fix test

5c62b3a

Merge branch 'changes-for-7258' into attach-data-refactor

ae89837

carmocca commented Apr 29, 2021

View reviewed changes

Comment thread pytorch_lightning/trainer/trainer.py Outdated

Apply suggestions from code review

4502329

Co-authored-by: Nicki Skafte <skaftenicki@gmail.com>

carmocca added the bug Something isn't working label Apr 29, 2021

awaelchli reviewed Apr 30, 2021

View reviewed changes

carmocca added 3 commits April 30, 2021 13:40

Undo deprecation

f2ccf21

Fix docs

9cbc154

Fix docs

46a4e92

carmocca commented Apr 30, 2021

View reviewed changes

Comment thread docs/source/advanced/lr_finder.rst Outdated

Comment thread docs/source/advanced/training_tricks.rst Outdated

Apply suggestions from code review

175e2cd

SkafteNicki approved these changes Apr 30, 2021

View reviewed changes

carmocca added the ready to be merged PRs ready to be merged label Apr 30, 2021

kaushikb11 reviewed Apr 30, 2021

View reviewed changes

Comment thread pytorch_lightning/trainer/trainer.py Outdated

kaushikb11 reviewed Apr 30, 2021

View reviewed changes

Comment thread pytorch_lightning/tuner/tuning.py

kaushikb11 reviewed Apr 30, 2021

View reviewed changes

Comment thread tests/trainer/test_trainer.py Outdated

kaushikb11 approved these changes Apr 30, 2021

View reviewed changes

mergify Bot added the has conflicts label Apr 30, 2021

carmocca and others added 2 commits April 30, 2021 14:42

Apply suggestions from code review

23f2da2

Co-authored-by: Kaushik B <45285388+kaushikb11@users.noreply.github.com>

Merge branch 'master' into attach-data-refactor

f2ab21c

mergify Bot added has conflicts and removed has conflicts labels Apr 30, 2021

carmocca added 2 commits April 30, 2021 14:48

Update scale_batch_size mode docstring

841381c

Merge branch 'master' into attach-data-refactor

ae9d7e0

mergify Bot removed the has conflicts label Apr 30, 2021

tchaton approved these changes Apr 30, 2021

View reviewed changes

Merge branch 'master' into attach-data-refactor

70a16ea

carmocca enabled auto-merge (squash) April 30, 2021 13:35

carmocca merged commit 5af086a into master Apr 30, 2021

carmocca deleted the attach-data-refactor branch April 30, 2021 13:54

carmocca mentioned this pull request May 3, 2021

[WIP] Stricter argument hygiene for passing datamodules to trainer.fit or t… #7329

Closed

11 tasks

		# links data to the trainer
		self.data_connector.attach_data(model, val_dataloaders=val_dataloaders, datamodule=datamodule)


		max_lr: maximum learning rate to investigate

		num_training: number of learning rates to test

Conversation

carmocca commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Before submitting

PR review

Uh oh!

pep8speaks commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2021-04-30 13:34:56 UTC

Uh oh!

codecov Bot commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kaushikb11 commented Apr 28, 2021

Uh oh!

carmocca commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

awaelchli Apr 30, 2021

Choose a reason for hiding this comment

Uh oh!

carmocca Apr 30, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kaushikb11 Apr 30, 2021

Choose a reason for hiding this comment

Uh oh!

carmocca Apr 30, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kaushikb11 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tchaton left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

carmocca commented Apr 28, 2021 •

edited

Loading

pep8speaks commented Apr 28, 2021 •

edited

Loading

codecov Bot commented Apr 28, 2021 •

edited

Loading

carmocca commented Apr 28, 2021 •

edited

Loading

kaushikb11 left a comment •

edited

Loading