clab / clab/dynet

Maybe TextFileLoader should not seek

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

Description

The current TextFileSaver/Loader file format saves a each set of parameters as a line in plain text. It also stores the number of bytes in the line so that it can seek forward to skip the line if it doesn't want to load that particular parameter.

In my opinion, it's dangerous that the TextFileLoader uses byte counts and seeking to skip lines rather than just reading the file line by line. There are two reasons for my concern:
- Because it looks like a text file, some editors or tools in Windows may modify the newline character, which changes the number of bytes dynet would need to seek. For example, git on Windows will, by default, replace LF endings with CR-LF endings. If I save a model file, check it in to git, and then check it back out, it will cause dynet to hang.
- Having a file format that looks entirely text invites people to edit the text. I would not be surprised if someone modifies the file to, for instance, replace a bunch of parameters with zero or random numbers. The model file will then fail to load because their edited line won't contain the same number of bytes as before editing.

The first issue could be alleviated by automatically checking, after seeking, if you have landed on a CR and if so seek forward another character, but the second issue can't really be fixed easily.

In contrast, I think there is little benefit to seeking rather than reading one line at a time. Seeking is only faster if a line is >1MB in size (seek time is about 10ms, reading is about 100MB/sec). @xunzhang: You changed the reader to seek instead of calling getline (https://github.com/clab/dynet/commit/e8b69e0e9739c6a92d0c830036f329eb1f13c374). I was wondering if this was in response to your own experience, and did seek make it faster? Are we aware of model saving/loading with parameters significantly >1MB in size that benefit from the seek? I'm wondering if we should just change it back to getline. Other options are the make the file more obviously binary. For example, it would presumably take less space and CPU time to store the floats directly with 4 bytes rather than going through a read/write conversion to text. Or, add some binary gunk here and there so tools like git will automatically detect they are binary, and users will be afraid to edit it directly :)

Contributor guide

No contributing guide indexed for this repository

Research direction

Start by locating the TextFileLoader and TextFileSaver implementations and reviewing the cited commit e8b69e0e9739c6a92d0c830036f329eb1f13c374. Compare seeking with line-by-line loading, investigate newline and edited-line behavior, and determine whether the project has models with parameters large enough to benefit from seeking. Done requires an agreed file-format direction and corresponding validation.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
machine-learning
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.