Skip to content

Add support for reading and writing .wfdb archive files - #541

Open
tompollard wants to merge 16 commits into
mainfrom
tp/wfdb_file
Open

tompollard wants to merge 16 commits into
mainfrom
tp/wfdb_file

Conversation

@tompollard

@tompollard tompollard commented May 3, 2025 •

Copy link
Copy Markdown
Member

As discussed in wfdb/wfdb-spec#21 and #540 we would like to allow WFDB Records to be saved to a single file .wfdb.

This pull request introduces support for reading and writing .wfdb archive files: a ZIP file that holds the header, signal and annotation files of a single record.

Implementation

The pull request has three commits:

  1. Reading. rdrecord, rdsamp, rdheader and rdann read from an archive when given its path, including the .wfdb extension (for example wfdb.rdrecord("100.wfdb")).
    • The header is the file named after the archive (100.hea in 100.wfdb), or the only header in the archive. Signal and annotation files are read by the names that follow from the header, and a missing file is an error.
    • rdrecord reads the header and signals from a single open archive. Archives are opened and closed within each call, so nothing is cached between calls.
    • Headers larger than 1 MB are rejected, as a guard against damaged or malicious archives.
  2. Writing. wrsamp and Record.wrsamp take a wfdb_archive argument: a path, or an open WFDBArchive.
    • A new archive is written to a temporary file and moved into place only when writing succeeds. A failed write leaves no partial archive, and an existing archive at the same path is either fully replaced or left unchanged.
    • New archives get the permissions of the archive they replace, or the default permissions for new files.
    • create_archive(file_paths, archive_path) puts the files of an existing record into an archive.
  3. Docs. A section on archives in the I/O docs, and a changelog entry.

Format details:

  • One record per archive. WFDBArchive refuses to write a second header file, so it's always clear which record an archive holds. Annotation and other files can be added alongside the record.
  • Compression. Header files are compressed. Signal files are stored without compression, so that reading a range of samples doesn't require decompressing the whole file.

Not supported yet: multi-segment records, and archives in cloud storage or on PhysioNet. These raise a clear error. Multi-segment support would extend the one-record rule to "the main header plus the segment headers it lists", and is planned as a follow-up.

Example usage

import numpy as np
import wfdb
from wfdb.io.archive import WFDBArchive, create_archive

# Write a record straight into an archive
record = wfdb.rdrecord("sample-data/test01_00s", physical=False)
record.wrsamp(wfdb_archive="test01_00s.wfdb")

# Read it back, in full or in part
archive_record = wfdb.rdrecord("test01_00s.wfdb", physical=False)
np.testing.assert_array_equal(archive_record.d_signal, record.d_signal)
part = wfdb.rdrecord("test01_00s.wfdb", sampfrom=100, sampto=200, channels=[0, 2])
signals, fields = wfdb.rdsamp("test01_00s.wfdb")
header = wfdb.rdheader("test01_00s.wfdb")

# Or with the wrsamp function
wfdb.wrsamp(
    "ecg",
    fs=250,
    units=["mV", "mV"],
    sig_name=["I", "II"],
    p_signal=np.random.default_rng(0).standard_normal((1000, 2)),
    fmt=["16", "16"],
    wfdb_archive="ecg.wfdb",
)

# Put an existing record and its annotations into an archive
create_archive(
    ["sample-data/100.hea", "sample-data/100.dat", "sample-data/100.atr"],
    "100.wfdb",
)
annotation = wfdb.rdann("100.wfdb", "atr")

# Add an annotation file while writing a record
with WFDBArchive("100_copy.wfdb", mode="w") as archive:
    wfdb.rdrecord("sample-data/100", physical=False).wrsamp(wfdb_archive=archive)
    archive.add_file("sample-data/100.atr")

Note: Record.wrsamp writes the digital signal, so the record is read with physical=False above. Calling it on a record read with physical signals fails with an unclear TypeError. That's existing behaviour, not specific to archives, and could be improved separately.

Tests

tests/test_archive.py compares records read from archives with the same records read from separate files, including FLAC formats, multiple samples per frame and sample ranges. It also covers writing, annotations, the one-record rule, permissions, failed writes, and error cases such as missing files and unsupported multi-segment records.

