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

Refactor model summary + generalize example input array #1773

Merged
merged 19 commits into from
Jun 15, 2020

Conversation

awaelchli
Copy link
Contributor

@awaelchli awaelchli commented May 11, 2020

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

Fixes #1556
currently blocked by #1756

Before: Model summary with "example_input_array" would break if order defined layers is not consistent with order of execution in forward, e.g.,

self.layer1 = nn.Linear(5, 10)
self.layer2 = nn.Linear(10, 20)

if layers are called in the order self.layer2(self.layer1(input))
but if we switched the order to

self.layer2 = nn.Linear(10, 20)
self.layer1 = nn.Linear(5, 10)

it would break.

Now: We solve this issue by installing a PyTorch hook on each layer. The hook will collect the input and output shape in the order of execution.

  • Supports example_input_array in order of execution
  • Adds many tests
  • Better code quality, improved readability
  • Better documentation

Open questions:

  • What should we display for layers that are unused in the forward pass? Currently I print "?" for the input-/output shapes (see tests).

@mergify mergify bot requested a review from a team May 11, 2020 00:22
@awaelchli awaelchli changed the title Refactor model summary + generalize example input array [WIP] Refactor model summary + generalize example input array May 11, 2020
@awaelchli awaelchli removed the request for review from a team May 11, 2020 00:31
@mergify mergify bot requested a review from a team May 11, 2020 00:31
@awaelchli awaelchli removed the request for review from a team May 11, 2020 00:31
@mergify mergify bot requested a review from a team May 11, 2020 00:31
@Borda Borda added the feature Is an improvement or enhancement label May 14, 2020
@mergify mergify bot requested a review from a team May 14, 2020 11:55
@awaelchli awaelchli force-pushed the bugfix/example-input-array branch 2 times, most recently from ab1f83c to 405fcf1 Compare May 20, 2020 00:29
@codecov
Copy link

codecov bot commented May 20, 2020

Codecov Report

Merging #1773 into master will increase coverage by 1%.
The diff coverage is 95%.

@@          Coverage Diff           @@
##           master   #1773   +/-   ##
======================================
+ Coverage      87%     88%   +1%     
======================================
  Files          68      68           
  Lines        5221    5235   +14     
======================================
+ Hits         4544    4590   +46     
+ Misses        677     645   -32     

@Borda Borda added this to the 0.7.7 milestone May 24, 2020
@Borda
Copy link
Member

Borda commented May 24, 2020

Let's make this example input area as property..

@awaelchli
Copy link
Contributor Author

What would be the benefit of that? Could you clarify

@Borda
Copy link
Member

Borda commented May 24, 2020

What would be the benefit of that? Could you clarify

it would be read-only and also it MAY resolve soma attribute issue...

@Borda Borda modified the milestones: 0.7.7, 0.8.0 May 26, 2020
@awaelchli awaelchli force-pushed the bugfix/example-input-array branch from 405fcf1 to fbf3eff Compare May 26, 2020 22:40
@awaelchli awaelchli added the bug Something isn't working label May 28, 2020
@awaelchli awaelchli force-pushed the bugfix/example-input-array branch from 632a456 to cf303ba Compare June 6, 2020 18:38
@awaelchli
Copy link
Contributor Author

@Borda getting this failed import "assert_speed_parity". Which one should I use, absolute or relative speed parity?

@awaelchli
Copy link
Contributor Author

awaelchli commented Jun 6, 2020

strange that master branch is passing the tests

awaelchli added 4 commits June 7, 2020 01:25
variant a


variant b


add test


revert rename


add changelog


docs


move changelog entry to top


use hooks


wip


wipp


layer summary


clean up, refactor


type hints


rename


remove obsolete code


rename


unused imports


simplify formatting of table and increase readability


doctest


superclass object


update examples


print unknown sizes


more docs and doctest


testing


unknown layers


add rnn test


remove main


restore train mode


test device wip


device


constant


simplify model forward transfer


return summary object in method


extend tests


fix summary for empty module


extend tests


refactor and added hook


variant a


variant b


add test


revert rename


add changelog


docs


move changelog entry to top


remove hardcoded string


simplify


test unknown shapes and all others


comments for tests


fix hparams attribute
@mergify mergify bot requested a review from a team June 7, 2020 09:05
@mergify
Copy link
Contributor

mergify bot commented Jun 8, 2020

This pull request is now in conflict... :(

tests/core/test_memory.py Outdated Show resolved Hide resolved
tests/core/test_memory.py Outdated Show resolved Hide resolved
pytorch_lightning/core/memory.py Show resolved Hide resolved
pytorch_lightning/core/memory.py Show resolved Hide resolved
pytorch_lightning/core/memory.py Show resolved Hide resolved
shape = [parse_batch_shape(el) for el in batch]
return shape

return UNKNOWN_SIZE


def _format_summary_table(*cols) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

we already have a table printer for Profiler, so lets general that function and use it here too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is no utility for table printing in Profiler that can be extracted.
the shape, columns and rows of the table are all hardcoded and differ between Simple and AdvancedProfiler.

not sure how to extract this, seems this is a larger effort that does not fit well into this pr here.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes that's exactly what I was looking at.

@mergify mergify bot requested a review from a team June 8, 2020 12:55
@Borda Borda modified the milestones: 0.8.0, 0.9.0 Jun 9, 2020
@Borda Borda added the waiting on author Waiting on user action, correction, or update label Jun 9, 2020
@mergify
Copy link
Contributor

mergify bot commented Jun 12, 2020

This pull request is now in conflict... :(

@mergify
Copy link
Contributor

mergify bot commented Jun 13, 2020

This pull request is now in conflict... :(

@awaelchli awaelchli removed the waiting on author Waiting on user action, correction, or update label Jun 13, 2020
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

LGTM 🤖

@mergify mergify bot requested a review from a team June 15, 2020 12:19
Copy link
Member

@SkafteNicki SkafteNicki left a comment

Choose a reason for hiding this comment

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

LGTM

@mergify mergify bot requested a review from a team June 15, 2020 12:33
@williamFalcon williamFalcon merged commit 7dc58bd into master Jun 15, 2020
@Borda Borda deleted the bugfix/example-input-array branch June 15, 2020 21:11
@Borda Borda modified the milestones: 0.9.0, 0.8.0 Jun 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working feature Is an improvement or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"example_input_array" depends on ordering of modules
5 participants