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

Fix: Don't skip saving the model if the save path already exists. #2264

Merged
merged 5 commits into from
Jul 13, 2022

Conversation

justinxzhao
Copy link
Contributor

@justinxzhao justinxzhao commented Jul 12, 2022

Without this change, the model is only saved the first time save() is called (at the end of the first round of evaluation).

This is a bug that was introduced in #2027. This was not caught by any model loading unit tests like tests/integration_tests/test_model_save_and_load.py because the best saved weights are reloaded automatically at the end of train(), so there's never a discrepancy between the model returned from train() and models loaded from disk.

@jimthompson5802 will send a follow up PR that adds a test that catches this regression specifically.

Closes #2259

Copy link
Collaborator

@jimthompson5802 jimthompson5802 left a comment

Choose a reason for hiding this comment

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

I confirmed that this fix addresses the issue I found.

@github-actions
Copy link

github-actions bot commented Jul 12, 2022

Unit Test Results

       6 files  +    1         6 suites  +1   2h 31m 2s ⏱️ + 30m 36s
2 930 tests +    1  2 884 ✔️ +    1    46 💤 ±  0  0 ±0 
8 790 runs  +174  8 648 ✔️ +151  142 💤 +23  0 ±0 

Results for commit c38aad9. ± Comparison against base commit 9c58d5e.

♻️ This comment has been updated with latest results.

@jimthompson5802
Copy link
Collaborator

@justinxzhao I added the test we talked about into tests/integration_tests/test_model_save_and_load.py::test_model_weights_match_training

I confirmed that this test will pick up the situation when the wrong weights are loaded from checkpoint.

Assuming we get a clean run in the CI test, this PR is good to go.

@justinxzhao justinxzhao merged commit 6ee335f into master Jul 13, 2022
@justinxzhao justinxzhao deleted the fix_weight_saving branch July 13, 2022 01:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect predictions are produced
4 participants