Multithreading archive extraction#186
Conversation
Reviewer's GuideIntroduces a num_workers option to enable parallel ZIP/TAR extraction by chunking archive members and dispatching tasks, with refactored extract functions and accompanying tests. Sequence diagram for parallel extraction of archive chunkssequenceDiagram
participant Caller
participant extract_archive
participant run_tasks
participant extract_chunk
Caller->>extract_archive: call with num_workers > 1
extract_archive->>run_tasks: dispatch extract_chunk tasks for each chunk
run_tasks->>extract_chunk: extract chunk (parallel)
extract_chunk-->>run_tasks: return extracted files
run_tasks-->>extract_archive: return all files
extract_archive-->>Caller: return file list
Class diagram for updated archive extraction functionsclassDiagram
class extract_archive {
+extract_archive(archive, destination, keep_archive, verbose, num_workers)
+extract_zip(archive, members=None)
+extract_tar(archive, members=None)
+extract_chunk(archive, members, destination, extension)
}
class extract_archives {
+extract_archives(archives, destination, keep_archive, verbose, num_workers)
}
extract_archives --> extract_archive : uses
extract_archive o-- extract_zip : calls
extract_archive o-- extract_tar : calls
extract_archive o-- extract_chunk : calls
extract_archive o-- run_tasks : uses
extract_archive o-- flatten_list : uses
Flow diagram for multithreaded archive extraction processflowchart TD
A["extract_archive() called with num_workers > 1"] --> B["Determine archive type (ZIP/TAR)"]
B --> C["List archive members"]
C --> D["Split members into chunks"]
D --> E["Dispatch extract_chunk tasks via run_tasks"]
E --> F["Flatten results and return file list"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Add validation to ensure num_workers is at least 1 and raise a clear error for invalid or zero/negative values.
- Remove the duplicated keep_archive entry in the docstrings and streamline the description for the new num_workers parameter.
- Parallel extraction splits members into contiguous chunks which can change output order and imbalance work; consider preserving original ordering in the flattened result or using round-robin assignment for better load distribution.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Add validation to ensure num_workers is at least 1 and raise a clear error for invalid or zero/negative values.
- Remove the duplicated keep_archive entry in the docstrings and streamline the description for the new num_workers parameter.
- Parallel extraction splits members into contiguous chunks which can change output order and imbalance work; consider preserving original ordering in the flattened result or using round-robin assignment for better load distribution.
## Individual Comments
### Comment 1
<location> `audeer/core/io.py:376-377` </location>
<code_context>
disable = not verbose
- def extract_zip(archive: str) -> list:
+ def extract_zip(
+ archive: str,
+ members: Sequence[str] | None = None,
+ ) -> list:
</code_context>
<issue_to_address>
**suggestion:** Type annotation for members parameter could be more precise.
Since `members` can be a sequence of either member names or ZipInfo objects, update the annotation to reflect this for better clarity and type safety.
Suggested implementation:
```python
def extract_zip(
archive: str,
members: Sequence[str | zipfile.ZipInfo] | None = None,
) -> list:
```
If `zipfile` is not already imported at the top of the file, add:
```python
import zipfile
```
</issue_to_address>
### Comment 2
<location> `audeer/core/io.py:389-377` </location>
<code_context>
return [m.filename for m in members]
- def extract_tar(archive: str) -> list:
+ def extract_tar(
+ archive: str,
+ members: Sequence[str] | None = None,
+ ) -> list:
</code_context>
<issue_to_address>
**issue (bug_risk):** Potential exception risk with tf.getmember(m) if member does not exist.
If a member name is missing from the archive, tf.getmember(m) will raise a KeyError. Please handle this exception or clarify the expected behavior.
</issue_to_address>
### Comment 3
<location> `tests/test_io_multithread.py:6-10` </location>
<code_context>
+import audeer
+
+
+def test_extract_archive_multithread(tmpdir):
+ root = audeer.mkdir(tmpdir, "root")
+ # Create a bunch of files
+ files = [f"file_{i}.txt" for i in range(10)]
+ for file in files:
+ audeer.touch(root, file)
+
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for num_workers=1 (sequential extraction).
Add a test for num_workers=1 to verify sequential extraction matches multithreaded behavior.
</issue_to_address>
### Comment 4
<location> `tests/test_io_multithread.py:31-10` </location>
<code_context>
+ assert os.path.exists(os.path.join(destination, file))
+
+
+def test_extract_tar_multithread(tmpdir):
+ root = audeer.mkdir(tmpdir, "root")
+ files = [f"file_{i}.txt" for i in range(10)]
+ for file in files:
+ audeer.touch(root, file)
+
</code_context>
<issue_to_address>
**suggestion (testing):** No test for extracting archives with zero files.
Add a test for extracting an empty archive with num_workers > 1 to ensure the function returns an empty list and handles the case correctly.
</issue_to_address>
### Comment 5
<location> `tests/test_io_multithread.py:10-11` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 6
<location> `tests/test_io_multithread.py:27-28` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 7
<location> `tests/test_io_multithread.py:34-35` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 8
<location> `tests/test_io_multithread.py:50-51` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 9
<location> `tests/test_io_multithread.py:57-58` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def test_extract_archive_multithread(tmpdir): | ||
| root = audeer.mkdir(tmpdir, "root") | ||
| # Create a bunch of files | ||
| files = [f"file_{i}.txt" for i in range(10)] | ||
| for file in files: |
There was a problem hiding this comment.
suggestion (testing): Missing test for num_workers=1 (sequential extraction).
Add a test for num_workers=1 to verify sequential extraction matches multithreaded behavior.
|
Adding multi-threading to archive extraction does not provide any speedups. Instead we should consider extracting archives in parallel (#187) or when downloading the archive from a backend use streaming ZIP extraction to extract the archive while downloading (audeering/audbackend#279). |
Alternative implementation to #184.
This adds the
num_workersargument toaudeer.extract_archive()andaudeer.extract_archives().Benchmark
I run a benchmark on a 1GB ZIP file containing 1000 files, the results are
We do not really get any speedup by using multithreading for extracting the archive.
Summary by Sourcery
Enable parallel extraction of ZIP and TAR archives by adding a num_workers parameter to extract_archive and extract_archives.
New Features:
Enhancements:
Tests: