fix(dataset_tools): don't split XRootD URLs at the port colon when parsing a files list - #1588
fix(dataset_tools): don't split XRootD URLs at the port colon when parsing a files list#1588NJManganelli wants to merge 5 commits into
Conversation
…s-list parsing
DatasetSpec.preprocess_data split each files-list entry with
rsplit(":", maxsplit=1) to separate the filename from a trailing ROOT
object path, guarding only remainders starting with "//". An XRootD URL
with a port and no object path (e.g. root://host:1094//store/f.root)
therefore split at the port colon, yielding filename "root://host" with
a spurious object_path "1094//store/f.root".
Delegate the split to uproot's file_object_path_split via a new
_file_object_path_split helper (with a semantics-mirroring fallback),
which correctly handles the root:// scheme, ports, //-paths, object
paths containing '/', and local paths. Add regression tests covering all
these cases.
Partially addresses scikit-hep#1578 (filespec.py rsplit(":") bug).
Assisted-by: Claude Fable 5
lgray
left a comment
There was a problem hiding this comment.
🤖 AI text below 🤖
Summary. This correctly fixes the port-colon mis-split for files-list input by delegating to uproot's file_object_path_split. I verified the fix end-to-end: on master a list entry like root://host.cern.ch:1094//store/data/dir.parquet is mangled into filename root://host.cern.ch + object_path 1094//... (→ RuntimeError in identify_file_format); at PR head it parses correctly. The uproot-backed helper matches uproot exactly across the full matrix (port ± object path, object paths with /, local abs/rel, https://host:port, Windows drive). The dict-input path is genuinely untouched (split runs only in the isinstance(files, list) branch of the mode="before" validator), and dict/model round-tripping still passes. Full tests/test_dataset_tools_filespec.py = 152 passed. Good, well-scoped fix — one real issue in the fallback, below.
Should fix
- The pure-Python fallback re-introduces the exact bug this PR fixes, and nothing tests it (
src/coffea/dataset_tools/filespec.py:38-44). The fallback splits onrpartition(":"); for a port URL without an object path the last colon is the port colon, soroot://host:1094//store/data/dir.parquet→('root://host', '1094//store/data/dir.parquet')— precisely the manglingmasterproduced. I confirmed this with a differential test (uproot vs fallback over the PR's own matrix): they disagree on all three port-URL-no-object-path cases, and a forced-fallback run (del uproot._util.file_object_path_split) fails 2 of the 8 helper cases — including the parquet-directory case thattest_list_input_xrootd_url_with_port_no_object_pathis meant to guard. Because uproot's splitter keys off a.root/regex match while the fallback treats any non-/trailing colon-suffix as an object path, the fallback can't be made faithful with a one-line tweak. Options, in order of laziness:- Drop the fallback entirely and import
from uproot._util import file_object_path_splitat module top. coffea already hard-depends on uproot; if that private symbol ever moves, a loudImportErrorat import time is far better than silently reverting to buggy parsing. This is the smallest, safest diff. - If you keep a fallback, it must reproduce uproot's
.root-anchored semantics (notrpartition), and add a test that runs the regression matrix with the fallback forced active — otherwise the safety net is untested and wrong for the very case it's insuring.
- Drop the fallback entirely and import
except Exceptionis too broad (filespec.py:37). The comment says the guard is for uproot's helper being "unavailable/moved," i.e.ImportError/AttributeError. As written it also swallows any exception uproot's splitter itself raises on some input and silently falls through to the (buggy) fallback. Narrow toexcept (ImportError, AttributeError).
Nits / Optional
files_list = files(filespec.py:514) is a redundant alias; you can iteratefilesbefore rebinding, or just inline. Trivial.- Worth a one-line comment noting
uproot._util.file_object_path_splitis a private uproot API (hence the guard) — future readers will wonder why a public function isn't used; there isn't one.
Test coverage. Non-vacuous and discriminating — proven by revert: test_list_input_xrootd_url_with_port_no_object_path fails on master with the mangled root://host.cern.ch parse and passes here, and the tests assert the full (filename, object_path) pair, not just "didn't raise." The matrix is well chosen (port ± object path, object paths with /, local abs/rel, bare filename). One gap: every test exercises only the uproot path; the fallback (the whole reason for the try/except) is never run, and it fails the port case when it is — please add a forced-fallback test or delete the fallback per above. A C:\...:Events case and an explicit dict-input-unchanged assertion would round it out (both verified fine here).
Pydantic. Idiomatic: the split lives in the existing @model_validator(mode="before"), correct mode for reshaping raw input, mutating-and-returning data on the list branch only; no mutable-default or recompilation concerns introduced.
…path_split
The fallback still split XRootD URLs with a port and no object path at the port
colon (root://host:1094//f.root -> ('root://host', '1094//f.root')) — the exact
bug this fix targets — and was never exercised while uproot's parser is present.
Delegate to uproot unconditionally and add a contract test pinning the import and
the behaviour we rely on, so an upstream move is flagged in CI (coffea tracks
uproot closely) rather than silently mis-parsing URLs.
Assisted-by: Claude Opus 4.8
|
Rather than have a fallback, just rely on uproot's implementation, and add a test to catch any functionality drift. Rerunning tests |
…r only Drop issue-tracker references, CI-contract narration, and "battle-tested"/ "error-prone" editorializing from the _file_object_path_split helper docstring, the files-list promotion comment, and the port-colon parsing tests. Comments now minimally describe existing behavior. Assisted-by: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01XeYa8sEdeLGa1VX2frvoNz
850e105 to
523950e
Compare
master reorganized DatasetSpec.preprocess_data to build a new_data dict while iterating the input, which conflicted with this branch's edit to the old structure. Resolved in favor of master's structure, re-applying the fix on top: the files-list promotion uses uproot's splitter instead of rsplit on the last colon, so an XRootD URL carrying a port is not split at the port. Assisted-by: Claude Opus 4.7 (1M context)
|
This PR straightforwardly delegates to a better implementation in uproot, and we add a few tests to try to ensure that functionality suits our needs. Anything else needed? |
Part of #1578 — the
filespec.pyrsplit(":")parsing bug.When
DatasetSpecis givenfilesas a list, each entry was split withrsplit(":", maxsplit=1)to peel off a trailing ROOT object path, guarding only remainders starting with//. This mishandles XRootD URLs that carry a port, e.g.root://host:1094//store/f.root: the split lands on the port colon, silently mangling the filename toroot://hostand inventing an object path1094//store/f.root.This PR delegates the split to uproot's battle-tested
file_object_path_split(via a small_file_object_path_splithelper with a semantics-mirroring pure-Python fallback should that internal helper move), which correctly understands theroot://scheme, ports,//-paths, object paths containing/, and local absolute/relative paths. The dict-input path and the dual dict/model contract are unchanged. Regression tests cover the full case matrix (file.root:Events,file.root:Dir/Tree, scheme URLs with and without ports and object paths, local paths); the port-URL case fails onmasterand passes here.tests/test_dataset_tools_filespec.py: 152 passed (141 existing + 11 new). Pre-commit clean.🤖 Generated with Claude Code