@tompollard
tompollard force-pushed the tp/wfdb_file branch 6 times, most recently from f98cedc to 31fcfdb Compare May 29, 2025 21:13
@tompollard
tompollard force-pushed the tp/wfdb_file branch 2 times, most recently from 1901e3d to d72055c Compare October 27, 2026 20:25
@tompollard
tompollard requested a review from briangow October 27, 2026 20:26
@tompollard
tompollard requested a review from cbrnr October 27, 2026 20:53
@tompollard tompollard changed the title WIP: Add support for reading and writing .wfdb archive files Add support for reading and writing .wfdb archive files Oct 27, 2026
@cbrnr

cbrnr commented Oct 28, 2026

Copy link
Copy Markdown
Contributor

I asked Claude Opus 5.5 for a review. Overall, the design looks good, but there are some issues that should be addressed (grouped into three categories).

Points 1, 2, 3, and 5 must be fixed before merge. Points 4 and 6 should be fixed (they are very cheap but not blocking), and the remaining points 7, 8, 9, and 10 are optional (ideally for a follow-up PR).

Bugs

  1. Source files dated before 1980 break archive creation (archive.py:90). add_file goes through ZipFile.write, which by default raises ValueError for any mtime before 1980. That happens with files from Nix or reproducible builds (SOURCE_DATE_EPOCH=0), for example.
    Fix: zipfile.ZipFile(self._tmp_path, mode="w", strict_timestamps=False).

  2. Reading the umask is not thread-safe (archive.py:313). The os.umask(0) / os.umask(umask) pair briefly changes a process-wide setting. Files that another thread creates in that window come out world-writable.
    Fix: Don't read the umask at all. Create the temp file with os.open(tmp, os.O_CREAT | os.O_EXCL | os.O_WRONLY, 0o666) and a random suffix, instead of mkstemp (which uses 0o600). The kernel then applies the umask itself. Only chmod when replacing an existing archive, to keep its mode.

  3. The temp file is left behind if close() fails during abort (archive.py:299).
    Fix:

    try:
        if zip_file is not None:
            zip_file.close()
    finally:
        if tmp_path is not None:
            with contextlib.suppress(FileNotFoundError):
                os.remove(tmp_path)
  4. A closed archive fails with an unclear AttributeError on None (archive.py:373 and every accessor).
    Fix: Add a _require_open() that raises ValueError(f"Archive '{self.path}' is closed") when self._zipfile is None. Call it from _require_mode, namelist, exists, file_size and open.

  5. write_dir is silently ignored when wfdb_archive is a path (record.py:913). The archive ends up in the current directory.
    Fix: Pass write_dir to archive_for_writing and use os.path.join(write_dir, wfdb_archive) for relative paths. Alternatively, raise if both are given.

  6. The extension check is case-sensitive (archive.py:355, plus the same check in __init__). 100.WFDB is treated as a plain record name.
    Fix: Compare .lower().endswith(ARCHIVE_EXTENSION) in both places. record_name slices by length, so it keeps working.

Performance

  1. Partial reads are O(file size) on Python 3.11 (_signal.py:1695). Confirmed: 3.11's ZipExtFile.seek always reads up to the target offset, and 3.12 only skips that for stored entries. Archives made by other zip tools with a deflated .dat are slow on every version.
    Fix: For stored, unencrypted entries, work out the data offset from the local header (header_offset + 30 + name_len + extra_len, unpacked from bytes 26–30). Then read straight from the archive file with a normal open() and seek(offset + start_byte). Keep ZipExtFile as a fallback for deflated entries, and maybe warn there.

  2. Writing to an archive copies the whole signal in memory (_signal.py:2561). tobytes() duplicates the array, and the FLAC path makes two copies (BytesIO plus getvalue()).
    Fix: Stream it with self._zipfile.open(ZipInfo(...), "w", force_zip64=True) and f.write(memoryview(b_write)). For FLAC, soundfile needs a seekable target, so keep the BytesIO but write buf.getbuffer() instead of getvalue().

Structure and cleanup

  1. Archive support is a third branch in about 10 I/O helpers (record.py:2132). Each one now handles local, pn_dir and archive separately, and _rdrecord repeats rdheader's archive branch.
    Fix: Put the sources behind one small interface with open(name) and size(name), implemented for local/fsspec, PhysioNet and archive. Pass that interface through the helpers instead of the three arguments, and have _rdrecord call rdheader. This could be a follow-up PR.

  2. Duplicate checks in read_header and friends (archive.py:184).
    Fix: Drop the size check before the read, because the bounded f.read(MAX_HEADER_SIZE + 1) already enforces the limit. Move the .hea scan into a _header_names() helper used by both header_file_name and _check_new_file. In exists(), use self._zipfile.NameToInfo (or try getinfo) instead of rebuilding namelist() on every call.

A .wfdb archive is a ZIP file holding the header, signal and
annotation files of one record. rdrecord, rdsamp, rdheader and rdann
now read from an archive when given its path, including the .wfdb
extension.

The header is the file named after the archive, or the only header in
it. Signal and annotation files are read by the names that follow from
the header, and a missing file is an error. rdrecord reads the header
and signals from a single open archive. Multi-segment records and
archives in cloud storage or on PhysioNet are not supported yet.

An archive holds a single record: WFDBArchive refuses to write a
second header file, so an archive can't be written that rdrecord
cannot read. Annotation and other files can be added alongside the
record.
wrsamp and Record.wrsamp take a wfdb_archive argument: a path creates
a new archive containing the header and dat files of the record, and
an open WFDBArchive lets several files be written to one archive.

A new archive is written to a temporary file named after it, and moved
into place only when writing succeeds, so a failed write leaves no
partial archive and an existing archive at the same path is either
fully replaced or left unchanged. The new archive gets the permissions
of the archive it replaces, or the default permissions for new files.
ZIP can't represent dates before 1980, and ZipFile.write raises a
ValueError for such files by default. This happens with files from
reproducible builds (SOURCE_DATE_EPOCH=0), for example. These files now
get the earliest date ZIP supports.
To give a new archive the default permissions for new files, the umask
was read by setting it to 0 and back. The umask is process-wide, so
files that another thread created in between came out world-writable.

The temporary file is now created with os.open and mode 0o666, so the
operating system applies the umask, instead of with tempfile.mkstemp,
which uses 0o600. The permissions are only copied when an existing
archive is replaced.
When an archive is discarded, the temporary file was only removed if
closing the zip file succeeded, so a failure (such as a full disk)
left it behind.
Using a WFDBArchive after closing it failed with an AttributeError on
None. It now raises a ValueError saying that the archive is closed.
When wfdb_archive was a path, write_dir was silently ignored, so the
archive ended up in the current directory. A relative archive path is
now relative to write_dir, like the files of a record written without
an archive. An absolute path is used as it is.
A path such as '100.WFDB' was treated as a plain record name, and
WFDBArchive added another '.wfdb' to it. The extension is now compared
without regard to case.
Signal files were copied before being added to an archive: tobytes()
for dat files, and BytesIO.getvalue() for FLAC files. WFDBArchive.write
now streams any bytes-like object into the archive, so the array (or the
FLAC buffer) is passed without a copy. The archive entries are written
as before, and ZIP64 is used for files of 2 GiB or more.
- read_header no longer checks the declared size of the header before
  reading it: the bounded read already enforces the limit, whatever size
  the archive declares.
- The scan for header files is in one helper, used by header_file_name
  and _check_new_file.
- exists() looks the file up with getinfo, instead of building the list
  of all files on every call.
@tompollard

Copy link
Copy Markdown
Member Author

Thanks for the thorough review! I've pushed a commit for each point I fixed. The tests added for 1–6 fail without their fix; 8 has a new test for the streamed writes, and 10 is a refactor covered by the existing tests.

Bugs

  1. Fixed in 2afd1c3: the zip file is opened with strict_timestamps=False.
  2. Fixed in 23cc7a5: the temporary file is created with os.open(..., 0o666) and a random name, so the umask is applied by the OS and never read or changed. The mode is only copied when an existing archive is replaced. The test also checks a stricter umask (027), and that os.umask is never called.
  3. Fixed in e573d50, as you suggested.
  4. Fixed in 1d6261d: _require_open() is called by every method that uses the zip file, so a closed archive raises ValueError: Archive '...' is closed.
  5. Fixed in eae96a9: a relative archive path is now relative to write_dir, like the files of a record written without an archive, and an absolute path is used as it is. I preferred this to raising an error when both are given.
  6. Fixed in 90f72c2: the check is now case-insensitive in is_archive_path, and WFDBArchive.__init__ uses the same function.

