Add num_workers argument to extract_archive()#184
Closed
hagenw wants to merge 5 commits into
Closed
Conversation
Contributor
Reviewer's GuideThis PR extends extract_archive() to support parallel extraction by introducing a num_workers parameter, refactoring extraction into per-member helper functions, and dispatching tasks via run_tasks, plus new tests to validate multi-threaded behavior. Sequence diagram for parallel extraction in extract_archive()sequenceDiagram
participant Caller
participant "extract_archive()"
participant "run_tasks()"
participant "extract_zip_member()"
participant "extract_tar_member()"
Caller->>"extract_archive()": Call with num_workers > 1
alt ZIP archive
"extract_archive()"->>"run_tasks()": Dispatch extract_zip_member for each file
"run_tasks()"->>"extract_zip_member()": Extract file (parallel)
else TAR archive
"extract_archive()"->>"run_tasks()": Dispatch extract_tar_member for each file
"run_tasks()"->>"extract_tar_member()": Extract file (parallel)
end
"extract_archive()"-->>Caller: Return list of extracted files
Class diagram for updated extract_archive() and helpersclassDiagram
class extract_archive {
+keep_archive: bool
+verbose: bool
+num_workers: int | None
+extract_zip(archive: str) list
+extract_tar(archive: str) list
}
class extract_zip_member {
+member
}
class extract_tar_member {
+member
}
class run_tasks {
+func
+params
+num_workers
+progress_bar
+task_description
}
extract_archive o-- extract_zip_member
extract_archive o-- extract_tar_member
extract_archive ..> run_tasks
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The
extract_zip_memberandextract_tar_memberfunctions reopen the archive for every file, which can be inefficient—consider opening the archive once and sharing it across worker threads or batching extractions. - The
num_workers=Nonepathway isn’t actually implemented—None is passed directly torun_tasks, so you should default it to CPU count * 5 (as documented) or validate/normalize the parameter before use. - Archive extraction should sanitize member paths to prevent directory traversal attacks (e.g., stripping leading ‘..’), especially when unpacking untrusted archives.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `extract_zip_member` and `extract_tar_member` functions reopen the archive for every file, which can be inefficient—consider opening the archive once and sharing it across worker threads or batching extractions.
- The `num_workers=None` pathway isn’t actually implemented—None is passed directly to `run_tasks`, so you should default it to CPU count * 5 (as documented) or validate/normalize the parameter before use.
- Archive extraction should sanitize member paths to prevent directory traversal attacks (e.g., stripping leading ‘..’), especially when unpacking untrusted archives.
## Individual Comments
### Comment 1
<location> `audeer/core/io.py:372-374` </location>
<code_context>
)
disable = not verbose
+ def extract_zip_member(member):
+ """Extract a single member from a ZIP archive."""
+ with zipfile.ZipFile(archive, "r") as zf:
+ zf.extract(member, destination)
+
</code_context>
<issue_to_address>
**issue (performance):** Repeated opening of the archive file for each member may impact performance.
Opening the archive file for each extraction can add overhead and reduce parallelization efficiency. Refactor to open the archive once and share the handle if possible, or clarify in documentation if this approach is necessary for thread safety.
</issue_to_address>
### Comment 2
<location> `audeer/core/io.py:370` </location>
<code_context>
+ # Sequential extraction with progress bar
for member in progress_bar(members, desc=desc, disable=disable):
- zf.extract(member, destination)
- return [m.filename for m in members]
+ extract_zip_member(member)
+ else:
</code_context>
<issue_to_address>
**issue:** Inconsistent return value for ZIP and TAR extraction.
Standardize the return value between extract_zip and extract_tar to avoid confusion for downstream consumers, particularly when handling directories or special files.
</issue_to_address>
### Comment 3
<location> `tests/test_io.py:1632-1641` </location>
<code_context>
+def test_extract_archive_multithread(tmpdir, archive_type):
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for num_workers=None and num_workers > number of files.
Please add tests for num_workers=None and for cases where num_workers is greater than the number of files to fully cover the argument's logic.
</issue_to_address>
### Comment 4
<location> `tests/test_io.py:1637-1641` </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 5
<location> `tests/test_io.py:1668-1680` </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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
🚀 New features to boost your workflow:
|
This reverts commit 1c02461.
Member
Author
|
Closing in favor of #186 |
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
1 participant
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.
This speeds up extracting of archives containing multiple files by adding a
num_workersargument.Summary by Sourcery
Enable configurable parallel extraction in extract_archive by introducing a num_workers parameter, refactoring member extraction, and adding tests to ensure multi-threaded behavior matches sequential results.
New Features:
Enhancements:
Documentation:
Tests: