Repository navigation
Raise a failed download thread's error from iteration and reject a cache_limit that local-only shards would fill - #49
Merged
Conversation
…ort epoch or a hang, and reject a cache_limit that local-only shards would fill at construction.
Documentation build overview
|
…being waited for, and let that wait stop at exit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When one of the two threads behind
__iter__(download, ready) raises, the error never reaches the caller.on_exceptionre-raised inside a futures done-callback, where only theconcurrent.futureslogger sees it, and the thread skipped itsit.on_exit(). Depending on timing, the epoch ended early with no error, or_Iterator.exit()waited forever for a thread that was already dead.#27 made
_evict_coldest_shardraise instead of deleting shards of a stream with no remote, which turned a silent data loss in 0.13.0 into this: acache_limittoo tight for a mix of local-only and remote streams hung in 6 of 10 runs and returned an empty epoch in the other 4.Fix
_run_threadwraps the download and ready threads: on any exception it records the error, sets the event, callson_exit()for the dead thread and re-raises._raise_if_background_failedraises aRuntimeErrorchained to that error inget_item, at the start of__iter__and after the yield loop, so a failed thread ends the epoch with its exception.on_exceptionis removed.cache_limitthat cannot hold the index files, the local-only shards and four times the largest shard, and the message gives the numbers. The mixed case fails there now.Tests
test_background_thread_failure_raises: a download thread that raises ends iteration with a chainedRuntimeError, and the next epoch reports it too.test_cache_limit_leaves_room_above_local_only_streams: one byte under the bound fails at construction; at the bound the epoch completes and the local-only shards stay.test_streaming,test_evictionandtest_barrierpass on Linux 3.12 with Fix forked DataLoader workers hanging after the parent iterated the dataset #48 included (228 tests).Found by the differential test of 0.14.1 against 0.13.0.