[Parallel] Avoid race condition when downloading datasets

[eric-haibin-lin]

Description

Acquire a file lock before downloading the dataset. This helps when using multi-processing for multi-GPU training, or multi-machine training with a shared file system

Checklist

Essentials

• PR's title starts with a category (e.g. [BUGFIX], [MODEL], [TUTORIAL], [FEATURE], [DOC], etc)
• Changes are complete (i.e. I finished coding on this PR)
• All changes have test coverage
• Code is well-documented

Changes

• Feature1, tests, (and when applicable, API doc)
• Feature2, tests, (and when applicable, API doc)

Comments

• If this change is a backward incompatible change, why must this change be made.
• Interesting edge cases to note here

[codecov]

Codecov Report

Merging #1078 into master will increase coverage by 0.02%.
The diff coverage is 64%.

@@            Coverage Diff             @@
##           master    #1078      +/-   ##
==========================================
+ Coverage   88.25%   88.28%   +0.02%     
==========================================
  Files          67       67              
  Lines        6274     6280       +6     
==========================================
+ Hits         5537     5544       +7     
+ Misses        737      736       -1

Impacted Files	Coverage Δ
src/gluonnlp/data/utils.py	79.36% <100%> (+5.31%)	⬆️
src/gluonnlp/utils/files.py	45.9% <18.18%> (-3.12%)	⬇️
src/gluonnlp/model/sampled_block.py	90.74% <0%> (+0.08%)	⬆️

[eric-haibin-lin]

#726

[eric-haibin-lin]

Is there any recent change in numpy version on CI? The error does not seem to be relevant to the code changes.. @leezu

[szha]

Not sure if it's worth having a file lock for such use case. Nor am I sure about taking on a dependency for this. How does it compare to an approach where files are downloaded and extracted in a unique temporary folder, and then moved to the target location? The goal of such approach is that the download action would be idempotent so that race condition doesn't matter.

[leezu]

Yes. Numpy 1.18 is released and one of our dependencies is incompatible.

[leezu]

What kind of distributed filesystem are you using? It may be more efficient to cache the downloads on each machine locally. In my experience S3 is faster than for example EFS.
For multiprocessing, a workaround is to set the MXNET_HOME variable differently for each process?

I don't think MXNet upstream does locking / supports this? So introducing it only on the GluonNLP side may not be sufficient?

[eric-haibin-lin]

How do you signal other processes when the move is done so that others don't override the file, or don't read a corrupted file?

The goal is to have atomic file level operations so that multiple processes can read/write a complete file. Here are some candidates I previously think of:

• shutil.copyfile: it's implementation is not atomic
• os.rename(): atomic but does not support across file system. We do have lots of users using EFS/FSX.
• shutil.move: not atomic

Having a file locker is just a lot simpler.

concern addressed and the approach is reasonable

[leezu]

os.rename(): atomic but does not support across file system.

I'm not sure how this is related to our use-case. Files in the same directory will always be in the same filesystem. Here you are interested in downloading a file and making it atomically available once the download is complete. This operation does not involve cross-filesystem renames.

We do have lots of users using EFS/FSX.

It's a bad practice to keep the MXNET_HOME on EFS. EFS is slower than just using S3 or EBS. Also it is much more expensive. As a cheap and better performant alternative I suggest you look into https://github.com/kahing/goofys/

But in either way, this should be unrelated to atomic rename and locking.

Also how about the upstream support in MXNet (#1078 (comment))?

Finally, why not https://docs.python.org/3/library/os.html#os.lockf ?

[eric-haibin-lin]

@leezu os.lockf is only available for Unix. I assume we don't want to stop supporting windows users. The advantage is that portalocker is portable across different operating systems.

Yes, the support should be upstream'd to both gluon-nlp and mxnet

[leezu]

Yes, if file locking is supported on Windows and we want to use file-locking, we should also support Windows.

So how about we first use idempotent operations in GluonNLP until support is upstreamed? Then we can reuse the upstream implementation.

[sxjscience]

I think we can use the filelock. It's also used in sacrebleu: https://github.com/mjpost/sacreBLEU/blob/069b0c88fceb29f3e24c3c19ba25342a3e7f96cb/sacrebleu.py#L1154

[eric-haibin-lin]

So how about we first use idempotent operations in GluonNLP until support is upstreamed? Then we can reuse the upstream implementation.

What do you recommend as the way to implement idempotent operations and avoid race condition?

[leezu]

File-renames within the same directory are idempotent and atomic. You mentioned above that rename is not supported across filesystems, but we don't need to cross-filesystem rename. The temporary file with randomized name should be in the same directory as the "target" file.

[mli]

Job PR-1078/3 is complete.
Docs are uploaded to http://gluon-nlp-staging.s3-accelerate.dualstack.amazonaws.com/PR-1078/3/index.html

[mli]

Job PR-1078/4 is complete.
Docs are uploaded to http://gluon-nlp-staging.s3-accelerate.dualstack.amazonaws.com/PR-1078/4/index.html

[mli]

Job PR-1078/5 is complete.
Docs are uploaded to http://gluon-nlp-staging.s3-accelerate.dualstack.amazonaws.com/PR-1078/5/index.html

[eric-haibin-lin]

@leezu I removed portalocker from the dependency. Could you check my current implementation?