Add compression argument to publish() - #65
Conversation
audeer.create_archive() used to hardcode deflate at zlib's default level 6, which is single threaded at around 10 MB/s and thereby accounted for nearly the whole publication time of a model, e.g. 758 s of 776 s for a 7.4 GiB model. audmodel.publish() now exposes the compression level added in audeer 2.6.0, and defaults it to 1. On model weights level 1 compresses as well as level 6, 0.8 pp apart, at 2.4x the speed, and 0 stores the files and makes publication 44x faster than before at the price of a 24% larger archive. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reviewer's GuideAdds a validated Sequence diagram for publishing a model with configurable compressionsequenceDiagram
participant Caller
participant Publish as audmodel.publish()
participant Archive as put_archive()
participant Audeer as audeer.create_archive()
participant Backend
Caller->>Publish: publish(compression)
Publish->>Publish: validate compression 0..9
Publish->>Archive: put_archive(compression=compression)
Archive->>Audeer: create_archive(compression=compression)
Audeer-->>Archive: model.zip
Archive->>Backend: upload model.zip
Backend-->>Caller: published model
File-Level Changes
Possibly linked issues
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:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="audmodel/core/backend.py" line_range="393-394" />
<code_context>
repository: Repository,
alias: str | None = None,
author: str | None = None,
+ compression: int = 1,
date: datetime.date | None = None,
meta: dict[str, object] | None = None,
</code_context>
<issue_to_address>
**issue (bug_risk):** Adding `compression` before `tmp_root` changes the positional calling convention of `put_archive()`. Existing callers that pass `tmp_root` as the eighth positional argument now supply that path as `compression`, causing archive creation to fail instead of using the requested temporary directory.
**Triggers:** When downstream code calls the internal backend helper positionally with a `tmp_root` argument.
**Suggested fix:** Append `compression` after `tmp_root`, or make the new parameter keyword-only while preserving the existing positional parameters.
```suggestion
tmp_root: str | None = None,
compression: int = 1,
```
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and an incorrect compression setting is recorded in published archives, affecting their storage size and download/decompression behavior; reverting only affects future publications. Existing archives can be replaced or republished, so the impact is bounded and repairable.
Blocking findings: audmodel/core/backend.py:394
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
put_archive() is internal, and publish() is its only caller, so the argument does not need a default. This also keeps tmp_root the last argument, and the only one with a default. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new compression validation can raise TypeError for non-numeric inputs (instead of the intended ValueError), and the new test stores inconsistent params metadata when the compression argument is omitted.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR exposes audeer.create_archive()’s new compression option through audmodel.publish(), allowing callers to trade off publication speed vs. archive size and ensuring audeer>=2.6.0 is available at runtime.
Changes:
- Add
compressionkeyword argument toaudmodel.publish()(defaulting to level1) and validate it early. - Thread
compressionthrough the backend upload path intoaudeer.create_archive(). - Add tests covering compression behavior and invalid compression levels; update dependencies to require
audeer>=2.6.0.
File summaries
| File | Description |
|---|---|
audmodel/core/api.py |
Adds compression argument, docs, and pre-upload validation; forwards to backend archiving. |
audmodel/core/backend.py |
Extends put_archive() to accept/pass compression into audeer.create_archive(). |
tests/test_publish.py |
Adds coverage for archive compression mode and invalid compression levels. |
pyproject.toml |
Moves audeer into runtime deps and bumps minimum version to >=2.6.0. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| compression: compression level | ||
| of the model archive. | ||
| ``0`` stores the model files | ||
| without compression, | ||
| ``1``-``9`` selects a deflate level. | ||
| Deflate is single threaded, | ||
| and dominates the publication time | ||
| of large models. | ||
| Higher levels than ``1`` | ||
| hardly compress better | ||
| on model weights, | ||
| but take at least twice as long. | ||
| Select ``0``, | ||
| if you want to publish | ||
| as fast as possible |
There was a problem hiding this comment.
As higher levels as 1 do not make sense, we could even simplify to compression: bool = True.
Otherwise, we should maybe also support None as input to make it equal to audeer.create_archive(). Staying at 1 as default instead of None makes sense.
There was a problem hiding this comment.
In fact a user could already pass False -> 0 and True -> 1 if she wanted to do so. Yet, generally restricting to a boolean I find a bit too limitating. There might still be models (e.g. with large text files) where a higher compression rate can still make sense.
we should maybe also support None
If we allow None it should default to a reasonable value, but audeer maps it to library default, which is 6, and according to the benchmark this is bad choice.
audeer.create_archive()used to hardcode deflate at zlib's default level 6. Deflate is single threaded at around 10 MB/s on model weights, so it accounted for nearly the whole publication time of a model: 758 s of 776 s for a 7.4 GiB model.audeer2.6.0 added acompressionargument, and this PR exposes it inaudmodel.publish():0stores the model files without compression1-9selects a deflate levelThe level is validated in
publish()before anything is uploaded, as the cleanup handler around the upload would otherwise replace the error with"Could not publish model due to an unexpected error."audeer >=2.6.0is added todependencies.Up for discussion: the default
This PR defaults to
compression=1.Measurements on a 7.38 GiB model with 16 files, archive written to and read from local disk:
6(behaviour before)1(this PR)0The alternatives:
None, keep the previous behaviour.Would need the type widened to
int | None, withNoneforwarded toaudeer, which then picks zlib's level 6. Disadvantage: level 6 is a bad operating point on model weights: it compresses 0.8 pp better than level 1 and takes 2.4x as long.0, no compression by default.Compressing at level 1 costs 299 s to save 1.73 GiB, so it only pays off below ~5.9 MB/s (~50 Mbit/s) of upload bandwidth.
1, the middle way, as implemented.An archive is written once but downloaded many times. On the load side inflating costs 53 s to save the same 1.73 GiB, break even at ~34 MB/s (~270 Mbit/s) of download bandwidth, so for most consumers level 1 is at worst neutral. It also keeps the storage on the backend at today's size.