bugfix/fsspec-local-file-opener-cpp-buffer-for-czi

[evamaxfield]

Description

Looks like one more thing snuck in on the recent fsspec releases. A change to the __enter__ behavior of fsspec.local.LocalFileOpener: fsspec/filesystem_spec@06c4003

Pinging @martindurant to see if this is a verified "breaking change" or unintended.

Thanks for contributing!

[evamaxfield]

The context here is that we generally can ignore the fsspec typing and simply rely on the trusty: "io.IOBase" or "io.BufferedIOBase" with one exception: CZI reading calls out to a C++ lib which strictly enforces typing and raises an error when it encounters something besides the "io.BufferedIOBase" it expects.

The change made in fsspec upstream simply changes the return type of __enter__ and breaks C++ lib usage.

[codecov-commenter]

Codecov Report

Merging #425 (8606b27) into main (c325ba2) will increase coverage by 10.19%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##             main     #425       +/-   ##
===========================================
+ Coverage   83.83%   94.03%   +10.19%     
===========================================
  Files          46       46               
  Lines        3972     3972               
===========================================
+ Hits         3330     3735      +405     
+ Misses        642      237      -405     

Impacted Files	Coverage Δ
aicsimageio/readers/czi_reader.py	95.22% <100.00%> (+88.53%)	⬆️
aicsimageio/aics_image.py	87.73% <0.00%> (+0.47%)	⬆️
aicsimageio/readers/reader.py	92.52% <0.00%> (+1.14%)	⬆️
aicsimageio/tests/image_container_test_utils.py	100.00% <0.00%> (+2.24%)	⬆️
aicsimageio/metadata/utils.py	90.24% <0.00%> (+5.85%)	⬆️
...eio/tests/readers/extra_readers/test_czi_reader.py	100.00% <0.00%> (+100.00%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c325ba2...8606b27. Read the comment docs.

[martindurant]

The change in fsspec was intentional, as the LocalFileOpened instances are serialisable, but the builtin python instances are not, so the previous behaviour was preventing pickle within a context. LocalFileOpener is a subclass of IOBase, not io.BufferedIOBase. Changing that might be reasonable.

[AetherUnbound]

Nice find, quick fix!

[toloudis]

I tried messing with the pybind11 code in aicspylibczi for a few minutes to try to change IOBufferedReader to IOBase but had no luck. It will take a deeper dive to understand how that interface is working to accept python classes.

[toloudis]

See AllenCellModeling/aicspylibczi#109

[toloudis]

I would suggest making this a really small function that returns resource.f so that the actual .f part only shows up in one place. It's a minor thing that could help hide that implementation detail.
for example:

def get_file_handle(fs_local_opener):
    return fs_local_opener.f

[toloudis]

or maybe get_buffered_reader or whatever

[evamaxfield]

I am going to merge without this. Returning an open buffer means we may have a open file handle down the line.

Would need to do something like:

def get_file_handle(fs, path):
    yield fs.open(path).f

but that also doesn't ensure a closed handle after we are done.

[toloudis]

I'm fine w/that, but what's the difference between

with fs.open(path) as file:
    CziFile(function_that_returns_file_dot_f(file))

and

with fs.open(path) as file:
    CziFile(file.f)

?

[toloudis]

I wasn't saying to wrap the with part in a function. only the bit that calls .f

[evamaxfield]

Ahhhh that was my confusion. I thought you were asking for the with / context manager to be included. My mistake.