Performance

  1. I'd leave this for a follow-up. I measured it: seeking 200 MB into a stored entry takes 0.016 s on 3.11 (with the file in the page cache) and is instant on 3.12, while a deflated entry takes about the same on both. Since wfdb stores dat files uncompressed, this mainly matters for multi-GB files on 3.11, or for archives made by other tools. Reading the local header ourselves means handling ZIP64, data descriptors and encryption, so I think it fits better with point 9 (see below).
  2. Fixed in 7f2e29b: WFDBArchive.write now streams any bytes-like object, so the array (or the FLAC buffer, via getbuffer()) is written without a copy. The entries are written as before. ZIP64 is only used for files of 2 GiB or more, rather than force_zip64=True for every file. To be fair, this doesn't change the peak memory in practice: writing an 8 MB signal peaks at about 144 MB, with or without an archive, because of the format conversion earlier in wr_dat_file. That would be worth improving separately.

Structure and cleanup

  1. Agreed, but I'd do this as a follow-up after this PR and WIP: Support multi-segment records in .wfdb archive files #583, which adds more archive branches for multi-segment records. It also changes the PhysioNet and cloud code paths, which are mostly covered by the network tests. A source object would also be a natural place for the fast reads from point 7. One note: _rdrecord reads the header from the archive it already has open, so the header and signals can't come from different files if the archive is replaced in between. With a source object, rdheader could take the source and keep that.
  2. Fixed in 889c97d: the size check before the bounded read is gone, the .hea scan is in _header_names(), and exists() uses getinfo() (rather than NameToInfo, which isn't documented).

I'll rebase #583 onto these changes next.

@lukepayyapilli lukepayyapilli left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Tom, checked 889c97df locally. The offline tests pass on Python 3.11 (132 tests), and ruff check and ruff format --check are clean. I also went back over @cbrnr's review. For 1 to 6, each new test fails with the source from before its fix and passes with it. For 8, test_archive_write_bytes_like checks the archive entries rather than the copy (it passes before the change too), and I measured the same as you: writing an 8 MB signal peaks at about 126 MB, with or without an archive, before and after the change. For 7, I see the same pattern: seeking 200 MB into a stored entry takes 0.044 s on 3.11 and no measurable time on 3.12, and a deflated entry takes about 0.07 s on both, so leaving 7 and 9 for the follow-up with #583 makes sense to me. A few things from reading archives made by other tools, and two questions about the format:

  • Archives zipped on macOS: when Finder zips 100.hea and 100.dat, it also adds __MACOSX/._100.hea and __MACOSX/._100.dat. _header_names() counts the first as a second header, so if the archive is renamed (for example rec2.wfdb), rdrecord fails with Could not find 'rec2.hea' ... (it contains several header files: ['100.hea', '__MACOSX/._100.hea']). The spec draft in wfdb/wfdb-spec#21 says the files are at the top level, so the suggestion only counts top-level headers, with a test that fails without it. It also gives a clearer error for a zipped folder (100/100.hea, 100/100.dat): Could not find '100.hea' instead of Could not find '100.dat'. An archive made with the zip tool, where everything is deflated, and a renamed archive both read correctly.
  • Header compression: the outline in wfdb/wfdb-spec#21 says header files "should be stored uncompressed", and your last comment there says to start without any zip compression, but _compress_type() deflates headers. Archives stay readable either way, but if headers were stored too, a reader such as the WFDB C library wouldn't need zlib at all. It's worth settling before archives are written in the wild; changing it would touch _compress_type(), the module docstring, test_create_archive_compression and the docs.
  • Finding archives: section 4.1 of the outline has tools look for 100.wfdb when the loose files aren't found, whereas here an archive is only read when the path ends in .wfdb. Requiring the explicit path seems simpler and less surprising to me, but the spec draft should say whichever is intended.

Comment thread src/wfdb/io/archive.py Resolved
Comment thread tests/test_archive.py Resolved
@tompollard

Copy link
Copy Markdown
Member Author

@cbrnr Any objections to merging this? If you're okay with doing so, I'll [try to] work out how to roll in support for the .wfdb files in the C package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants