Skip to content

Rename default ProgressBar to TQDMProgressBar #9229

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Rename default ProgressBar to TQDMProgressBar #9229

wants to merge 3 commits into from

Conversation

@kaushikb11
Copy link
Contributor

@kaushikb11 kaushikb11 commented Aug 31, 2021 •
edited
Loading

What does this PR do?

Fixes #9302

Does your PR introduce any breaking changes? If yes, please list them.

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 list all the breaking changes introduced by this pull request?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is welcome to review the PR.
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 🙃

@kaushikb11 kaushikb11 marked this pull request as ready for review September 6, 2021 04:13
@codecov
Copy link

codecov bot commented Sep 6, 2021 •
edited
Loading

Codecov Report

Merging #9229 (40a358e) into master (f6614b3) will increase coverage by 0%.
The diff coverage is 100%.

❗ Current head 40a358e differs from pull request most recent head a199934. Consider uploading reports for the commit a199934 to get more accurate results

@@          Coverage Diff           @@
##           master   #9229   +/-   ##
======================================
  Coverage      92%     92%           
======================================
  Files         176     179    +3     
  Lines       14805   14863   +58     
======================================
+ Hits        13658   13714   +56     
- Misses       1147    1149    +2

@mergify mergify bot added the has conflicts label Sep 6, 2021
awaelchli
awaelchli reviewed Sep 6, 2021
View reviewed changes
Copy link
Contributor

@awaelchli awaelchli left a comment •
edited
Loading

Choose a reason for hiding this comment

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

can you also rename the test_progress_bar.py test file?
also note I added fixes in #9310

tchaton
tchaton approved these changes Sep 6, 2021
View reviewed changes
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 !

Borda
Borda requested changes Sep 6, 2021
View reviewed changes
pytorch_lightning/callbacks/progress/progress.py
if not bar.disable:
bar.reset(total=convert_inf(total))
# points to the default implementation
ProgressBar = TQDMProgressBar
Copy link
Collaborator

@Borda Borda Sep 6, 2021

Choose a reason for hiding this comment

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

rather inherit and add a deprecation warning, see what we did with Metrics

Copy link
Contributor Author

@kaushikb11 kaushikb11 Sep 6, 2021

Choose a reason for hiding this comment

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

@Borda reference link?

Copy link
Collaborator

@Borda Borda Sep 6, 2021

Choose a reason for hiding this comment

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

for example #6584

Copy link
Contributor

@tchaton tchaton Sep 7, 2021

Choose a reason for hiding this comment

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

Like this

class ProgressBar(TQDMProgressBar):
    pass

ethanwharris
ethanwharris reviewed Sep 6, 2021
View reviewed changes
@kaushikb11 @ethanwharris
Update CHANGELOG.md
a199934 
Co-authored-by: Ethan Harris <[email protected]>
@daniellepintz
Copy link
Contributor

daniellepintz commented Sep 7, 2021

please also update callbacks.rst

@daniellepintz
Copy link
Contributor

daniellepintz commented Sep 20, 2021

@kaushikb11 what is the status of this PR?

@stale stale bot added the won't fix This will not be worked on label Oct 5, 2021
@Lightning-AI Lightning-AI deleted a comment from stale bot Oct 5, 2021
@stale stale bot removed the won't fix This will not be worked on label Oct 5, 2021
@Borda
Copy link
Collaborator

Borda commented Oct 5, 2021

@kaushikb11 can we pls resolve the conflicts?

@daniellepintz daniellepintz mentioned this pull request Oct 17, 2021
Closed
@stale stale bot added the won't fix This will not be worked on label Oct 20, 2021
@carmocca carmocca added this to the v1.5 milestone Oct 21, 2021
@stale stale bot removed the won't fix This will not be worked on label Oct 21, 2021
@Lightning-AI Lightning-AI deleted a comment from stale bot Oct 25, 2021
@kaushikb11 kaushikb11 closed this Oct 25, 2021
@carmocca carmocca deleted the rename/progress_bar branch October 25, 2021 14:36
@daniellepintz
Copy link
Contributor

daniellepintz commented Oct 25, 2021

@kaushikb11 why did you decide to close this?

@kaushikb11
Copy link
Contributor Author

kaushikb11 commented Oct 26, 2021

@daniellepintz I am adding it in #10134.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Borda Borda Borda requested changes

@ethanwharris ethanwharris ethanwharris left review comments

@tchaton tchaton tchaton approved these changes

@justusschock justusschock Awaiting requested review from justusschock justusschock is a code owner

@SeanNaren SeanNaren Awaiting requested review from SeanNaren

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

+2 more reviewers

@awaelchli awaelchli awaelchli left review comments

@carmocca carmocca carmocca approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

has conflicts refactor

Projects

None yet

Milestone

pl:1.5

Development

Successfully merging this pull request may close these issues.

Rename ProgressBar callback to TQDMProgressBar

7 participants

@kaushikb11 @daniellepintz @Borda @awaelchli @ethanwharris @carmocca @tchaton