asyml / texar-pytorch

Integrating the Best of TF into PyTorch, for Machine Learning, Natural Language Processing, and Text Generation. This is part of the CASL project: http://casl-project.ai/
https://asyml.io
Apache License 2.0
744 stars 118 forks source link

Fixing index error after close files and clear `_log_destination` #323

Closed hunterhector closed 3 years ago

hunterhector commented 3 years ago

This PR fixes https://github.com/asyml/texar-pytorch/issues/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[bot] commented 3 years ago

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%) :arrow_up:

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 commented 3 years ago

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 commented 3 years ago

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.