Profiler summary

[Borda]

What does this PR do?

Reaction to #1253. in particular:

• moves profilers do debugging package
• created abstraction over Profiler writing streams and add summary methods returning a string

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

[pep8speaks]

Hello @Borda! Thanks for updating this PR.

• In the file pytorch_lightning/profiler/__init__.py:

Line 116:111: E501 line too long (116 > 110 characters)

Comment last updated at 2020-03-31 11:39:34 UTC

[Borda]

@jeremyjordan mind have a look at it before finalizing?

[jeremyjordan]

i don't understand the package rename. we went from logging/ to loggers/ but now we're changing profiler/ to profiling/? it seems like an unnecessary change.

i like the abstraction over writing the summary more flexibly, however if you're going to rename describe to summary, then you'll want to update the training loop teardown method to call summary instead.

[Borda]

i don't understand the package rename. we went from logging/ to loggers/ but now we're changing profiler/ to profiling/? it seems like an unnecessary change.

The case with with logging was that there was collision with native python logging package and it was not clear which was imposed as root __init__ imported both
In parallel to loggers here we also have multiple profilers so plural make more sense, right?

i like the abstraction over writing the summary more flexibly, however if you're going to rename describe to summary, then you'll want to update the training loop teardown method to call summary instead.

It is not renaming at all as it may appear from diff, just adding extra function to generate the string text and so there is no no change for user at all (except before you write it file or console, no always write console and optional file)

[jeremyjordan]

It is not renaming at all as it may appear from diff, just adding extra function to generate the string text

ahh i see the describe method is just in the base class now and it calls the abstract method summary. makes sense, i like this refactor.

however, i'd say that we should keep the module where it's currently located in pytorch_lightning/profiler/. changing the location doesn't seem to give us much benefit but then we're left with adding more deprecation warnings and asking users to change their code. as a user it might be frustrating to have to change your import from pytorch_lightning.profiler to pytorch_lightning.profilers

[williamFalcon]

I agree with @jeremyjordan we need to stop making unnecessary changes to package names @Borda

[mergify]

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

[codecov]

Codecov Report

Merging #1259 into master will decrease coverage by 0%.
The diff coverage is 92%.

@@          Coverage Diff           @@
##           master   #1259   +/-   ##
======================================
- Coverage      92%     92%   -0%     
======================================
  Files          62      62           
  Lines        3181    3210   +29     
======================================
+ Hits         2924    2949   +25     
- Misses        257     261    +4     

[mergify]

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

[mergify]

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

[jeremyjordan]

@Borda mind submitting a follow-up PR which makes log.info write stream optional for the AdvancedProfiler?

[Borda]

I changed it as it was so it is file or info... Or you want to remove info for any case?

[jeremyjordan]

ah i missed that last round of commits, thanks! this looks great.