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

[RFC] Remove has_{setup/teardown}_{stage} properties from DataModule #7301

Closed
ananthsub opened this issue Apr 30, 2021 · 2 comments · Fixed by #7657
Closed

[RFC] Remove has_{setup/teardown}_{stage} properties from DataModule #7301

ananthsub opened this issue Apr 30, 2021 · 2 comments · Fixed by #7657
Assignees
Labels
design Includes a design discussion discussion In a discussion stage refactor

Comments

@ananthsub
Copy link
Contributor

ananthsub commented Apr 30, 2021

🚀 Feature

Carrying forward discussion from #7289 (comment)

Remove these settings to give more control to the users and making trainer execution easier
https://github.com/PyTorchLightning/pytorch-lightning/blob/490cc57809ebeba19003b4101393a8a058217c31/pytorch_lightning/core/datamodule.py#L33-L52

Motivation

Consecutive calls like this have different behavior:

dm = MyDataModule()
model = MyLightningModule()
trainer.fit(model, datamodule=dm)
trainer.fit(model, datamodule=dm)

The second trainer.fit call will not run the prepare data, setup, or teardown hooks.

Removing this check gives us these advantages

  1. Simpler, consistent hook execution: More control for users for how they want to handle state changes
  2. Less state management inside the trainer and datamodule: less framework overhead/maintenance for us long term and easier debuggability

cc @carmocca

Pitch

Remove this code:

cc @carmocca

Alternatives

Additional context

@ananthsub ananthsub added feature Is an improvement or enhancement help wanted Open to be worked on refactor labels Apr 30, 2021
@carmocca carmocca added design Includes a design discussion discussion In a discussion stage and removed feature Is an improvement or enhancement help wanted Open to be worked on labels May 3, 2021
@kandluis
Copy link
Contributor

kandluis commented May 7, 2021

+1 that this would be helpful in simplifying the overhead of the DataModule API.

@carmocca
Copy link
Contributor

Continuing discussion from @tchaton #7238 (review):

I think this is an important feature as preparing a dataset can be costly and shouldn't be done several times.
How do you propose to solve this problem if we remove those properties ?

The argument is that the users should implement the checks for "should download/prepare" themselves, giving them more power and responsibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Includes a design discussion discussion In a discussion stage refactor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants