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

Missing LightningModule datamodule reference #6929

Closed
carmocca opened this issue Apr 9, 2021 · 8 comments · Fixed by #7168
Closed

Missing LightningModule datamodule reference #6929

carmocca opened this issue Apr 9, 2021 · 8 comments · Fixed by #7168
Assignees
Labels
bug Something isn't working data handling Generic data-related topic docs Documentation related good first issue Good for newcomers help wanted Open to be worked on priority: 1 Medium priority task
Milestone

Comments

@carmocca
Copy link
Contributor

carmocca commented Apr 9, 2021

🐛 Bug

This docs snippet does not work:

https://pytorch-lightning.readthedocs.io/en/latest/common/lightning_module.html#datamodule

To Reproduce

def test_bug(tmpdir):
    class TestModel(BoringModel):
        def configure_optimizers(self):
            # works
            len(self.trainer.datamodule.train_dataloader())
            # does not
            len(self.datamodule.train_dataloader())
            
            return super().configure_optimizers()

    dm = BoringDataModule()
    trainer = Trainer(default_root_dir=tmpdir, fast_dev_run=True)
    trainer.fit(TestModel(), datamodule=dm)
line 128, in configure_optimizers
    num_training_samples = len(self.datamodule.train_dataloader())
AttributeError: 'NoneType' object has no attribute 'train_dataloader'

Expected behavior

self.datamodule works

Alternatives

Update docs

Environment

master

Additional context

Reported by Brian Staber in Slack

@carmocca carmocca added bug Something isn't working help wanted Open to be worked on priority: 1 Medium priority task data handling Generic data-related topic labels Apr 9, 2021
@carmocca carmocca added this to the 1.2.x milestone Apr 9, 2021
@edenlightning edenlightning added the good first issue Good for newcomers label Apr 13, 2021
@mesejo
Copy link
Contributor

mesejo commented Apr 15, 2021

Can I work on this?

@ananthsub
Copy link
Contributor

@carmocca is the model expected to have a reference to the data module during training?

@carmocca
Copy link
Contributor Author

carmocca commented Apr 15, 2021

Can I work on this?

Sure!

@carmocca is the model expected to have a reference to the data module during training?

I guess so, given the docs link above and the fact that it has a property for it

https://github.com/PyTorchLightning/pytorch-lightning/blob/f29ecbfd909ff431ef837fcc8ebff451e897cb0b/pytorch_lightning/core/lightning.py#L164-L170

I personally wouldn't mind removing this and forcing access through the trainer.

@Borda Borda modified the milestones: 1.2.x, 1.3 Apr 18, 2021
@tchaton
Copy link
Contributor

tchaton commented Apr 19, 2021

Dear @mesejo,

Could you make a PR for this ?

Best,
T.C

@mesejo
Copy link
Contributor

mesejo commented Apr 20, 2021

Hi @carmocca, when you said:

I personally wouldn't mind removing this and forcing access through the trainer.

Were you referring to the datamodule property?

@carmocca
Copy link
Contributor Author

Were you referring to the datamodule property?

I was referring to deprecating model.datamodule in favor of model.trainer.datamodule

cc: @PyTorchLightning/core-contributors Any opinions about this? Should we keep both ways?

@awaelchli
Copy link
Contributor

I would prefer if we don't attach the datamodule to the LightningModule and instead reference it through trainer.

@carmocca
Copy link
Contributor Author

Okay, let's deprecate it then and update the docs/ocurrences

mesejo added a commit to mesejo/pytorch-lightning that referenced this issue Apr 22, 2021
mesejo added a commit to mesejo/pytorch-lightning that referenced this issue Apr 22, 2021
@edenlightning edenlightning added the docs Documentation related label Apr 27, 2021
carmocca added a commit that referenced this issue May 4, 2021
…ner one (#6929) (#7168)

Co-authored-by: Adrian Wälchli <[email protected]>
Co-authored-by: Carlos Mocholí <[email protected]>
kaushikb11 pushed a commit to kaushikb11/pytorch-lightning that referenced this issue May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working data handling Generic data-related topic docs Documentation related good first issue Good for newcomers help wanted Open to be worked on priority: 1 Medium priority task
Projects
None yet
7 participants