While working on reviewing #3433 and having Claude look at it, it identified this issue. I'm not as familiar with the FSFile stuff as @mraspaud or maybe @pnuu (?) so I was hoping you guys could look at what Claude identified. Below is what Claude thought was useful related to the issue including a suggested fix.
Description
satpy.readers.core.utils.unzip_file() dispatches on the input type:
https://github.com/pytroll/satpy/blob/main/satpy/readers/core/utils.py#L201-L204
if isinstance(filename, str):
return _unzip_local_file(filename, prefix=prefix)
elif isinstance(filename, FSFile):
return _unzip_FSFile(filename, prefix=prefix)
Anything that is neither a str nor an FSFile falls off the end and returns None
implicitly. None is also the "this file was not compressed" return value, so a
pathlib.Path pointing at a .bz2 file is indistinguishable from an uncompressed
file, and the caller goes on to hand the still-compressed file to netCDF4 / xarray /
the HRIT parser.
This matters because FileYAMLReader passes filenames through to file handlers
untouched — filename_items_for_filetype() yields the objects the user supplied, and
_new_filehandler_instances() calls filetype_cls(filename, ...) with them. So
Scene(filenames=list(Path("/data").glob("*.bz2"))) reaches the file handler as Path
objects, while Scene(filenames=glob.glob("/data/*.bz2")) reaches it as str. Only
the second one gets decompressed.
The internals already look PathLike-ready — _unzip_local_file() does
os.fspath(filename).endswith("bz2") — so it seems to be only the dispatch that is
str-only.
Affected readers
Every caller of unzip_file() / unzip_context():
satpy/readers/ahi_hsd.py
satpy/readers/ahi_l1b_gridded_bin.py
satpy/readers/nwcsaf_nc.py
satpy/readers/core/hrit.py (via unzip_context(), which yields the original
compressed filename unchanged in this case)
and any future reader that adopts the pattern (e.g. the METimage/VII bz2 support in #3433).
How to reproduce
from pathlib import Path
from satpy.readers.core.utils import unzip_file
unzip_file("/data/some_file.nc.bz2") # -> '/tmp/tmpXXXXXXXX' (decompressed)
unzip_file(Path("/data/some_file.nc.bz2")) # -> None (silently ignored)
End to end, this surfaces as an opaque failure far from the cause, e.g. netCDF4
raising an HDF error because it was handed bzip2 bytes.
Expected behaviour
Path (and any os.PathLike) should behave the same as the equivalent str.
Suggested fix
Order matters here, because FSFile is itself a subclass of os.PathLike — it has to
be checked first or it will be routed to the local-file branch:
def unzip_file(filename: str | os.PathLike | FSFile, prefix=None):
if isinstance(filename, FSFile):
return _unzip_FSFile(filename, prefix=prefix)
return _unzip_local_file(os.fspath(filename), prefix=prefix)
A test in satpy/tests/reader_tests/test_utils.py covering a Path input alongside
the existing str cases would pin this down.
Possible follow-up
Separately from the type dispatch, the None-means-two-things return value is what
turns this into a silent failure rather than a loud one. Not proposing to change the
signature — several readers depend on the falsy-means-not-compressed behaviour — but
worth noting if this area is ever revisited.
While working on reviewing #3433 and having Claude look at it, it identified this issue. I'm not as familiar with the FSFile stuff as @mraspaud or maybe @pnuu (?) so I was hoping you guys could look at what Claude identified. Below is what Claude thought was useful related to the issue including a suggested fix.
Description
satpy.readers.core.utils.unzip_file()dispatches on the input type:https://github.com/pytroll/satpy/blob/main/satpy/readers/core/utils.py#L201-L204
Anything that is neither a
strnor anFSFilefalls off the end and returnsNoneimplicitly.
Noneis also the "this file was not compressed" return value, so apathlib.Pathpointing at a.bz2file is indistinguishable from an uncompressedfile, and the caller goes on to hand the still-compressed file to netCDF4 / xarray /
the HRIT parser.
This matters because
FileYAMLReaderpasses filenames through to file handlersuntouched —
filename_items_for_filetype()yields the objects the user supplied, and_new_filehandler_instances()callsfiletype_cls(filename, ...)with them. SoScene(filenames=list(Path("/data").glob("*.bz2")))reaches the file handler asPathobjects, while
Scene(filenames=glob.glob("/data/*.bz2"))reaches it asstr. Onlythe second one gets decompressed.
The internals already look PathLike-ready —
_unzip_local_file()doesos.fspath(filename).endswith("bz2")— so it seems to be only the dispatch that isstr-only.
Affected readers
Every caller of
unzip_file()/unzip_context():satpy/readers/ahi_hsd.pysatpy/readers/ahi_l1b_gridded_bin.pysatpy/readers/nwcsaf_nc.pysatpy/readers/core/hrit.py(viaunzip_context(), which yields the originalcompressed filename unchanged in this case)
and any future reader that adopts the pattern (e.g. the METimage/VII bz2 support in #3433).
How to reproduce
End to end, this surfaces as an opaque failure far from the cause, e.g. netCDF4
raising an HDF error because it was handed bzip2 bytes.
Expected behaviour
Path(and anyos.PathLike) should behave the same as the equivalentstr.Suggested fix
Order matters here, because
FSFileis itself a subclass ofos.PathLike— it has tobe checked first or it will be routed to the local-file branch:
A test in
satpy/tests/reader_tests/test_utils.pycovering aPathinput alongsidethe existing
strcases would pin this down.Possible follow-up
Separately from the type dispatch, the
None-means-two-things return value is whatturns this into a silent failure rather than a loud one. Not proposing to change the
signature — several readers depend on the falsy-means-not-compressed behaviour — but
worth noting if this area is ever revisited.