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

feature: Allow str arguments in Trainer.profiler #3656

Merged

Conversation

ddrevicky
Copy link
Contributor

@ddrevicky ddrevicky commented Sep 25, 2020

What does this PR do?

Fixes #3330

  • Allows str parameters for profiler arg in Trainer
  • Deprecates bool parameters for profiler arg in Trainer
  • Removes bool parameters for profiler from examples in docs
  • Adds tests checking correct profilers are returned for str arguments
# equivalent options
# CLI: --profiler true (deprecated) or --profiler simple
trainer = Trainer(profiler=True)        # deprecated
trainer = Trainer(profiler=SimpleProfiler())
trainer = Trainer(profiler="simple")

# equivalent options
# CLI: --profiler advanced
trainer = Trainer(profiler=AdvancedProfiler())
trainer = Trainer(profiler="advanced")

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 your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?
  • Did you verify new and existing tests pass locally with your changes?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

@pep8speaks
Copy link

pep8speaks commented Sep 25, 2020

Hello @ddrevicky! Thanks for updating this PR.

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

Comment last updated at 2020-10-27 10:08:35 UTC

@ddrevicky ddrevicky marked this pull request as draft September 25, 2020 12:21
@ddrevicky ddrevicky marked this pull request as ready for review September 25, 2020 13:22
@mergify mergify bot requested a review from a team September 25, 2020 13:22
@mergify mergify bot requested a review from a team September 25, 2020 13:27
CHANGELOG.md Outdated Show resolved Hide resolved
@mergify
Copy link
Contributor

mergify bot commented Sep 25, 2020

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

@ddrevicky ddrevicky changed the title Allow str arguments in Trainer.profiler [WIP] Allow str arguments in Trainer.profiler Oct 1, 2020
@ddrevicky ddrevicky force-pushed the feature/3330_trainer_profiler_str branch 3 times, most recently from 98c0d7f to 43ec93f Compare October 1, 2020 17:45
@ddrevicky ddrevicky changed the title [WIP] Allow str arguments in Trainer.profiler Allow str arguments in Trainer.profiler Oct 1, 2020
@ddrevicky
Copy link
Contributor Author

I've added deprecation of bool argument for the profiler like @justusschock requested. I'm not sure I've done all the steps for deprecation correctly, especially:

  • version in which removal occurs
  • deprecation tests in test_deprecated.py
  • I removed examples of passing bool to profiler from docs, is that right?

Let me know if there's anything that needs correction. The failing checks seem to be unrelated to the PR.

@codecov
Copy link

codecov bot commented Oct 1, 2020

Codecov Report

Merging #3656 into master will increase coverage by 1%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #3656    +/-   ##
=======================================
+ Coverage      91%     93%    +1%     
=======================================
  Files         111     111            
  Lines        8272    8096   -176     
=======================================
- Hits         7550    7506    -44     
+ Misses        722     590   -132     

@ydcjeff
Copy link
Contributor

ydcjeff commented Oct 1, 2020

It shall be better to add warning of bool here too

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.

Just some suggestions. Btw LGTM. Nice work!

pytorch_lightning/trainer/connectors/profiler_connector.py Outdated Show resolved Hide resolved
tests/test_deprecated.py Outdated Show resolved Hide resolved
tests/trainer/test_trainer.py Outdated Show resolved Hide resolved
tests/trainer/test_trainer.py Outdated Show resolved Hide resolved
pytorch_lightning/trainer/connectors/profiler_connector.py Outdated Show resolved Hide resolved
@mergify mergify bot requested a review from a team October 1, 2020 19:14
@ddrevicky ddrevicky force-pushed the feature/3330_trainer_profiler_str branch from dfdd14e to 53d8cd6 Compare October 2, 2020 07:14
@mergify mergify bot requested a review from a team October 2, 2020 07:38
@ddrevicky ddrevicky force-pushed the feature/3330_trainer_profiler_str branch from 61d66dd to d9dd6df Compare October 2, 2020 08:24
@rohitgr7 rohitgr7 added ready PRs ready to be merged feature Is an improvement or enhancement labels Oct 2, 2020
@rohitgr7 rohitgr7 requested review from Borda and awaelchli October 2, 2020 11:59
ddrevicky and others added 26 commits October 27, 2020 11:02
Co-authored-by: Carlos Mocholí <[email protected]>
Co-authored-by: Adrian Wälchli <[email protected]>
@ddrevicky ddrevicky force-pushed the feature/3330_trainer_profiler_str branch from 8b5b5ef to f911f37 Compare October 27, 2020 10:04
@rohitgr7 rohitgr7 merged commit c50c225 into Lightning-AI:master Oct 27, 2020
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 ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trainer.profiler should allow str