Add str method to datamodule #20301

MrWhatZitToYaa · 2024-09-25T18:06:57Z

What does this PR do?

Added a str function to the DataModule in order to be able to print Datasets included in the module instead of an adress.

Fixes #9947

Before submitting

Was this discussed/agreed 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 list all the breaking changes introduced by this pull request?
Did you update the CHANGELOG? (not for typos, docs, test updates, or minor internal changes/refactors)

PR review

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

Reviewer checklist

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

📚 Documentation preview 📚: https://pytorch-lightning--20301.org.readthedocs.build/en/20301/

MrWhatZitToYaa · 2024-09-25T18:09:05Z

Will add documentation once code is fixed

MrWhatZitToYaa · 2024-10-14T14:34:09Z

I have been waiting for quite some time. How do I go on? Do I just wait some more until someone looks at this?

lantiga · 2024-11-12T23:07:04Z

hey @MrWhatZitToYaa thanks for taking this up and sorry for the wait

can you add a bit more detail on what do we think it's good printing from a datamodule?

the original design in #9947 was a bit different, as was #9967, but happy to consider alternatives after there's some discussion on the rationale

tests/tests_pytorch/core/test_datamodules.py

MrWhatZitToYaa · 2024-11-20T20:43:34Z

hey @MrWhatZitToYaa thanks for taking this up and sorry for the wait

can you add a bit more detail on what do we think it's good printing from a datamodule?

the original design in #9947 was a bit different, as was #9967, but happy to consider alternatives after there's some discussion on the rationale

The idea originally came to me when I had to to use the Datamodule to implement some datasets we have in our lab. It was really annoying to deal with the fact that you never really know what you are dealing with and makes debugging really hard. I talked to some colleagues and they agreed that they ran into the same problem. That's why I would like to add a str functionality for the Datamodule: primarily for debugging / logging.
I think the simplest approach is to scan the whole module for instances of datasets and then return a string where each line contains the name of the dataset and the length if it is available. This would be a big quality of life improvement when dealing with / constructing Datamodules.

First implementation scetch

Added alternative Boring Data Module implementations Added test cases for all possible options Added additional check for NotImplementedError in string function of DataModule

Made changes to comply with requested suggestions Switched from hardcoded \n to more general os.linesep

Corrected the annotation for the internal function and the list that is suppsoed to store the information on the datasets

lantiga

Thanks for the contribution, I aligned it with the latest master and added a comment for a fix before we can merge. It should be quick.

src/lightning/pytorch/core/datamodule.py

codecov · 2024-11-25T09:36:08Z

Codecov Report

Attention: Patch coverage is 92.59259% with 6 lines in your changes missing coverage. Please review.

Project coverage is 79%. Comparing base (601c060) to head (36fdb57).
Report is 2 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (601c060) and HEAD (36fdb57). Click for more details.

HEAD has 187 uploads less than BASE

Flag BASE (601c060) HEAD (36fdb57)

cpu 68 23

lightning 51 17

pytest 46 0

python3.9 18 6

lightning_fabric 13 0

python3.10 9 3

python3.11 18 6

python3.12 23 8

gpu 2 0

pytest-full 24 23

pytorch2.4.1 3 2

Additional details and impacted files

@@            Coverage Diff            @@
##           master   #20301     +/-   ##
=========================================
- Coverage      88%      79%     -9%     
=========================================
  Files         267      264      -3     
  Lines       23304    23328     +24     
=========================================
- Hits        20407    18365   -2042     
- Misses       2897     4963   +2066

Fixed type annotation issue Reduced code size by using Sized object from abc library

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

Switched from Dataset based implementation to Dataloader based implementation

Added missing size value to tuple in the error case instead of returning only a string

Adjusted test to match the new implementation requirenemnts Added necessary BoringModules for tests Fixed bugs and annotation issues in the str method

Refactored code and made it more readable by implementing more abstarct fucntion methods Adjusted tests Removed debug statements Removed TODO comments

Renamed varaibles to more sensible names to increase readability

MrWhatZitToYaa · 2024-12-06T19:51:59Z

The string method will now check if any of the 4 dataloaders are available and try to print information on them. I hope this is how you imagined it to work

lantiga

That's correct. We're almost there, I propose we update the strings from

Train dataset: available=yes, size=unknown
Validation dataset: available=yes, size=64
Test dataset: available=no, size=unknown
Prediction dataset: available=yes, size=64

to

Train dataloader: size=NA
Validation dataloader: size=64
Test dataloader: None
Predict dataloader: size=64

summary:

dataset -> dataloader
Prediction -> Predict
remove "available"
unknown -> NA
"available=no" -> None

what do you think?

Switched name from dataset to dataloader Switched name Prediction to Predict removed available keyword and instead write None if not available Switched from unknown to NA

MrWhatZitToYaa · 2024-12-10T21:17:00Z

Alright, sounds good to me
I made the changes so I assume it complies with your specification now

src/lightning/pytorch/core/datamodule.py

github-actions bot added the pl Generic label for PyTorch Lightning package label Sep 25, 2024

MrWhatZitToYaa marked this pull request as ready for review October 3, 2024 15:22

MrWhatZitToYaa requested review from lantiga, Borda, tchaton and justusschock as code owners October 3, 2024 15:22

lantiga changed the title ~~Feature/9947 Add str method to datamodule~~ Add str method to datamodule Nov 12, 2024

