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

Simplify progress bar args #1108

Merged
merged 16 commits into from
Apr 2, 2020

Conversation

gerardrbentley
Copy link
Contributor

@gerardrbentley gerardrbentley commented Mar 10, 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 #1102 suggestion to collapse show_progress_bar into progress_bar_refresh_rate

Adds show_progress_bar to "to be deprecated" Trainer args (unsure on best approach here for community)

Removes show_progress_bar from tests that used it

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 🙃
Learned some Git 🙃

@pep8speaks
Copy link

pep8speaks commented Mar 10, 2020

Hello @gerardrbentley! Thanks for updating this PR.

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

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

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

Comment last updated at 2020-04-02 22:32:08 UTC

@Borda
Copy link
Member

Borda commented Mar 11, 2020

hey there, we have added GPU CI test, so could we kindly ask to rebase/merge master which will trigger these tests so we do not need to test it manually... Thx for your understanding 🤖

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.

Thx for taking care of this change, here are some todos to be completed:

  • update changelog
  • could we kindly ask to rebase/merge master

pytorch_lightning/trainer/__init__.py Outdated Show resolved Hide resolved
pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved
pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved
pytorch_lightning/trainer/trainer.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
tests/trainer/test_trainer.py Outdated Show resolved Hide resolved
@Borda Borda added the feature Is an improvement or enhancement label Mar 13, 2020
@Borda Borda added this to the 0.7.2 milestone Mar 13, 2020
@gerardrbentley
Copy link
Contributor Author

Adding your suggestions right now

@gerardrbentley
Copy link
Contributor Author

gerardrbentley commented Mar 14, 2020

Forget to add a commit for Changelog, @Borda should I add that on this PR or is there a different way the changelog is updated? (Sorry, new to contributing)

EDIT
Seems like other changelog updates aren't generally batched so I added it here

@Borda
Copy link
Member

Borda commented Mar 14, 2020

Forget to add a commit for Changelog, @Borda should I add that on this PR or is there a different way the changelog is updated? (Sorry, new to contributing)

It is kind of responsibility to edit the changelog along with the change in a particular PR
Btw, it is in the PR template - Before submitting ;)

@mergify
Copy link
Contributor

mergify bot commented Mar 24, 2020

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

@codecov
Copy link

codecov bot commented Mar 24, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@724b787). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             master   #1108   +/-   ##
========================================
  Coverage          ?     91%           
========================================
  Files             ?      61           
  Lines             ?    3153           
  Branches          ?       0           
========================================
  Hits              ?    2880           
  Misses            ?     273           
  Partials          ?       0

@Borda
Copy link
Member

Borda commented Mar 24, 2020

@gerardrbentley how is it going? could you please rebase on master so it is clear what are the changes...

@gerardrbentley
Copy link
Contributor Author

gerardrbentley commented Mar 25, 2020

@Borda Hopefully done. Rebased and I think I cut down all the erroneous things I tried today in my git log.

Apologies, definitely made some errors around the flake8 checking and autopep8 -v -r --max-line-length 120 --in-place . command.

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.

pls move the deprecated attributes to deprecated API

pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved
pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved
pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved
pytorch_lightning/trainer/training_loop.py Outdated Show resolved Hide resolved
@mergify
Copy link
Contributor

mergify bot commented Mar 25, 2020

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

@mergify
Copy link
Contributor

mergify bot commented Mar 27, 2020

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

@mergify mergify bot requested a review from a team March 30, 2020 22:33
@williamFalcon williamFalcon added the ready PRs ready to be merged label Apr 2, 2020
@Borda Borda requested a review from a team April 2, 2020 16:02
@mergify
Copy link
Contributor

mergify bot commented Apr 2, 2020

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

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
pytorch_lightning/trainer/__init__.py Outdated Show resolved Hide resolved
@mergify mergify bot requested a review from a team April 2, 2020 18:55
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
tests/models/test_amp.py Outdated Show resolved Hide resolved
@Borda
Copy link
Member

Borda commented Apr 2, 2020

some issues with GitHub, need to accept commits...

Copy link
Contributor Author

@gerardrbentley gerardrbentley left a comment

Choose a reason for hiding this comment

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

Trying to Commit Suggestions, but it's telling me the diff is outdated

Co-Authored-By: Jirka Borovec <[email protected]>
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 🚀

@Borda Borda requested a review from a team April 2, 2020 21:53
@gerardrbentley
Copy link
Contributor Author

I think I might have missed that /test_amp.py fix. Bulk commit suggestions didn't work and only one went through I believe

Copy link
Member

@ethanwharris ethanwharris 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
Copy link
Member

Borda commented Apr 2, 2020

Trying to Commit Suggestions, but it's telling me the diff is outdated

there was as incident recently... https://www.githubstatus.com/

@Borda Borda merged commit f33b5a8 into Lightning-AI:master Apr 2, 2020
alexeykarnachev pushed a commit to alexeykarnachev/pytorch-lightning that referenced this pull request Apr 4, 2020
* show progress bar dependent on refresh_rate

* test progress_bar_refresh control show bar

* remove show_progress_bar from other tests

* borda fixes

* flake8 fix

* changelog update prog bar refresh rate

* move show_progress_bar to deprecated 0.9 api

* rm show_progress_bar references, test deprecated

* Update pytorch_lightning/trainer/__init__.py

* fix test

* changelog

* minor CHANGELOG.md format

* Update pytorch_lightning/trainer/__init__.py

* Update pytorch_lightning/trainer/trainer.py

Co-authored-by: Gerard Bentley <[email protected]>
Co-authored-by: William Falcon <[email protected]>
Co-authored-by: Jirka Borovec <[email protected]>
Co-authored-by: J. Borovec <[email protected]>
@Borda Borda modified the milestones: v0.7., v0.7.x Apr 18, 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 ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enable progress bar = update freq 1
5 participants