[WIP] Added max_block and request_batch to pyfive.File and backend

[dwest77a]

Description

• Added max_block and request_batch as parameters to the pyfive.File declaration. This is relayed to the backend bulk_fsspec method which now uses the merge_requests utility from fsspec if max_block is defined. Request batch allows configuration via the fsspec cat_ranges method to restrict the number of simultaneous requests down from the default for remote connections of 1280.

Checklist

• This pull request has a descriptive title and labels
• This pull request has a minimal description (most was discussed in the issue, but a two-liner description is still desirable)
• Unit tests have been added (if codecov test fails)
• Any changed dependencies have been added or removed correctly (if need be)
• If you are working on the documentation, please ensure the current build passes
• All tests pass

[valeriupredoi]

thanks @dwest77a 🍺 Note that if you wish the PR to be truly draft, you can always convert it to a Draft PR. I approved the tests to run here, and if you don't mind, I'll also add you to PyActiveStorage 🍻

[dwest77a]

This relates to issue #257

[valeriupredoi]

@dwest77a the issue with pre-commit kicks in since you change the functional call and args:

131c139,141
<                 self._read_bulk_fsspec(fh, chunks, out, dtype)
---
>                 self._read_bulk_fsspec(
>                     fh, chunks, out, dtype, max_block=max_block, batch_size=batch_size
>                 )

posix is defined as attr only if not fh, and the same for _fh - not great in terms of coding, but also mypy is not catching that. I think you can safely ignore those, and to run the tests you can temporarily turn off the call to pre-commit in the Github Action workflow

[valeriupredoi]

@dwest77a I realized we've had quite a few of these [attr-defined] mypy barfs, that I had to manually allow - the way the code is written is a bit confusing to mypy, and having checked manually all is kosher, I told mypy to mind its own business in 702bf12 - note, though, it's still a good idea to run pre-commit install && pre-commit run -a when you done with code changes, so to catch any issues locally, and fix em 🍺

[codecov]

Codecov Report

❌ Patch coverage is 86.66667% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.63%. Comparing base (050f3e7) to head (34ae13e).
⚠️ Report is 15 commits behind head on main.

Files with missing lines	Patch %	Lines
pyfive/h5d.py	84.61%	3 Missing and 1 partial ⚠️

❌ Your patch status has failed because the patch coverage (86.66%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #256   +/-   ##
=======================================
  Coverage   78.63%   78.63%           
=======================================
  Files          15       15           
  Lines        3491     3511   +20     
  Branches      555      560    +5     
=======================================
+ Hits         2745     2761   +16     
- Misses        603      606    +3     
- Partials      143      144    +1     

☔ View full report in Codecov by Harness.
📢 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.

[dwest77a]

@valeriupredoi thanks, I'll look into the test coverage now.

[valeriupredoi]

Good man Dan 🍺

[dwest77a]

@valeriupredoi happy to reconfigure this in some way that doesn't use a hardcoded path, it does need to be remote to test the block and batch sizes though.

[bnlawrence]

I'd like to get the two other pull requests that affect file handling into main before doing this one ... just in case there is an interaction ... I'll try and get them done in the next couple of days, and then move to this one.

[bnlawrence]

Two fixes in that: 1) added a local test instead of the remote test, I think that covers the same ground, and in doing that, the AI discovered a bug in h5d.py ... and fixed it. Before the backend sliced each original chunk out of its containing merged response, it attempted to decode an entire merged buffer as one chunk and failed with a ValueError in the new test.

@dwest77a Can you check that you are happy with the revised test (and bug fix)?

[bnlawrence]

(When you're happy with the idea, I'll worry about the conflicts.)

[dwest77a]

Yes all looks good to me in terms of fixing the tests and the issue with handling the merged responses!

[bnlawrence]

@valeriupredoi I'm happy with this now. You can merge if you are happy with the codecov

[valeriupredoi]

Perfect! Many thanks @dwest77a and @bnlawrence 🍺🍺