lantiga added waiting on author Waiting on user action, correction, or update lightningdatamodule pl.LightningDataModule labels Nov 12, 2024

Borda requested changes Nov 13, 2024

View reviewed changes

tests/tests_pytorch/core/test_datamodules.py Outdated Show resolved Hide resolved

MrWhatZitToYaa requested review from williamFalcon and ethanwharris as code owners November 20, 2024 20:46

github-actions bot added docs Documentation related ci Continuous Integration fabric lightning.fabric.Fabric dependencies Pull requests that update a dependency file dockers package labels Nov 20, 2024

MrWhatZitToYaa added 5 commits November 20, 2024 16:06

Add feature implementation to datamodule for str method

a011366

First implementation scetch

Removed list / tuple case for datamodule str method

137e7b5

Added test cases for DataModule string function

efe0c3c

Added alternative Boring Data Module implementations Added test cases for all possible options Added additional check for NotImplementedError in string function of DataModule

Reverted accidental changes in DataModule

23326d7

Updated dataloader str method

122cf6d

Made changes to comply with requested suggestions Switched from hardcoded \n to more general os.linesep

MrWhatZitToYaa force-pushed the feature/9947_dataloader-string branch from 0c57055 to 122cf6d Compare November 20, 2024 21:07

lantiga and others added 2 commits November 21, 2024 00:49

Merge branch 'master' into feature/9947_dataloader-string

9acf680

Improvements to implementation of str method for datamodule

51a4901

Corrected the annotation for the internal function and the list that is suppsoed to store the information on the datasets

mergify bot removed the has conflicts label Nov 25, 2024

lantiga reviewed Nov 25, 2024

View reviewed changes

src/lightning/pytorch/core/datamodule.py Outdated Show resolved Hide resolved

MrWhatZitToYaa and others added 8 commits November 25, 2024 13:54

Implementing str method for datamodule

20a2a0c

Fixed type annotation issue Reduced code size by using Sized object from abc library

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

e03aefb

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

Add string method to datamodule

6d05cfe

Switched from Dataset based implementation to Dataloader based implementation

Implementing str mehtod for dataloader

acd2f73

Added missing size value to tuple in the error case instead of returning only a string

Implementing str fucntion for datamodule

b08e1fe

Adjusted test to match the new implementation requirenemnts Added necessary BoringModules for tests Fixed bugs and annotation issues in the str method

Implementing str method for datamodule

21029d2

Refactored code and made it more readable by implementing more abstarct fucntion methods Adjusted tests Removed debug statements Removed TODO comments

Finilized required adjustments for dataloader string proposal method

330a88c

Renamed varaibles to more sensible names to increase readability

Merge branch 'master' into feature/9947_dataloader-string

dbaabaa

MrWhatZitToYaa requested a review from lantiga December 6, 2024 19:49

lantiga removed ci Continuous Integration waiting on author Waiting on user action, correction, or update dependencies Pull requests that update a dependency file dockers package docs Documentation related labels Dec 8, 2024

lantiga reviewed Dec 8, 2024

View reviewed changes

MrWhatZitToYaa added 2 commits December 10, 2024 15:31

Merge branch 'master' into feature/9947_dataloader-string

5bf252e

Implementing str method

06e4f1c

Switched name from dataset to dataloader Switched name Prediction to Predict removed available keyword and instead write None if not available Switched from unknown to NA

MrWhatZitToYaa requested a review from lantiga December 10, 2024 21:17

Merge branch 'master' into feature/9947_dataloader-string

63200a0

lantiga approved these changes Dec 10, 2024

View reviewed changes

src/lightning/pytorch/core/datamodule.py Outdated Show resolved Hide resolved

src/lightning/pytorch/core/datamodule.py Outdated Show resolved Hide resolved

lantiga and others added 3 commits December 11, 2024 00:24

Update src/lightning/pytorch/core/datamodule.py

33cda77

Update src/lightning/pytorch/core/datamodule.py

f492167

Merge branch 'master' into feature/9947_dataloader-string

36fdb57

lantiga merged commit 9709c64 into Lightning-AI:master Dec 11, 2024
74 of 76 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add str method to datamodule #20301

Add str method to datamodule #20301

MrWhatZitToYaa commented Sep 25, 2024 •

edited

Loading

MrWhatZitToYaa commented Sep 25, 2024

MrWhatZitToYaa commented Oct 14, 2024

lantiga commented Nov 12, 2024

MrWhatZitToYaa commented Nov 20, 2024

lantiga left a comment

codecov bot commented Nov 25, 2024 •

edited

Loading

MrWhatZitToYaa commented Dec 6, 2024

lantiga left a comment

MrWhatZitToYaa commented Dec 10, 2024

Add str method to datamodule #20301

Add str method to datamodule #20301

Conversation

MrWhatZitToYaa commented Sep 25, 2024 • edited Loading

What does this PR do?

PR review

MrWhatZitToYaa commented Sep 25, 2024

MrWhatZitToYaa commented Oct 14, 2024

lantiga commented Nov 12, 2024

MrWhatZitToYaa commented Nov 20, 2024

lantiga left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 25, 2024 • edited Loading

Codecov Report

MrWhatZitToYaa commented Dec 6, 2024

lantiga left a comment

Choose a reason for hiding this comment

MrWhatZitToYaa commented Dec 10, 2024

MrWhatZitToYaa commented Sep 25, 2024 •

edited

Loading

codecov bot commented Nov 25, 2024 •

edited

Loading