Better error message when dataloader or datamodule is None #14614

awaelchli · 2022-09-08T18:06:50Z

What does this PR do?

Does your PR introduce any breaking changes? If yes, please list them.

Yes, when the user set None explicitly before but had dataloader methods implemented in the module.
But I couldn't find another way to provide a meaningful error message.

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

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

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

Did you have fun?

I made sure I had fun coding 🙃

awaelchli · 2022-09-09T09:32:10Z

src/pytorch_lightning/utilities/types.py

+class _NO_DATA(Sentinel):
+    """A sentinel representing the default value for 'no dataloader passed to Trainer method'."""


Note: Sometimes we see sentinels defined as

DEFAULT = object()

But this does not work for us, because pickling (e.g. ddp_spawn) will re-instantiate this object and then checks like dataloader is DEFAULT have the wrong value!

for more information, see https://pre-commit.ci

carmocca · 2022-09-09T12:15:21Z

tests/tests_pytorch/trainer/test_config_validator.py

+    with pytest.raises(ValueError, match="You explicitly passed .*dataloaders=None.* but this is not supported"):
+        trainer_fn = getattr(trainer, trainer_fn)


This should be outside the raises block

Suggested change

with pytest.raises(ValueError, match="You explicitly passed .*dataloaders=None.* but this is not supported"):

trainer_fn = getattr(trainer, trainer_fn)

trainer_fn = getattr(trainer, trainer_fn)

with pytest.raises(ValueError, match="You explicitly passed .*dataloaders=None.* but this is not supported"):

carmocca · 2022-09-09T12:15:33Z

tests/tests_pytorch/trainer/test_config_validator.py

+    with pytest.raises(ValueError, match="You explicitly passed .*datamodule=None.* but this is not supported"):
+        trainer_fn = getattr(trainer, trainer_fn)


Suggested change

with pytest.raises(ValueError, match="You explicitly passed .*datamodule=None.* but this is not supported"):

trainer_fn = getattr(trainer, trainer_fn)

trainer_fn = getattr(trainer, trainer_fn)

with pytest.raises(ValueError, match="You explicitly passed .*datamodule=None.* but this is not supported"):

carmocca · 2022-09-09T12:19:20Z

src/pytorch_lightning/trainer/configuration_validator.py

+
+def _check_dataloader_none(stage: str, **dataloader_args: Any) -> None:
+    for arg_name, value in dataloader_args.items():
+        dataloader_method = arg_name if not arg_name.endswith("s") else arg_name[:-1]


If you pass the RunningStage object instead of the value, you can use stage.dataloader_prefix to construct the method name

carmocca · 2022-09-09T12:22:21Z

src/pytorch_lightning/CHANGELOG.md

@@ -61,6 +61,9 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 - When using multiple loggers, by default checkpoints and profiler output now get saved to the log dir of the first logger in the list ([14325](https://github.com/Lightning-AI/lightning/pull/14325))


+- Explicitly passing `train_dataloaders=None`, `val_dataloaders=None`, `dataloaders=None` or `datamodule=None` is officially no longer supported and will inform using a better error message than before ([14614](https://github.com/Lightning-AI/lightning/pull/14614))


IMO this is a huge breaking change as described in #14602 (comment).

Very often people write scripts with

train_dataloader = None if ...: train_dataloader = ... trainer.fit(model, train_dataloader=train_dataloader)

(you had to update examples like this in our CI)

I don't think we can afford to break this usecase, sure, it can be error-prone, but it's a perfectly valid Python pattern when you want to pass arguments optionally.

I strongly think it should be a PossibleUserWarning for this reason.

If we make it a warning, then another error will raise later on and completely overshadow any warning printed earlier. It won't help the user.

The only other option I see is to change the message in "No trainining_step defined ..." to hint at the possibility that one of the dataloaders is None. But this is very strange. Ideally we would want to tell the user directly what the issue is, not a list of possibilities.

Another possibility would be to check at the same time whether there' a *_dataloader fallback. If there is, allow None and proceed normally. Otherwise, raise an error.

If we do this, we should also re-write the "No train_dataloader() method defined" error as it would be covered by this new error

I have an alternative implementation here: #14637

My brain is melting.

Thanks! Much better

awaelchli added 2 commits September 8, 2022 14:31

wip

239d2a9

update

c0c363e

github-actions bot added the pl Generic label for PyTorch Lightning package label Sep 8, 2022

more

2e16932

awaelchli force-pushed the bugfix/no-dataloader branch from d8ba236 to 2e16932 Compare September 8, 2022 18:14

awaelchli added priority: 0 High priority task trainer: fit labels Sep 8, 2022

awaelchli added this to the pl:1.7.x milestone Sep 8, 2022

awaelchli self-assigned this Sep 8, 2022

sentinel that can be pickled

ff700dd

awaelchli changed the title ~~Adjust error message when dataloader has unsupported type~~ Adjust error message when dataloader is None Sep 8, 2022

awaelchli added 3 commits September 9, 2022 00:30

add changelog

f1917bb

fixes

796d0b5

add tests

e6194f5

awaelchli changed the title ~~Adjust error message when dataloader is None~~ Better error message when dataloader or datamodule is None Sep 9, 2022

awaelchli marked this pull request as ready for review September 9, 2022 09:29

awaelchli requested review from williamFalcon, tchaton, Borda, carmocca, SkafteNicki, justusschock, kaushikb11, rohitgr7 and otaj as code owners September 9, 2022 09:29

reset bugreport model

2f95973

awaelchli commented Sep 9, 2022

View reviewed changes

awaelchli and others added 2 commits September 9, 2022 11:34

fix literal import

12cea42

[pre-commit.ci] auto fixes from pre-commit.com hooks

d6c8d1c

for more information, see https://pre-commit.ci

carmocca reviewed Sep 9, 2022

View reviewed changes

Borda approved these changes Sep 9, 2022

View reviewed changes

Merge branch 'master' into bugfix/no-dataloader

ac68a08

Borda requested a review from carmocca September 9, 2022 19:17

awaelchli mentioned this pull request Sep 10, 2022

Better error message when dataloader and datamodule is None (V2) #14637

Merged

11 tasks

awaelchli closed this in #14637 Sep 13, 2022

carmocca deleted the bugfix/no-dataloader branch September 13, 2022 12:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better error message when dataloader or datamodule is None #14614

Better error message when dataloader or datamodule is None #14614

awaelchli commented Sep 8, 2022 •

edited

Loading

awaelchli Sep 9, 2022 •

edited

Loading

carmocca Sep 9, 2022

carmocca Sep 9, 2022

carmocca Sep 9, 2022

carmocca Sep 9, 2022

awaelchli Sep 9, 2022 •

edited

Loading

carmocca Sep 9, 2022 •

edited

Loading

awaelchli Sep 10, 2022

carmocca Sep 11, 2022

		class _NO_DATA(Sentinel):
		"""A sentinel representing the default value for 'no dataloader passed to Trainer method'."""

		with pytest.raises(ValueError, match="You explicitly passed .dataloaders=None. but this is not supported"):
		trainer_fn = getattr(trainer, trainer_fn)

		@@ -61,6 +61,9 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
		- When using multiple loggers, by default checkpoints and profiler output now get saved to the log dir of the first logger in the list ([14325](https://github.com/Lightning-AI/lightning/pull/14325))


		- Explicitly passing `train_dataloaders=None`, `val_dataloaders=None`, `dataloaders=None` or `datamodule=None` is officially no longer supported and will inform using a better error message than before ([14614](https://github.com/Lightning-AI/lightning/pull/14614))

Better error message when dataloader or datamodule is None #14614

Better error message when dataloader or datamodule is None #14614

Conversation

awaelchli commented Sep 8, 2022 • edited Loading

What does this PR do?

Does your PR introduce any breaking changes? If yes, please list them.

Before submitting

PR review

Did you have fun?

awaelchli Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

carmocca Sep 9, 2022

Choose a reason for hiding this comment

carmocca Sep 9, 2022

Choose a reason for hiding this comment

carmocca Sep 9, 2022

Choose a reason for hiding this comment

carmocca Sep 9, 2022

Choose a reason for hiding this comment

awaelchli Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

carmocca Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

awaelchli Sep 10, 2022

Choose a reason for hiding this comment

carmocca Sep 11, 2022

Choose a reason for hiding this comment

awaelchli commented Sep 8, 2022 •

edited

Loading

awaelchli Sep 9, 2022 •

edited

Loading

awaelchli Sep 9, 2022 •

edited

Loading

carmocca Sep 9, 2022 •

edited

Loading