Skip to content

Conversation

@awaelchli
Copy link
Contributor

@awaelchli awaelchli commented Feb 14, 2021

What does this PR do?

Fixes #5967

The tuner tries to detect if the model has an attribute for batch size, however before the trainer attaches the datamodule.
Hence the batch size attribute can't be found.

The newly added test fails on master.

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
  • Check that target branch and milestone match!

Did you have fun?

Make sure you had fun coding 🙃

@codecov
Copy link

codecov bot commented Feb 14, 2021

Codecov Report

Merging #5968 (840f74b) into master (34b733b) will decrease coverage by 3%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #5968    +/-   ##
=======================================
- Coverage      94%     91%    -3%     
=======================================
  Files         161     161            
  Lines       11474   11368   -106     
=======================================
- Hits        10731   10339   -392     
- Misses        743    1029   +286     

@awaelchli awaelchli added bug Something isn't working data handling Generic data-related topic tuner labels Feb 14, 2021
@stale
Copy link

stale bot commented Feb 28, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If you need further help see our docs: https://pytorch-lightning.readthedocs.io/en/latest/generated/CONTRIBUTING.html#pull-request or ask the assistance of a core contributor here or on Slack. Thank you for your contributions.

@stale stale bot added the won't fix This will not be worked on label Feb 28, 2021
@edenlightning
Copy link
Contributor

@awaelchli any plans to get this in?

@awaelchli
Copy link
Contributor Author

Yes I try to get it done by the end of this week.

@awaelchli awaelchli marked this pull request as ready for review March 6, 2021 02:35
@awaelchli awaelchli requested a review from williamFalcon as a code owner March 6, 2021 02:35
Copy link
Contributor

@carmocca carmocca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@awaelchli
Copy link
Contributor Author

Mildly related, why do we have this loop?

legacy stuff, pre- accelerator refactor
the loop goes over the lightning module and the potentially wrapped lightning module.

this model connector class will be refactored-away.
In the end, the only thing we will ever want to set directly on the model is the trainer reference :)
and all others can be derived through it

Copy link
Contributor

@rohitgr7 rohitgr7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have few tests related to tuner here: https://github.com/PyTorchLightning/pytorch-lightning/blob/master/tests/trainer/test_trainer_tricks.py

These should be added to tests/tuner too.

Else LGTM ✌️

@awaelchli awaelchli added this to the 1.2.x milestone Mar 6, 2021
@awaelchli awaelchli added the ready PRs ready to be merged label Mar 6, 2021
@carmocca carmocca enabled auto-merge (squash) March 6, 2021 23:39
Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix !

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 ready PRs ready to be merged tuner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"Wrong" code example in doc auto-scaling-of-batch-size section

7 participants