clab / clab/dynet

TextFileLoader can and does fail silently on errors

Open
#1,558 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
3.4k
Forks
701
PR merge metrics
No merged PRs in 30d

Description

In TextFileLoader::populate, the calls to stream::operator>> are not checked for the side-effect that is available through stream::fail() to ensure that the operation has completed successfully. This is problematic for lines like `iss >> values;` which despite the instruction to `values.resize(dim.size());` can and do fail, at least for Windows 10 and Visual Studio 2015. This results in not all values being transferred from the string to the vector of values and consequenctly not transferred to the Parameter or LayoutParameter without an exception or other warning. iss.fail(), however, does indicate an observable error. It can probably also happen in other situations, for example if line cannot be resized in `std::getline(datastream, line);`.

This can catch people off guard, because the problem, probably a low memory issue, is unexpected. The code uses up to three times the amount of memory that should be required to read in the data: once for a string containing contents of the entire line in the file, once again for a vector to hold all parsed values, and then a third for use in the parameter or lookup parameter. At least one can probably be eliminated. We are reading in a 10GB lookup parameter on a computer with 32GB of memory and it fails silently during the transfer from the string to the vector. The vector appears to be large enough, so perhaps there is some allocation failing during conversion from the string to float. In any case, the result is that the model gets corrupted. The error checking needs to be tightened up here if not the efficiency as well.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start in TextFileLoader::populate and inspect the stream::operator>> calls and std::getline(datastream, line) mentioned in the report, including their fail() behavior. Reproduce the large lookup-parameter case if possible; done means errors during loading are no longer silent and corrupted parameter data is reported or prevented.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
data
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.