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

Remove ModelSummary validation from train loop on_trainer_init #6610

Merged
merged 6 commits into from
Mar 24, 2021

Conversation

ananthsub
Copy link
Contributor

@ananthsub ananthsub commented Mar 20, 2021

What does this PR do?

  • Start adding typehints to training_loop.py beginning with on_trainer_init for better static analysis w/ mypy or Pyre
  • Remove duplication for weights_summary across TrainLoop and Trainer - the validation and property setting was happening in both places

Fixes #<issue_number>

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

Did you have fun?

Make sure you had fun coding 🙃

@pep8speaks
Copy link

pep8speaks commented Mar 20, 2021

Hello @ananthsub! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-03-21 03:34:59 UTC

ref_model.summarize(mode=self.weights_summary)
else:
raise MisconfigurationException("weights_summary can be None, " + ", ".join(ModelSummary.MODES))
ref_model.summarize(mode=self.weights_summary)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@edenafek @awaelchli @justusschock the model summary could be another (small) component we could split out from the core lightning module and offer as a standalone utility. the constructor for the summary object could take:

  • an nn.Module
  • precision
  • device
  • mode

connecting it back into the trainer here would be easy. it's almost there now with the ModelSummary object

Copy link
Contributor

Choose a reason for hiding this comment

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

Related idea: #4541

Copy link
Contributor

Choose a reason for hiding this comment

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

Btw it's already a standalone utility. It can be used like this:

summary = LayerSummary(model, mode=...)
print(summary)

As you say it could also support nn.Module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

by standalone i mean no dependence on the lightning module. right now the precision and example inputs are looked up via attributes of model inside of model summary, but these could be passed in explicitly to the model summary constructor. it's not a huge deal, but something small for people to see how lightning builds reusable components

@carmocca
Copy link
Contributor

Is this ready to review? If yes: please fill out the checklist in the original post, otherwise mark it as [WIP]/convert to draft

@ananthsub
Copy link
Contributor Author

Is this ready to review? If yes: please fill out the checklist in the original post, otherwise mark it as [WIP]/convert to draft

yes this is ready for review

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.

LGTM !

@ananthsub ananthsub changed the title Add typehints for tranining loop on_trainer_init + cleanup Remove ModelSummary validation from train loop on_trainer_init Mar 24, 2021
@carmocca carmocca merged commit ab4c838 into Lightning-AI:master Mar 24, 2021
@carmocca carmocca added feature Is an improvement or enhancement refactor labels Mar 29, 2021
@carmocca carmocca added this to the 1.3 milestone Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Is an improvement or enhancement refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants