Skip to content
This repository has been archived by the owner on Dec 16, 2022. It is now read-only.

Fix ShouldValidateCallback #5536

Merged
merged 5 commits into from
Jan 12, 2022

Conversation

JohnGiorgi
Copy link
Contributor

@JohnGiorgi JohnGiorgi commented Jan 4, 2022

Addresses a bug in #5534.

Changes proposed in this pull request:

There were two subtle bugs in ShouldValidateCallback merged in #5534. One caused validation to always happen after the first epoch regardless of validation_start. The other caused validation to happen every validation_interval + 1 epochs instead of every validation_interval epochs. Fixed both with the following changes:

  • Add on_start method to ShouldValidateCallback that correctly sets _should_validate_this_epoch when training begins.
  • Because on_epoch is called at the end of every epoch, it needs to consider epoch + 1 (instead of epoch) when setting _should_validate_this_epoch.
  • Update TrainerTest.test_should_validate_callback so that it fails without these fixes.

Before submitting

  • I've read and followed all steps in the Making a pull request
    section of the CONTRIBUTING docs.
  • I've updated or added any relevant docstrings following the syntax described in the
    Writing docstrings section of the CONTRIBUTING docs.
  • If this PR fixes a bug, I've added a test that will fail without my fix.
  • If this PR adds a new feature, I've added tests that sufficiently cover my new functionality.

After submitting

  • All GitHub Actions jobs for my pull request have passed.
  • codecov/patch reports high test coverage (at least 90%).
    You can find this under the "Actions" tab of the pull request once the other checks have finished.

@dirkgr dirkgr enabled auto-merge (squash) January 12, 2022 16:41
@dirkgr
Copy link
Member

dirkgr commented Jan 12, 2022

Nice catch, and special thanks for adjusting the tests to catch this in the future!

@dirkgr dirkgr merged commit a711703 into allenai:main Jan 12, 2022
@JohnGiorgi JohnGiorgi deleted the fix-should-validate-callback branch January 12, 2022 20:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants