Skip to content

fix: correct errors found while testing file reading and sources - #1825

Merged
ariostas merged 1 commit into
scikit-hep:mainfrom
ariostas:fix/reading
Nov 10, 2026
Merged

ariostas merged 1 commit into
scikit-hep:mainfrom
ariostas:fix/reading

Conversation

@ariostas

@ariostas ariostas commented Nov 6, 2026 •

Copy link
Copy Markdown
Member

🤖 AI text below 🤖

Part of the split of #1814 (the list is in #1834). These bugs were found while writing the coverage tests of #1814, and tests/test_1825_fix_reading.py holds their regression tests: every test in it fails on main (for parametrized tests, at least one of their cases).

  • HTTP
    • Responses are closed on every path, which removes the ResourceWarnings.
    • _ResponseBuffer.readline no longer loops forever at the end of a stream that has no final newline.
  • Empty requests: a step of arrays/iterate within baskets that were already read requests nothing, and the HTTP and XRootD sources return no chunks for no ranges, instead of sending an empty multi-range or vector read.
  • uproot.dask: it reads grouped TBranches, and with library="np" it says why it can't read one.
  • cut: a cut that is neither None nor a string raises a TypeError.
  • Deflate: the decompressor passes the uncompressed size positionally, since the deflate package doesn't take bufsize as a keyword.
  • Files and directories
    • Files without streamers have streamer rules.
    • streamer_dependencies keeps the order of versions.
    • Directory paths can go through an RNTuple to its fields.
  • Executors: ResourceTrivialExecutor has num_workers.

@github-actions github-actions Bot added the type/fix PR title type: fix (set automatically) label Nov 6, 2026
@ariostas
ariostas marked this pull request as ready for review November 6, 2026 14:47
These were found by the tests of scikit-hep#1814, which are the regression tests here.
The fixes are to HTTP and XRootD sources (closing responses, empty requests,
readline at the end of a stream), the deflate decompressor, streamer
dependencies, TBranch steps inside already-read baskets, paths into
RNTuples, files without streamers, cuts that aren't strings, and grouped
TBranches in uproot.dask.

Assisted-by: Claude Code:claude-opus-5-5
@codecov

codecov Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.71429% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.37%. Comparing base (b4acc0f) to head (bc65668).
⚠️ Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
src/uproot/source/http.py 78.94% 3 Missing and 1 partial ⚠️
src/uproot/reading.py 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
src/uproot/_dask.py 87.43% <100.00%> (+0.06%) ⬆️
src/uproot/behaviors/TBranch.py 87.98% <100.00%> (+0.11%) ⬆️
src/uproot/compression.py 81.25% <100.00%> (+3.90%) ⬆️
src/uproot/source/futures.py 92.46% <100.00%> (+2.66%) ⬆️
src/uproot/source/xrootd.py 81.60% <100.00%> (-1.41%) ⬇️
src/uproot/reading.py 85.85% <66.66%> (+2.79%) ⬆️
src/uproot/source/http.py 82.97% <78.94%> (+11.04%) ⬆️

... and 4 files with indirect coverage changes

ariostas added a commit to ariostas/uproot5 that referenced this pull request Nov 6, 2026
MET_pt is in one TBasket, so with steps of 15 entries the later steps of a
file had nothing to request, and the XRootD source then sent an empty vector
read, which failed now and then. scikit-hep#1825 stops sending it; until then, these
tests iterate in one step per file.

Assisted-by: Claude Code:claude-opus-5-5
ianna pushed a commit that referenced this pull request Nov 7, 2026
* test: tighten the existing tests and make their fixtures robust

Changes to existing tests from #1814 that don't depend on its fixes: they
check values instead of only running, restore checks that had been dropped,
close the files they open, and run concurrently (each XRootD server has its
own admin path and waits until it accepts connections, and reset_classes puts
the original model classes back).

Assisted-by: Claude Code:claude-opus-5-5

* test: keep the tests that the new coverage tests replace

Six tests (of Future.result timeouts, ThreadPoolExecutor.shutdown, the order
of LRUCache, and Library equality) were deleted here in favor of tests in
#1829, #1830 and #1832, which are based on this PR; until those are merged
nothing else would cover them. They are now deleted together with the tests
that replace them. A comment no longer points to a file of #1828.

Assisted-by: Claude Code:claude-opus-5-5

* test: iterate over XRootD in one step per file

MET_pt is in one TBasket, so with steps of 15 entries the later steps of a
file had nothing to request, and the XRootD source then sent an empty vector
read, which failed now and then. #1825 stops sending it; until then, these
tests iterate in one step per file.

Assisted-by: Claude Code:claude-opus-5-5

@ianna ianna left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ariostas -- Thanks! It looks like coverage is lower that expected but since more tests should be merged later I let you to decide if you want to merge this one now.

@ariostas
ariostas merged commit 9ce5501 into scikit-hep:main Nov 10, 2026
23 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type/fix PR title type: fix (set automatically)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants