fix: canonicalize input file paths for graphed duplicate refusal (#1767) by sumitsaraswat362 · Pull Request #1777 · scikit-hep/uproot5

sumitsaraswat362 · 2026-10-25T07:44:32Z

Summary

uproot.graphed_write refused duplicate inputs when spelled identically, but missed entries referring to the same file under different path spellings (such as relative vs absolute paths).

Changes

Added uproot._util.canonical_path to normalize local files via os.path.realpath and remote URLs via fsspec / urllib.parse.
Updated _resolve_entries in uproot._graphed to count and cache files by canonical key, so duplicate entries under alternate path spellings resolve their tree path correctly.
Added canonical key duplicate checking in uproot.graphed_write to refuse duplicate (canonical_path, object_path) inputs before building write plans.
Added regression tests in tests/test_1767_graphed_write_duplicate_paths.py.

…kit-hep#1767)

codecov · 2026-10-25T07:54:26Z

Codecov Report

❌ Patch coverage is 92.18750% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.13%. Comparing base (b9a1dfa) to head (2324c44).

Files with missing lines	Patch %	Lines
src/uproot/_util.py	88.88%	4 Missing and 1 partial ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
src/uproot/_graphed.py	`96.82% <100.00%> (+0.05%)`	⬆️
src/uproot/writing/_graphed_write.py	`98.13% <100.00%> (+0.17%)`	⬆️
src/uproot/_util.py	`65.20% <88.88%> (+1.68%)`	⬆️

... and 2 files with indirect coverage changes

ariostas

🤖 AI text below 🤖

Thanks for the fix! A few comments below. The main one: with known_base_form=, object paths are not normalized, so some duplicate spellings still get through (#1767). Case-insensitive filesystems (macOS) have a similar gap.

ariostas · 2026-10-27T17:13:50Z

        )
+    seen = set()
+    for file_path, object_path in source._file_tree:
+        canon_key = (uproot._util.canonical_path(file_path), object_path or "")


🤖 AI text below 🤖

Only the file path is canonicalized here, not the object path. With known_base_form=, no file is opened and file_tree keeps object paths exactly as typed, so the same tree spelled two ways is still accepted:

uproot.graphed({absp: "events", relp: "/events"}, known_base_form=bf) uproot.graphed({absp: "events;1", relp: "events"}, known_base_form=bf)

These give keys like (realpath, "events") and (realpath, "/events"), which differ, so the rows are written twice. That is the bug #1767 is about. None (single-tree file) vs "events" has the same problem. The object path probably needs normalizing too: strip the leading / and the ;cycle.

ariostas · 2026-10-27T17:13:50Z

            "partition alone); 'files' with explicit 'steps' cannot be written — use "
            "steps_per_file"
        )
+    seen = set()


🤖 AI text below 🤖

This loop repeats the duplicate check and error message from graphed.write.file_bases, just on different keys. Canonicalization is also added separately in three places (the Counter, the directories cache, and here). Could file_tree be canonicalized once in graphed() (both branches) instead? Then file_bases and _PartName would key on it, and every other consumer of _file_tree (e.g. uproot.dask, which per #1767 still reads duplicates twice) would get it for free.

ariostas · 2026-10-27T17:13:50Z

+                    else parsed.netloc
+                )
+            file_path = urllib.request.url2pathname(path_str)
+        return os.path.realpath(os.path.expanduser(file_path))


🤖 AI text below 🤖

os.path.realpath only resolves symlinks. It doesn't fold case on case-insensitive filesystems (macOS's default APFS), and it doesn't detect hard links. On macOS, {"/data/A.root": "events", "/data/a.root": "events"} gives two different keys and the rows are written twice. For local files that exist, comparing (st_dev, st_ino) from os.stat (or using os.path.samefile) would catch both cases.

ariostas · 2026-10-27T17:13:50Z

+
+    if isinstance(file_path, bytes):
+        file_path = os.fsdecode(file_path)
+    if isinstance(file_path, os.PathLike):


🤖 AI text below 🤖

The bytes check runs before the PathLike conversion. A PathLike whose __fspath__ returns bytes is still bytes when it reaches "://" in file_path below, which raises TypeError: a bytes-like object is required, not 'str'. Calling os.fspath first and then os.fsdecode handles both cases.

ariostas · 2026-10-27T17:13:50Z

+    if not ("://" in file_path and parsed.scheme not in ("", "file", "local")):
+        if parsed.scheme in ("file", "local"):
+            path_str = parsed.path
+            if parsed.netloc and parsed.netloc != "localhost":


🤖 AI text below 🤖

A file:// URL with a non-localhost host becomes host/path with no leading slash, and realpath then resolves it relative to the current directory. file://somehost/tmp/a.root turns into <cwd>/somehost/tmp/a.root. If a local relative file somehost/tmp/a.root exists, the two would be wrongly treated as duplicates (or share one opened directory in _resolve_entries). Maybe leave non-local hosts un-canonicalized, or keep them as URLs?

ariostas · 2026-10-27T17:13:50Z

 import re
 import sys
+import urllib.parse
+import urllib.request


🤖 AI text below 🤖

urllib.request is only needed for url2pathname, but importing it at the top level loads it on every import uproot. Before this PR it wasn't in sys.modules after importing uproot. Could it be imported inside canonical_path instead?

ariostas · 2026-10-27T17:13:50Z

    leaf ``TBranch``, or none when the first names a ``TTree``."""
-    listed = collections.Counter(_file_and_path(f, o)[0] for f, o, *_ in files)
+    listed = collections.Counter(
+        uproot._util.canonical_path(_file_and_path(f, o)[0]) for f, o, *_ in files


🤖 AI text below 🤖

canonical_path calls realpath, which does filesystem calls for every path component. It now runs twice per entry here (once for the Counter, once in the loop) and a third time per entry in graphed_write. With thousands of inputs on EOS/NFS/FUSE, that adds up on the driver. Computing the canonical keys once per entry and reusing them would avoid the repeats.

ariostas · 2026-10-27T17:13:50Z

@@ -312,26 +312,27 @@ def _resolve_entries(
    ``/``, which could end in a ``TTree`` or in a ``TBranch``; any other path names a ``TTree``,
    or the first entry's leaf ``TBranch`` when it is the first entry's path. Every entry must select the first entry's
    leaf ``TBranch``, or none when the first names a ``TTree``."""


🤖 AI text below 🤖

Nit: the docstring above (lines 305-307) still says directories is {file_path: directory}, but it is now keyed by canonical_path(file_path). A caller looking up directories[file_path] with the path as typed would get a KeyError.

ariostas · 2026-10-27T17:13:50Z

+    return str(path)
+
+
+def _relative_path(path):


🤖 AI text below 🤖

_relative_path builds <tmp>/../<tmp-basename>/a.root, which is still an absolute path, so the relative-to-cwd spelling from the issue's reproducer is never tested. A monkeypatch.chdir(tmp_path) plus a bare "a.root" would cover it.

ariostas · 2026-10-27T17:13:50Z

+    file_url = f"file://{local_p}"
+    assert canonical_path(file_url) == str(local_p.resolve())
+    assert canonical_path(local_p.as_uri()) == str(local_p.resolve())
+    assert canonical_path("file://somehost/tmp/test.root") is not None


🤖 AI text below 🤖

This asserts nothing useful (canonical_path never returns None for a str). It would be better to assert the expected value, which would also expose the host-handling issue noted in _util.py.

sumitsaraswat362 · 2026-10-28T03:55:31Z

Thanks @ariostas for the detailed review! I have updated the branch to canonicalize file_tree once in graphed(), normalized object paths under known_base_form, handled case folding and hard links with inode tracking, and switched remote URL handling to keep credentials and protocol paths intact. I also updated the tests with cwd relative paths and exact assertions.

…parison

… key canonical_path and canonical_object_path are identity keys for the graphed_write duplicate refusal, not paths to read: rewriting the cycle to ;1 read an older cycle (or none), None no longer meant the file's only TTree, and resolving symlinks or remote URLs on the driver changed what the workers open. Keep _file_tree as given and check the keys in graphed_write. Drop _macos_real_casing, which the inode check covers. Assisted-by: Claude Code:claude-opus-5-5

graphed's name= now sets the session source name, so the duplicate key for a None object path uses the first entry's tree name instead. Assisted-by: Claude Code:claude-opus-5-5

ariostas · 2026-10-29T13:59:31Z

🤖 AI text below 🤖

I pushed a few commits to resolve the remaining issues and the conflict with main:

canonical_path / canonical_object_path are now used only as the duplicate key in graphed_write; _file_tree keeps the paths as given. Rewriting the object path to /<name>;1 made known_base_form= read cycle 1 (or fail) for a tree at a later cycle, and turned None into a fixed name instead of "the file's only TTree". Resolving symlinks and remote URLs also changed what workers open.
Removed _macos_real_casing; the inode check already catches case differences.
Merged main. Since graphed(name=...) can now set the source name, the key for a None object path uses the tree name.
Added regression tests for all of the above.

…its file POSIX normalization of a remote URL merged distinct objects: memory:// or an object store can hold different files at "a//b" and "a/b", and _resolve_entries then read the first for both. Remote keys are now fsspec's own normal form, unchanged when fsspec cannot parse them. Without opening the file, a None object path can only be the file's only TTree, so graphed_write refuses it beside any other input of that file, whichever is listed first. Assisted-by: Claude Code:claude-opus-5-5

ariostas

Thank you @sumitsaraswat362!

sumitsaraswat362 · 2026-10-29T15:16:08Z

Thanks a lot for the cleanup and getting this merged!

fix: canonicalize input file paths for graphed duplicate refusal (sci…

8927bb4

…kit-hep#1767)

github-actions Bot added the type/fix PR title type: fix (set automatically) label Oct 25, 2026

sumitsaraswat362 and others added 6 commits October 25, 2026 14:36

test: add coverage for canonical_path URL variants

9b15495

test: compare canonical paths dynamically for fsspec backends

d4997b2

style: pre-commit fixes

f4182e5

fix: normalize Windows file URLs and cover all canonical_path branches

c9ae751

fix: clean canonical_path URL handling and use valid file URIs in tests

d0b3bb6

test: remove invalid localhost path concatenation on Windows

a0214f4

ariostas reviewed Oct 27, 2026

View reviewed changes

fix: normalize object paths and canonicalize file_tree once

5916688

sumitsaraswat362 and others added 5 commits October 28, 2026 12:04

fix: handle Windows drive in file URLs and robust remote URL test com…

1d65968

…parison

test: remove redundant hardcoded remote URL assertion without port

23396fe

Merge remote-tracking branch 'origin/main' into pr1777-fix

282ce5b

fix: key a None object path on the tree name, not the source name

9e1fc09

graphed's name= now sets the session source name, so the duplicate key for a None object path uses the first entry's tree name instead. Assisted-by: Claude Code:claude-opus-5-5

ariostas approved these changes Oct 29, 2026

View reviewed changes

ariostas enabled auto-merge (squash) October 29, 2026 14:11

ariostas merged commit 73e024b into scikit-hep:main Oct 29, 2026
20 of 21 checks passed

Conversation

sumitsaraswat362 commented Oct 25, 2026

Summary

Changes

Uh oh!

codecov Bot commented Oct 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ariostas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sumitsaraswat362 commented Oct 28, 2026

Uh oh!

ariostas commented Oct 29, 2026

Uh oh!

ariostas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sumitsaraswat362 commented Oct 29, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Oct 25, 2026 •

edited

Loading