Skip to content

Conversation

@pauldmccarthy
Copy link
Contributor

Hi @effigies, I hope you're well! Also pinging @vanandrew, as this builds on the work you did in #1005.

This PR adjusts nibabel so that it will use one of compression.zstd, backports.zstd, or pyzstd, in that order of preference, for handling zstandard-compressed files.

I've been playing around with adding zstandard support to the core FSL tools, and noticed that nibabel currently relies on pyzstd. Support for this format is now built into Python as of 3.14 via compression.zstd, and the recommendation for older Python versions is to use the backports.zstd package, as outlined in this handy migration guide:

https://pyzstd.readthedocs.io/en/stable/

  1. To handle conditional import of one of several optional modules, I've adjusted the nibabel.optpkg.optional_package function to accept either a single module name, or a sequence of names - it will try to import each in order, returning the first one that succeeds.

  2. I moved _zstd_open, _gzip_open, and DeterministicGzipFile from nibabel.openers into nibabel._compression, as they seem like a better fit there.

  3. The compression.zstd.ZstdFile API differs very slightly from the pyzstd.ZstdFile API - the latter expects an overloaded level_or_option parameter, whereas the former expects separate level / option parameters. I've adjusted nibabel so that it will accept all three, and will pass the appropriate parameter to the underlying library.

  4. I've tried to update the metadata files (uv.lock, tox.ini etc), but am not particularly familiar with how the project is managed these days, so let me know if there's anything I ned to fix, or anything else that is needed.

Thanks!

@codecov
Copy link

codecov bot commented Nov 26, 2025

Codecov Report

❌ Patch coverage is 79.03226% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.42%. Comparing base (d537d33) to head (2d0040d).

Files with missing lines Patch % Lines
nibabel/_compression.py 66.66% 10 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1444      +/-   ##
==========================================
- Coverage   95.45%   95.42%   -0.03%     
==========================================
  Files         209      209              
  Lines       29805    29816      +11     
  Branches     4479     4484       +5     
==========================================
+ Hits        28449    28451       +2     
- Misses        924      932       +8     
- Partials      432      433       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@effigies
Copy link
Member

effigies commented Nov 26, 2025

Hey @pauldmccarthy, thanks for this! I think this makes a lot of sense, but it would be simplest just to drop pyzstd support altogether and only support compression.zstd (and backports.zstd until 3.14+).

My overall strategy would be to write things how we would want them to look with Python 3.14 plus, and then add guards in blocks labeled #PY313 that we can easily find and delete when dropping 3.13, e.g.

try:
    from compression import zstd
    HAVE_ZSTD = True
except ImportError:  # PY313
    HAVE_ZSTD = False

...

if TYPE_CHECKING:
    if not HAVE_ZSTD:  # PY313
        from backports import zstd
        HAVE_ZSTD = True
    ...
else:
    if not HAVE_ZSTD:  # PY313
        zstd, HAVE_ZSTD = optional_package('backports.zstd')

becomes:

from compression import zstd

And HAVE_ZSTD gets removed wherever it's found. This approach also doesn't require changing optpkg.

I would also aim to use zstd.ZstdFile directly in openers, and use _compression.open_zstd as a stop-gap that raises a FutureWarning/DeprecationWarning on level_or_option and translates into level and options. Something like:

def zstd_open(
    filename: str,
    mode: Mode = 'r',
    *,
    level: int | None = None,
    options: dict | None = None,
    zstd_dict: zstd.ZstdDict | None = None,
    level_or_option: int | dict | None = None,
) -> zstd.ZstdFile:
    if level_or_option is not None:
        # Warn
        # Set level or options (raising if duplicated)
    # Let ZstdFile raise its own error if level or options shouldn't be set
    return zstd.ZstdFile(filename, mode, level=level, options=options, zstd_dict=zstd_dict)

uv and tox stuff looks great to me. I'm glad it wasn't too challenging!

Looks like you need to run tox -e style-fix, and tox -e typecheck and address the complaints. If you get stuck, I'm happy to take a look.

@pauldmccarthy
Copy link
Contributor Author

@effigies, no problem - I like your suggestions! I had wondered about dropping pyzstd entirely, but thought I'd start with the more conservative approach.

I was just looking through the ruff output ... is it worth continuing to expose HAVE_INDEXED_GZIP, DeterministicGzipFile, etc in nibabel.openers, on the off-chance that some downstream code has imported them directly?

@effigies
Copy link
Member

This is all pretty deep API. Internally, we expect to use Opener, not DeterministicGzipFile, and a downstream user digging into our API would be more likely to encounter FileHolder when using img.make_file_map() than Opener directly, and even that is pretty much an implementation detail that allows us to make ImageClass.from_filename and ImageClass.to_filename work.

I would be okay with noting the removal in the changelog. If someone complains, we can put out a bug-fix release where we restore them or add a module-level __getattr__() to officially deprecate them.

@pauldmccarthy
Copy link
Contributor Author

I found an unrelated issue while testing locally:

___________________________________________________________ TestRunAllTests.test_first ____________________________________________________________

self = <nibabel.tests.test_api_validators.TestRunAllTests object at 0x7dcc2a3d3ca0>

    def meth(self):
        for imaker, params in self.obj_params():
>           validator(self, imaker, params)

nibabel/tests/test_api_validators.py:22:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <nibabel.tests.test_api_validators.TestRunAllTests object at 0x7dcc2a3d3ca0>, obj = 1, param = 2

    def validate_first(self, obj, param):
>       self.run_tests.add('first')
E       AttributeError: 'dict' object has no attribute 'add'

nibabel/tests/test_api_validators.py:108: AttributeError

It looks like run_tests should be a set()?

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.

2 participants