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

Fixing index error after close files and clear _log_destination #323

Merged
merged 4 commits into from
Sep 17, 2020

Conversation

hunterhector
Copy link
Member

This PR fixes #322

The fix

At the time of _open_files, we check whether the length of the destination is zero, and fill in the List if needed.

The test

In the executor tests, we called test after train in all the tests. In this way, we ensure the _files_opened to have both True and False value, to allow us to test both branches in the _open_files function.

@codecov
Copy link

codecov bot commented Sep 15, 2020

Codecov Report

Merging #323 into master will increase coverage by 0.06%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #323      +/-   ##
==========================================
+ Coverage   80.08%   80.14%   +0.06%     
==========================================
  Files         134      134              
  Lines       11195    11195              
==========================================
+ Hits         8965     8972       +7     
+ Misses       2230     2223       -7     
Impacted Files Coverage Δ
texar/torch/run/executor.py 80.41% <75.00%> (+1.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb62781...71de572. Read the comment docs.

@huzecong
Copy link
Collaborator

For some reason adding executor.test() doesn't raise exceptions when I test locally without applying the fix... Strange.

This fix definitely works, however I think the easier thing to do is to simply not set _log_destinations to []. Since we maintain _files_opened, we don't need to check whether _log_destinations has length 0.

@hunterhector
Copy link
Member Author

For some reason adding executor.test() doesn't raise exceptions when I test locally without applying the fix... Strange.

You are right indeed, to make the bug appear we need to add a file log in the log_destination parameter. I have added that in the newer commit.

This fix definitely works, however I think the easier thing to do is to simply not set _log_destinations to []. Since we maintain _files_opened, we don't need to check whether _log_destinations has length 0.

Added in the newer commit, see if that works.

@hunterhector hunterhector merged commit a9e79f8 into asyml:master Sep 17, 2020
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.

A logging bug in executor after close_files
2 participants