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.

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

Fix validation progress counter with check_val_every_n_epoch > 1 #5952

Merged
merged 4 commits into from
Apr 2, 2021
Merged

Fix validation progress counter with check_val_every_n_epoch > 1 #5952

merged 4 commits into from
Apr 2, 2021

Conversation

sailordiary
Copy link
Contributor

@sailordiary sailordiary commented Feb 13, 2021 •
edited
Loading

What does this PR do?

Fixes #5039

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 update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is free to review the PR once the tests have passed.
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
  • Check that target branch and milestone match!

Did you have fun?

Make sure you had fun coding 🙃

@codecov
Copy link

codecov bot commented Feb 13, 2021 •
edited
Loading

Codecov Report

Merging #5952 (e609f21) into master (680e83a) will decrease coverage by 5%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #5952    +/-   ##
=======================================
- Coverage      93%     88%    -5%     
=======================================
  Files         161     170     +9     
  Lines       11515   11807   +292     
=======================================
- Hits        10659   10371   -288     
- Misses        856    1436   +580

@carmocca
Copy link
Contributor

Hi. Thanks for your contribution!

Can you write a test so we avoid regressions?

@sailordiary
Copy link
Contributor Author

Hi. Thanks for your contribution!

Can you write a test so we avoid regressions?

Happy to! But I just stumbled upon #5413 and it seems that there was a consesus to deprecate the check_val_every_n_epoch argument soon, so I'm not sure if this patch would still be necessary...

@tchaton
Copy link
Contributor

tchaton commented Feb 15, 2021

Hey @sailordiary,

Any updates on this PR ? It seems there were a discussion on the other issue, but no PR were associated to it.

Best,
T.C

@stale
Copy link

stale bot commented Mar 2, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If you need further help see our docs: https://pytorch-lightning.readthedocs.io/en/latest/generated/CONTRIBUTING.html#pull-request or ask the assistance of a core contributor here or on Slack. Thank you for your contributions.

@stale stale bot added the won't fix This will not be worked on label Mar 2, 2021
@rohitgr7
Copy link
Contributor

rohitgr7 commented Mar 6, 2021

@sailordiary you can go ahead and complete this PR with a test for now. Since this is a bug and even if we are going to remove this in the future it will be deprecated first. So bugfix is required here. Mind add a test for it?

@stale stale bot removed the won't fix This will not be worked on label Mar 6, 2021
@tchaton
Copy link
Contributor

tchaton commented Mar 12, 2021

Hey @rohitgr7,

Mind adding a test to this PR, so we can merge it ?

Best,
T.C

