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.

Sign up for GitHub

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

Jump to bottom

Fix LightningDataModule hparams parsing #12806

Merged
merged 19 commits into from
Aug 25, 2022
Merged

Fix LightningDataModule hparams parsing #12806

merged 19 commits into from
Aug 25, 2022

Conversation

tanmoyio
Copy link
Contributor

@tanmoyio tanmoyio commented Apr 19, 2022 •
edited by rohitgr7
Loading

What does this PR do?

Updates DataModule hparams for tuner.scale_batch_size()

Fixes #12768

The actual fix was that we need to correctly parse datamodule hparams too. check the review for a better understanding.

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

No

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

  • 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?

Make sure you had fun coding 🙃

@tanmoyio
Copy link
Contributor Author

@awaelchli I have tried this, without touching the _lightning_get_all_attr_holders, and this seems to work.

@akihironitta akihironitta mentioned this pull request Apr 23, 2022
Closed
12 tasks
@awaelchli awaelchli added bug Something isn't working tuner labels Apr 24, 2022
@awaelchli awaelchli added this to the 1.6.x milestone Apr 24, 2022
@awaelchli
Copy link
Contributor

awaelchli commented Apr 24, 2022 •
edited
Loading

This looks great! Would you mind extending the test test_auto_scale_batch_size_set_model_attribute in tests/tuner/test_scale_batch_size.py with a call to self.save_hyperparameters in the datamodule and adding the necessary assertion?

@awaelchli awaelchli self-assigned this Apr 24, 2022
@tanmoyio
Copy link
Contributor Author

@awaelchli sure

@akihironitta akihironitta added the community This PR is from the community label Apr 26, 2022
@akihironitta
Copy link
Contributor

Hi @tanmoyio! Thanks for working on this!

Here's the TODO list in this PR!

Let us know if you need help!

@tanmoyio tanmoyio force-pushed the bug/12768_datamodule_hparam_update branch from b8c09d8 to e9d5205 Compare May 6, 2022 17:34
@tanmoyio
Copy link
Contributor Author

tanmoyio commented May 6, 2022

@akihironitta I have added the unit tests, let me know if I need to modify anything, then I modify the changelog when everything is ready to go.

@akihironitta
Copy link
Contributor

@tanmoyio Would you mind having a look at the test failures? Also, could you update the changelog?

@tanmoyio
Copy link
Contributor Author

tanmoyio commented Jun 2, 2022

@akihironitta sorry, I should have done that earlier, will take a look into that.

@mergify mergify bot removed the has conflicts label Jun 27, 2022
@rohitgr7
Copy link
Contributor

@tanmoyio

we should not consider batch_size anywhere and only rely on batch_arg_name for all the cases.

@tanmoyio
Copy link
Contributor Author

Thanks @rohitgr7 will modify it that way

@carmocca carmocca removed this from the pl:1.6.x milestone Jul 28, 2022
@rohitgr7 rohitgr7 requested a review from otaj as a code owner August 22, 2022 17:12
@github-actions github-actions bot added the pl Generic label for PyTorch Lightning package label Aug 22, 2022
@mergify mergify bot removed the has conflicts label Aug 22, 2022
@rohitgr7 rohitgr7 changed the title tuner.scale_batch_size() to update DataModule hparams Fix LightningDataModule hparams parsing Aug 22, 2022
@rohitgr7 rohitgr7 added this to the pl:1.7.x milestone Aug 22, 2022
@mergify mergify bot added the ready PRs ready to be merged label Aug 23, 2022
@awaelchli awaelchli enabled auto-merge (squash) August 23, 2022 20:52
@awaelchli awaelchli merged commit 8074358 into Lightning-AI:master Aug 25, 2022
rohitgr7 added a commit that referenced this pull request Aug 26, 2022
@tanmoyio @akihironitta
@Borda @rohitgr7
Fix LightningDataModule hparams parsing (#12806)
d33a334 
Co-authored-by: Akihiro Nitta <[email protected]>
Co-authored-by: Jirka <[email protected]>
Co-authored-by: Rohit Gupta <[email protected]>
rohitgr7 added a commit that referenced this pull request Aug 26, 2022
@tanmoyio @akihironitta
@Borda @rohitgr7
Fix LightningDataModule hparams parsing (#12806)
9d5ff59 
Co-authored-by: Akihiro Nitta <[email protected]>
Co-authored-by: Jirka <[email protected]>
Co-authored-by: Rohit Gupta <[email protected]>
lexierule pushed a commit that referenced this pull request Aug 31, 2022
@tanmoyio @akihironitta
@Borda @rohitgr7 @lexierule
Fix LightningDataModule hparams parsing (#12806)
c762804 
Co-authored-by: Akihiro Nitta <[email protected]>
Co-authored-by: Jirka <[email protected]>
Co-authored-by: Rohit Gupta <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rohitgr7 rohitgr7 rohitgr7 approved these changes

@carmocca carmocca carmocca approved these changes

@awaelchli awaelchli awaelchli approved these changes

@SkafteNicki SkafteNicki Awaiting requested review from SkafteNicki

@Borda Borda Awaiting requested review from Borda Borda is a code owner

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

@tchaton tchaton Awaiting requested review from tchaton tchaton is a code owner

@SeanNaren SeanNaren Awaiting requested review from SeanNaren

@justusschock justusschock Awaiting requested review from justusschock justusschock is a code owner

@kaushikb11 kaushikb11 Awaiting requested review from kaushikb11

@otaj otaj Awaiting requested review from otaj

Assignees

@rohitgr7 rohitgr7

Labels
bug Something isn't working community This PR is from the community pl Generic label for PyTorch Lightning package ready PRs ready to be merged tuner
Projects
No open projects
Frameworks Planning
Status: Done
Milestone
pl:1.7.x
Development

Successfully merging this pull request may close these issues.

tuner.scale_batch_size does not update DataModule hparams
6 participants
@tanmoyio @awaelchli @akihironitta @rohitgr7 @carmocca @Borda