rohitgr7
rohitgr7 reviewed Mar 12, 2021
View reviewed changes
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@rohitgr7 rohitgr7 added the bug Something isn't working label Mar 12, 2021
@rohitgr7 rohitgr7 added this to the 1.2.x milestone Mar 12, 2021
@carmocca carmocca added the ready PRs ready to be merged label Mar 14, 2021
@carmocca carmocca enabled auto-merge (squash) March 14, 2021 01:17
@carmocca carmocca merged commit 1bd5f36 into Lightning-AI:master Apr 2, 2021
@kaushikb11 kaushikb11 mentioned this pull request Apr 5, 2021
Closed
kaushikb11 pushed a commit to kaushikb11/pytorch-lightning that referenced this pull request Apr 6, 2021
@sailordiary @rohitgr7
@carmocca @kaushikb11
Fix validation progress counter with check_val_every_n_epoch > 1 (Lig…
5c9dbc3 
…htning-AI#5952)

Co-authored-by: rohitgr7 <rohitgr1998@gmail.com>
Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
lexierule pushed a commit that referenced this pull request Apr 7, 2021
@kaushikb11 @Borda
@tchaton @akihironitta @sailordiary @rohitgr7 @carmocca @elimohl @awaelchli @karthikprasad @sadiqj @mibaumgartner @BloodAxe @ethanwharris @THasthika
Weekly Patch Release v.1.2.7 [full merge, no squash] (#6850)
b2c7345 
* update readme by v1.2.x (#6728)

* [bugfix] Add support for omegaconf and tpu (#6741)

* fix_hydra

* update changelog

Co-authored-by: Your Name <you@example.com>

* [docs] Update Bolts link (#6743)

* Update Bolts link

* Update Bolts link

* formt

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

* Update logic for checking TPUs availability (#6767)

* Update logic for checking TPUs availability

* fix flake8

* add fix

* resolve bug (#6781)

* Fix validation progress counter with check_val_every_n_epoch > 1 (#5952)

Co-authored-by: rohitgr7 <rohitgr1998@gmail.com>
Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>

* Remove extinct parameters from lightning_module.rst (#6801)

Fixes  #6800

* Update TPU docs for installation (#6794)

* fix boolean check on iterable dataset when len not defined (#6828)

* fix iterable dataset len check

* update predict and validate

* add validate to test

* add changelog

* add predict

* Sanitize `None` params during pruning (#6836)

* sanitize none params during pruning

* amend

* Fix `unfreeze_and_add_param_group` expects `modules` rather than `module` (#6822)

* Enforce an epoch scheduler interval when using SWA (#6588)

Co-authored-by: Carlos Mocholi <carlossmocholi@gmail.com>

* Fix DPP + SyncBN (#6838)

* Fix DPP + SyncBN

Ensure that model is already on correct GPU before applying SyncBN conversion

* Fix order of SyncBN for ddp_spawn

* [Fix] TPU Training Type Plugin (#6816)

* Fix support for symlink save_dir in TensorBoardLogger (#6730)

* Add test for symlink support and initial fix

* Respond to comment and add docstring

* Update CHANGELOG.md

* Simplify

* Update pytorch_lightning/utilities/cloud_io.py

Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>

* Make `LightningLocalFileSystem` protected

Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>

* Fixed missing arguments in `lr_find` call (#6784)

There seem to be 3 arguments missing in the `lr_find` call in the tunining.py file.

* Update Changelog & version

* Fix TPU tests for checkpoint

Skip advanced profiler for torch > 1.8

Skip pytorch profiler for torch > 1.8

Fix save checkpoint logic for TPUs

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
Co-authored-by: thomas chaton <thomas@grid.ai>
Co-authored-by: Your Name <you@example.com>
Co-authored-by: Akihiro Nitta <nitta@akihironitta.com>
Co-authored-by: Yuan-Hang Zhang <sailordiary@users.noreply.github.com>
Co-authored-by: rohitgr7 <rohitgr1998@gmail.com>
Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
Co-authored-by: Elizaveta Logacheva <elimohl@gmail.com>
Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>
Co-authored-by: Karthik Prasad <prasadkr@uci.edu>
Co-authored-by: Sadiq Jaffer <sadiq@toao.com>
Co-authored-by: Michael Baumgartner <m.baumgartner@ymail.com>
Co-authored-by: Eugene Khvedchenya <ekhvedchenya@gmail.com>
Co-authored-by: Ethan Harris <ewah1g13@soton.ac.uk>
Co-authored-by: Tharindu Hasthika <tharindubathigama@gmail.com>
lexierule pushed a commit that referenced this pull request Apr 7, 2021
@sailordiary @rohitgr7
@carmocca @lexierule
Fix validation progress counter with check_val_every_n_epoch > 1 (#5952)
feae0a0 
Co-authored-by: rohitgr7 <rohitgr1998@gmail.com>
Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carmocca carmocca carmocca approved these changes

@rohitgr7 rohitgr7 rohitgr7 approved these changes

@akihironitta akihironitta akihironitta approved these changes

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

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

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

@awaelchli awaelchli Awaiting requested review from awaelchli

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

@SeanNaren SeanNaren Awaiting requested review from SeanNaren

Assignees
No one assigned
Labels
bug Something isn't working ready PRs ready to be merged
Projects
None yet
Milestone
pl:1.2.x
Development

Successfully merging this pull request may close these issues.

validating bar keep showing 0 when check_val_every_n_epoch is bigger than 1
5 participants
@sailordiary @carmocca @tchaton @rohitgr7 @akihironitta