support universal paths - #645
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds universal-pathlib (UPath) support and comprehensive remote-file handling: new path utilities and runtime config, remote-materialization and caching with HTTP/S3/memory fallbacks, HDF5 reader/writer adapters for remote UPath, widespread IO API/type updates to accept UPath, and extensive test/fixture additions for remote IO. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7be9612c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+857
to
+859
| path = el if isinstance(el, UPath) else Path(el) | ||
| if path.exists(): | ||
| generator = _iter_filesystem( |
There was a problem hiding this comment.
When dc.scan(...) receives a remote directory as a plain string (for example "http://.../das" or "memory://..."), this branch wraps it with Path(...), so path.exists() checks the local filesystem and returns false. That skips _iter_filesystem recursion entirely and yields the raw string instead, which means remote directory scans via string inputs silently miss traversal unless callers manually construct a UPath first.
Useful? React with 👍 / 👎.
Comment on lines
+516
to
+519
| if isinstance(obj, str | Path | UPath): | ||
| path = obj if isinstance(obj, UPath) else Path(obj) | ||
| if path.exists(): | ||
| out = "directory" if path.is_dir() else "file" |
There was a problem hiding this comment.
This logic treats every str as a local Path, so remote URL strings are almost always classified as "file" because Path("http://...").exists() is false. As a result, directory-only FiberIO handlers are never considered for string-based remote directory inputs, even though remote path support was added elsewhere; callers must pass UPath objects to get the expected behavior.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_io/test_common_io.py (1)
132-151:⚠️ Potential issue | 🟡 MinorOnly skip real timeouts in these fetch fixtures.
Lines 139 and 150 now rely on the shared
skip_timeout()helper, and that helper currently skips on anyurllib.error.URLError. That means a bad registry entry, 404, or other transport failure will quietly skip the session fixture instead of failing the matrix and surfacing the regression.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_io/test_common_io.py` around lines 132 - 151, The fixtures io_path_tuple and data_file_path currently use skip_timeout() which swallows any urllib.error.URLError (hiding 404s/bad registry entries); change these fixtures to only skip on actual network timeouts by replacing the generic skip_timeout() usage with a context that only catches timeout-related errors (e.g., socket.timeout, TimeoutError, or urllib.error.URLError where the underlying reason is a timeout) around the fetch(fetch_name) and fetch(request.param) calls; keep the rest of the fixture logic (io_path_tuple, data_file_path, fetch, SKIP_DATA_FILES, get_registry_df) intact so non-timeout transport errors raise and fail the test matrix.
🧹 Nitpick comments (5)
pyproject.toml (1)
88-91: Redundantuniversal-pathlibin test dependencies.
universal-pathlibis already listed in the maindependencies(line 58), so it doesn't need to be repeated intestextras—main dependencies are always available when test extras are installed.♻️ Proposed fix
test = [ "aiohttp", "coverage>=7.4,<8", "pytest-cov>=4", "pre-commit", "pytest", "pytest-codeblocks", "pytest-cov", "starlette", "twine", - "universal-pathlib", "uvicorn", ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` around lines 88 - 91, Remove the redundant "universal-pathlib" entry from the test extras list in the pyproject.toml: update the [project.optional-dependencies] "test" array to delete "universal-pathlib" since it is already declared in the main [project] dependencies; ensure only one canonical declaration remains (keep the existing main dependency and remove the duplicate from the "test" extras).dascore/io/indexer.py (1)
127-130: Clarify type safety:self.pathcan beUPathbutwrite_updateexpectsstr | Path.After
requires_local_directoryvalidates the path is local,self.pathremains aUPathinstance if the input wasUPath, or aPathif the input wasPath. However,write_updateat line 348 is called withbase_path=self.path, and the signature declaresbase_path: str | Path(line 295 ofdascore/utils/hdf5.py), which does not includeUPath.While the code works in practice because
_remove_base_pathconverts the parameter usingstr(base), the type annotation mismatch indicates a gap in the API contract. For type safety and clarity, consider normalizingself.pathtoPathafter locality validation:♻️ Suggested change
self.path = ( UPath(path).absolute() if isinstance(path, UPath) else Path(path).absolute() ) requires_local_directory(self.path, label="DirectoryIndexer") + # Normalize to Path for downstream compatibility + self.path = Path(self.path) self.index_path = Path(self._find_index_file(self.path, index_path))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dascore/io/indexer.py` around lines 127 - 130, Normalize self.path to a stdlib Path after locality validation: ensure the constructor (where self.path is set and requires_local_directory(self.path, label="DirectoryIndexer") is called) always assigns a pathlib.Path instance (e.g., Path(str(path)).absolute() or Path(path).absolute()) instead of leaving a UPath, so that later calls like write_update(base_path=self.path) match the annotated base_path: str | Path in dascore/utils/hdf5.py; you can still call requires_local_directory(self.path) first but then overwrite self.path with a Path conversion to guarantee type-safety and consistency with _remove_base_path and write_update.dascore/clients/dirspool.py (1)
47-65: AddUPathto the public signature too.Line 64 now accepts
UPathat runtime, butbase_pathis still annotated asstr | Path | Self | AbstractIndexer. That leaves type checkers and generated docs out of sync with the API you just expanded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dascore/clients/dirspool.py` around lines 47 - 65, Update the __init__ signature so the base_path parameter includes UPath in its type annotation (e.g., change base_path: str | Path | Self | AbstractIndexer to base_path: str | Path | UPath | Self | AbstractIndexer), and ensure UPath is imported at the top of the module; keep the existing runtime isinstance check (Path | str | UPath) as-is so the behavior matches the public API.dascore/core/spool.py (1)
749-752: Handle URL strings as remote paths before coercing toPath.Right now, URL-like strings still get coerced via
Path(...), so remote string inputs can’t participate in the new UPath flow. Consider preserving scheme-based strings asUPathtoo.Suggested diff
def _spool_from_str(path, **kwargs): """Get a spool from a path.""" - path = path if isinstance(path, UPath) else Path(path) + if isinstance(path, UPath): + pass + elif isinstance(path, str) and "://" in path: + path = UPath(path) + else: + path = Path(path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dascore/core/spool.py` around lines 749 - 752, In _spool_from_str, avoid coercing URL-like strings with Path(...) — first detect if path is already a UPath, otherwise if it's a str and contains a URI scheme (e.g., startswith something like "<scheme>://" or matches regex r"^[a-zA-Z][a-zA-Z0-9+.-]*://"), construct a UPath(path) and leave it as such; only call Path(path) for non-URL strings. Update the initial lines in _spool_from_str to: if isinstance(path, UPath) -> keep, elif isinstance(path, str) and looks like URL -> path = UPath(path), else -> path = Path(path), so remote URL strings participate in the UPath flow.dascore/io/core.py (1)
466-478: Consider simplifying the path existence check.The branching logic could be simplified since
coerce_pathalready handlesUPathconversion. The current code has a redundant path throughis_local_pathcheck.♻️ Suggested simplification
- if isinstance(path, UPath): - exists = path.exists() - suffix = path.suffix - else: - local_path = ( - coerce_path(path) if not is_local_path(path) else Path(path) - ) - exists = local_path.exists() - suffix = local_path.suffix + coerced = coerce_path(path) if not isinstance(path, UPath) else path + exists = coerced.exists() + suffix = coerced.suffix🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dascore/io/core.py` around lines 466 - 478, The existence/suffix branching is redundant; always coerce the incoming path to a local Path using coerce_path(path) (which already handles UPath and local paths), then use local_path.exists() and local_path.suffix to set exists and suffix; keep the FileNotFoundError raise and the ext = suffix[1:] if suffix else None logic but remove the is_local_path branch and the isinstance(path, UPath) special-case so only local_path = coerce_path(path) followed by exists/suffix checks remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dascore/utils/paths.py`:
- Around line 33-36: is_local_path currently treats non-pathlike inputs as local
because get_path_protocol(resource) can return None and None is in the accepted
set; change is_local_path to first verify the input is pathlike (e.g.,
isinstance(resource, (str, os.PathLike))) or explicitly treat a None protocol as
non-local, then call get_path_protocol; update the function is_local_path to
return False for non-pathlike resources or when get_path_protocol(...) is None,
while still recognizing protocols "", "file", and "local" as local.
In `@tests/test_io/conftest.py`:
- Around line 113-119: The readiness probe loop using urlopen(probe_url) should
be hardened: import URLError from urllib.error and call urlopen with a timeout
(e.g., urlopen(probe_url, timeout=5)), catch only URLError and OSError instead
of Exception, and add an else: on the for loop that calls pytest.fail("server
did not start: readiness probe failed") so the fixture fails immediately if all
retries exhaust; update references to urlopen, probe_url, URLError, OSError, and
pytest.fail accordingly.
In `@tests/test_utils/test_paths.py`:
- Around line 10-11: The long single-line import of multiple symbols from
dascore.utils.paths is triggering E501; split the import across lines using
either implicit line continuation with parentheses or separate import statements
so each line is under the line-length limit—target the import that brings in
coerce_path, get_path_protocol, is_local_path, is_pathlike, and
requires_local_directory in tests/test_utils/test_paths.py and wrap those symbol
names onto multiple lines to satisfy the linter.
---
Outside diff comments:
In `@tests/test_io/test_common_io.py`:
- Around line 132-151: The fixtures io_path_tuple and data_file_path currently
use skip_timeout() which swallows any urllib.error.URLError (hiding 404s/bad
registry entries); change these fixtures to only skip on actual network timeouts
by replacing the generic skip_timeout() usage with a context that only catches
timeout-related errors (e.g., socket.timeout, TimeoutError, or
urllib.error.URLError where the underlying reason is a timeout) around the
fetch(fetch_name) and fetch(request.param) calls; keep the rest of the fixture
logic (io_path_tuple, data_file_path, fetch, SKIP_DATA_FILES, get_registry_df)
intact so non-timeout transport errors raise and fail the test matrix.
---
Nitpick comments:
In `@dascore/clients/dirspool.py`:
- Around line 47-65: Update the __init__ signature so the base_path parameter
includes UPath in its type annotation (e.g., change base_path: str | Path | Self
| AbstractIndexer to base_path: str | Path | UPath | Self | AbstractIndexer),
and ensure UPath is imported at the top of the module; keep the existing runtime
isinstance check (Path | str | UPath) as-is so the behavior matches the public
API.
In `@dascore/core/spool.py`:
- Around line 749-752: In _spool_from_str, avoid coercing URL-like strings with
Path(...) — first detect if path is already a UPath, otherwise if it's a str and
contains a URI scheme (e.g., startswith something like "<scheme>://" or matches
regex r"^[a-zA-Z][a-zA-Z0-9+.-]*://"), construct a UPath(path) and leave it as
such; only call Path(path) for non-URL strings. Update the initial lines in
_spool_from_str to: if isinstance(path, UPath) -> keep, elif isinstance(path,
str) and looks like URL -> path = UPath(path), else -> path = Path(path), so
remote URL strings participate in the UPath flow.
In `@dascore/io/core.py`:
- Around line 466-478: The existence/suffix branching is redundant; always
coerce the incoming path to a local Path using coerce_path(path) (which already
handles UPath and local paths), then use local_path.exists() and
local_path.suffix to set exists and suffix; keep the FileNotFoundError raise and
the ext = suffix[1:] if suffix else None logic but remove the is_local_path
branch and the isinstance(path, UPath) special-case so only local_path =
coerce_path(path) followed by exists/suffix checks remain.
In `@dascore/io/indexer.py`:
- Around line 127-130: Normalize self.path to a stdlib Path after locality
validation: ensure the constructor (where self.path is set and
requires_local_directory(self.path, label="DirectoryIndexer") is called) always
assigns a pathlib.Path instance (e.g., Path(str(path)).absolute() or
Path(path).absolute()) instead of leaving a UPath, so that later calls like
write_update(base_path=self.path) match the annotated base_path: str | Path in
dascore/utils/hdf5.py; you can still call requires_local_directory(self.path)
first but then overwrite self.path with a Path conversion to guarantee
type-safety and consistency with _remove_base_path and write_update.
In `@pyproject.toml`:
- Around line 88-91: Remove the redundant "universal-pathlib" entry from the
test extras list in the pyproject.toml: update the
[project.optional-dependencies] "test" array to delete "universal-pathlib" since
it is already declared in the main [project] dependencies; ensure only one
canonical declaration remains (keep the existing main dependency and remove the
duplicate from the "test" extras).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ccf6065f-e0d0-4c61-99a7-0a894968d270
📒 Files selected for processing (31)
dascore/clients/dirspool.pydascore/clients/filespool.pydascore/compat.pydascore/constants.pydascore/core/spool.pydascore/io/core.pydascore/io/dasdae/core.pydascore/io/h5simple/core.pydascore/io/indexer.pydascore/io/segy/core.pydascore/io/sentek/core.pydascore/io/sintela_binary/core.pydascore/io/tdms/core.pydascore/utils/hdf5.pydascore/utils/io.pydascore/utils/misc.pydascore/utils/paths.pydascore/utils/remote_io.pypyproject.tomltests/test_clients/test_filespool.pytests/test_io/_common_io_test_utils.pytests/test_io/conftest.pytests/test_io/test_common_io.pytests/test_io/test_indexer.pytests/test_io/test_io_core.pytests/test_io/test_remote_common_io.pytests/test_io/test_remote_http.pytests/test_io/test_remote_memory.pytests/test_utils/test_io_utils.pytests/test_utils/test_misc.pytests/test_utils/test_paths.py
d-chambers
force-pushed
the
universal-pathlib
branch
from
March 31, 2026 15:19
b7be961 to
a0fab7e
Compare
coderabbitai
Bot
added
the
documentation
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
dascore/utils/misc.py (1)
198-236:⚠️ Potential issue | 🟠 MajorWarn-once state is still reset during recursion.
warned_no_remote_mtimelives inside_iter_generic(), so nested remote walks can warn once per directory instead of once per traversal. Theexcept Exceptionpath also downgrades real auth/network/backend failures to an unfiltered scan. Please hoist the flag to_iter_filesystem()scope and only degrade on the specific “mtime unsupported” failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dascore/utils/misc.py` around lines 198 - 236, Move the warned_no_remote_mtime flag out of _iter_generic into the enclosing _iter_filesystem scope so a single traversal shares the flag; update _meets_timestamp to reference that outer warned_no_remote_mtime. Replace the broad except Exception in _meets_timestamp with a narrow catch for the specific failure(s) that indicate the backend doesn't support mtime (e.g., AttributeError and NotImplementedError or the backend-specific exception your path objects raise) so real auth/network errors still propagate; only on these mtime-unsupported exceptions set warned_no_remote_mtime once and return True to degrade to unfiltered iteration.
🧹 Nitpick comments (6)
tests/test_io/test_wav/test_wav.py (1)
56-57: Consider uniquememory://prefixes per test for stronger isolation.Using fixed in-memory paths can create avoidable coupling across repeated runs/reordered tests.
♻️ Optional test-isolation tweak
+from uuid import uuid4 ... - path = UPath("memory://dascore/temp.wav") + path = UPath(f"memory://dascore/{uuid4().hex}/temp.wav") ... - path = UPath("memory://dascore/wav_dir") + path = UPath(f"memory://dascore/{uuid4().hex}/wav_dir")Also applies to: 87-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_io/test_wav/test_wav.py` around lines 56 - 57, Replace the fixed in-memory URI with a unique one per test to avoid cross-test coupling: when constructing the UPath for writing (currently UPath("memory://dascore/temp.wav") used with dc.write(audio_patch, path, "wav")), generate a unique suffix (e.g., via uuid.uuid4() or pytest's tmp_path.name) and build the URI like f"memory://dascore/{unique_id}.wav" so each test gets its own memory:// path; apply the same change to the other occurrences around the dc.write calls.tests/test_io/test_h5simple/test_h5simple.py (1)
54-60: Replacegetattrwith direct attribute access.Static analysis correctly flags that
getattr(attrs, "dims")with a constant string offers no benefit overattrs.dims.♻️ Suggested fix
def test_get_root_attrs_supports_pytables(self, tmp_path): """PyTables handles should expose root attrs through the helper.""" path = tmp_path / "root_attrs.h5" with tables.open_file(path, "w") as h5: h5.root._v_attrs["dims"] = "distance,time" attrs = _get_root_attrs(h5) - assert getattr(attrs, "dims") == "distance,time" + assert attrs.dims == "distance,time"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_io/test_h5simple/test_h5simple.py` around lines 54 - 60, In test_get_root_attrs_supports_pytables, replace the redundant getattr call with direct attribute access: after calling _get_root_attrs(h5) assert the dims using attrs.dims instead of getattr(attrs, "dims"); update the assertion in test_get_root_attrs_supports_pytables to use attrs.dims for clarity and to satisfy static analysis.dascore/io/indexer.py (1)
123-127: Redundant.absolute()call.
self.pathis already absolute after Line 124-125. The second.absolute()on Line 127 is unnecessary.♻️ Suggested simplification
self.path = ( UPath(path).absolute() if isinstance(path, UPath) else Path(path).absolute() ) requires_local_directory(self.path, label="DirectoryIndexer") - self.path = Path(self.path).absolute() + self.path = Path(self.path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dascore/io/indexer.py` around lines 123 - 127, The second .absolute() call is redundant because self.path is already made absolute when set from UPath or Path; update the DirectoryIndexer initialization by removing the extra reassignment that calls .absolute() (i.e., delete the final "self.path = Path(self.path).absolute()" line) and ensure requires_local_directory(self.path, label="DirectoryIndexer") still runs after the initial absolute conversion so self.path remains a Path-like absolute value; references: self.path, UPath, Path, requires_local_directory, DirectoryIndexer.tests/test_io/test_dasdae/test_dasdae.py (1)
799-802: Use a raw string for the regex pattern inpytest.raises.The
|character is a regex metacharacter (alternation). While this likely works as intended, using a raw string makes the regex intent explicit and silences the static analysis warning.Suggested fix
- with pytest.raises(TypeError, match="Object dtype|object arrays"): + with pytest.raises(TypeError, match=r"Object dtype|object arrays"):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_io/test_dasdae/test_dasdae.py` around lines 799 - 802, Update the pytest.raises call to use a raw string for the regex pattern: change the pattern argument in the pytest.raises(...) that wraps the _save_array call (the one passing "obj", group=group, h5=h5) to a raw string literal (e.g., r"Object dtype|object arrays") so the alternation '|' is treated as a regex without triggering static analysis warnings.dascore/utils/io.py (1)
81-94: Silent exception swallowing in annotation helper warrants consideration.The static analysis correctly flags
try-except-passwith blindException. While the# pragma: no coversuggests this is defensive/best-effort code, silently swallowing exceptions can hide unexpected failures. Consider at least a debug-level log for traceability, or narrowing to specific exception types (e.g.,AttributeError,TypeError).♻️ Optional: Add narrow exception types or debug logging
- try: - setattr(handle, attr_name, path_str) - except Exception: - pass + try: + setattr(handle, attr_name, path_str) + except (AttributeError, TypeError): + pass # Read-only or frozen handle; annotation is best-effort🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dascore/utils/io.py` around lines 81 - 94, The helper _annotate_handle_path currently swallows all exceptions with bare excepts; change the handlers to either catch only specific exceptions (e.g., AttributeError, TypeError) when calling setattr/getattr on handle, and/or emit a debug-level log with the exception info instead of pass to preserve traceability; update the two try/except blocks around setattr(handle, attr_name, path_str) and setattr(handle, "name", path_str) to use narrow exception types and call the module logger (or logging.getLogger(__name__)) at debug level with the caught exception and a short context message.dascore/utils/remote_io.py (1)
20-46: Module-level mutable state may cause issues in concurrent scenarios.
_REMOTE_RESOURCE_CACHEis a module-level dict that's mutated innormalize_remote_id()and cleared inclear_remote_file_cache(). In multi-threaded or multiprocessing scenarios (common for data processing), this could lead to race conditions. Consider if thread-safe alternatives are needed, or document the single-threaded assumption.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dascore/utils/remote_io.py` around lines 20 - 46, The module-level dict _REMOTE_RESOURCE_CACHE is mutated in normalize_remote_id() and cleared in clear_remote_file_cache(), which is unsafe for concurrent access; make access thread-safe by introducing a module-level lock (e.g., threading.Lock or RLock) and acquiring it around any reads/writes to _REMOTE_RESOURCE_CACHE inside normalize_remote_id() and clear_remote_file_cache(), or alternatively replace the mutable global with a thread-safe mapping (e.g., collections.defaultdict with locking or concurrent.futures-safe structure) and update code paths that reference _REMOTE_RESOURCE_CACHE accordingly to use the locked access pattern; ensure the lock variable and the cache variable names remain unique and referenced in normalize_remote_id and clear_remote_file_cache.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dascore/io/dasdae/utils.py`:
- Around line 413-428: _decode_legacy_attr_value currently calls
pickle.loads(raw, encoding="latin1") which allows arbitrary code execution when
deserializing untrusted HDF5 attributes; update this function to avoid insecure
deserialization by either (a) documenting the trust assumption clearly at the
top of the function and gating pickle usage behind an explicit "trusted" flag or
environment check, or (b) replacing pickle.loads with a safer parser flow that
first attempts ast.literal_eval for simple Python literals and only falls back
to pickle for known-trusted inputs; ensure you reference the existing symbols
pickle.loads, _decode_legacy_attr_value, and the fallback path to unbyte so the
change keeps the same fallback behavior for unparseable data.
In `@dascore/io/xml_binary/utils.py`:
- Around line 241-243: The timestamp post-filter using x.stat() can raise on
remote backends; update the code in the timestamp block (where timestamp,
to_float, and paths are used) to perform a guarded stat access instead of
calling x.stat() unguarded—e.g., compute ts = to_float(timestamp) and rebuild
paths with a try/except around x.stat() (catching OSError/Exception) and only
include entries whose stat().st_mtime >= ts, or skip entries where stat is
unavailable; alternatively remove this local filter and rely on the upstream
_iter_filesystem() timestamp filtering so you don't call x.stat() here at all.
- Around line 187-190: The current branch uses path.read_bytes() (with UPath and
not is_local_path(path)) which loads the whole remote file into memory; instead,
materialize or cache the remote file locally and then open it with np.memmap so
slices stay on-disk. Replace the np.frombuffer(path.read_bytes(), ...) approach
by using the project’s local-materialization/cache path routine for UPath (i.e.,
ensure the remote UPath is downloaded to a local file) and pass that local path
into np.memmap(metadata.data_type) so the code continues to use np.memmap for
both local and remote files; keep checks around isinstance(path, UPath) and
is_local_path(path) and only trigger materialization for non-local UPath
instances.
In `@dascore/utils/hdf5.py`:
- Around line 548-596: The __init__ of _RemoteH5Writer can leak the temporary
file if H5pyFile(...) fails; wrap the H5pyFile construction in a try/except or
try/finally so that on any exception you unlink self._temp_path (and set any
state like _closed if needed) before re-raising; specifically protect the call
to H5pyFile(self._temp_path, mode=local_mode) so failures clean up the temp file
created earlier.
In `@dascore/utils/misc.py`:
- Around line 795-803: The current get_buffer_size logic assumes getattr(fid,
"name") points to a real local file and calls Path(path).stat().st_size which
can raise for non-files (e.g., BytesIO that exposes name); wrap the
Path(path).stat().st_size probe in a try/except and if stat() fails (catch
OSError/ValueError/TypeError), fall back to the existing buffer-based approach
using fid.tell()/seek()/tell() to compute file_size; keep the
is_local_path(path) check but ensure exceptions are handled so get_buffer_size
continues to work for wrapped/in-memory streams (refer to variables/functions:
fid, path, is_local_path, get_buffer_size).
In `@tests/test_io/conftest.py`:
- Around line 34-39: The copyfile method currently catches BrokenPipeError and
ConnectionResetError but misses ConnectionAbortedError (seen on Windows as
WinError 10053); update the exception tuple in copyfile to also include
ConnectionAbortedError so the method safely silences Windows connection-abort
scenarios when calling super().copyfile(source, outputfile) in
tests/test_io/conftest.py.
In `@tests/test_utils/test_misc.py`:
- Around line 461-469: The test uses _spool_map(random_spool[:4], ...,
client=DummyClient()) which when os.cpu_count() is 2 will produce two spools of
two patches and therefore four mapped results, so update the assertions in
tests/test_utils/test_misc.py for the _spool_map case: assert that out contains
four items (e.g. assert out == [3, 3, 3, 3] or assert len(out) == 4 and all(v ==
3 for v in out)) and keep/adjust the seen assertion if necessary for DummyClient
logging; locate the checks around the call to _spool_map and replace the old [3,
3, 3] expectation accordingly.
---
Duplicate comments:
In `@dascore/utils/misc.py`:
- Around line 198-236: Move the warned_no_remote_mtime flag out of _iter_generic
into the enclosing _iter_filesystem scope so a single traversal shares the flag;
update _meets_timestamp to reference that outer warned_no_remote_mtime. Replace
the broad except Exception in _meets_timestamp with a narrow catch for the
specific failure(s) that indicate the backend doesn't support mtime (e.g.,
AttributeError and NotImplementedError or the backend-specific exception your
path objects raise) so real auth/network errors still propagate; only on these
mtime-unsupported exceptions set warned_no_remote_mtime once and return True to
degrade to unfiltered iteration.
---
Nitpick comments:
In `@dascore/io/indexer.py`:
- Around line 123-127: The second .absolute() call is redundant because
self.path is already made absolute when set from UPath or Path; update the
DirectoryIndexer initialization by removing the extra reassignment that calls
.absolute() (i.e., delete the final "self.path = Path(self.path).absolute()"
line) and ensure requires_local_directory(self.path, label="DirectoryIndexer")
still runs after the initial absolute conversion so self.path remains a
Path-like absolute value; references: self.path, UPath, Path,
requires_local_directory, DirectoryIndexer.
In `@dascore/utils/io.py`:
- Around line 81-94: The helper _annotate_handle_path currently swallows all
exceptions with bare excepts; change the handlers to either catch only specific
exceptions (e.g., AttributeError, TypeError) when calling setattr/getattr on
handle, and/or emit a debug-level log with the exception info instead of pass to
preserve traceability; update the two try/except blocks around setattr(handle,
attr_name, path_str) and setattr(handle, "name", path_str) to use narrow
exception types and call the module logger (or logging.getLogger(__name__)) at
debug level with the caught exception and a short context message.
In `@dascore/utils/remote_io.py`:
- Around line 20-46: The module-level dict _REMOTE_RESOURCE_CACHE is mutated in
normalize_remote_id() and cleared in clear_remote_file_cache(), which is unsafe
for concurrent access; make access thread-safe by introducing a module-level
lock (e.g., threading.Lock or RLock) and acquiring it around any reads/writes to
_REMOTE_RESOURCE_CACHE inside normalize_remote_id() and
clear_remote_file_cache(), or alternatively replace the mutable global with a
thread-safe mapping (e.g., collections.defaultdict with locking or
concurrent.futures-safe structure) and update code paths that reference
_REMOTE_RESOURCE_CACHE accordingly to use the locked access pattern; ensure the
lock variable and the cache variable names remain unique and referenced in
normalize_remote_id and clear_remote_file_cache.
In `@tests/test_io/test_dasdae/test_dasdae.py`:
- Around line 799-802: Update the pytest.raises call to use a raw string for the
regex pattern: change the pattern argument in the pytest.raises(...) that wraps
the _save_array call (the one passing "obj", group=group, h5=h5) to a raw string
literal (e.g., r"Object dtype|object arrays") so the alternation '|' is treated
as a regex without triggering static analysis warnings.
In `@tests/test_io/test_h5simple/test_h5simple.py`:
- Around line 54-60: In test_get_root_attrs_supports_pytables, replace the
redundant getattr call with direct attribute access: after calling
_get_root_attrs(h5) assert the dims using attrs.dims instead of getattr(attrs,
"dims"); update the assertion in test_get_root_attrs_supports_pytables to use
attrs.dims for clarity and to satisfy static analysis.
In `@tests/test_io/test_wav/test_wav.py`:
- Around line 56-57: Replace the fixed in-memory URI with a unique one per test
to avoid cross-test coupling: when constructing the UPath for writing (currently
UPath("memory://dascore/temp.wav") used with dc.write(audio_patch, path,
"wav")), generate a unique suffix (e.g., via uuid.uuid4() or pytest's
tmp_path.name) and build the URI like f"memory://dascore/{unique_id}.wav" so
each test gets its own memory:// path; apply the same change to the other
occurrences around the dc.write calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b3489a7b-28f7-490b-91e4-676338412f2d
📒 Files selected for processing (68)
.github/scripts/cache_test_data.pybenchmarks/readme.mdbenchmarks/test_io_benchmarks.pydascore/__init__.pydascore/clients/dirspool.pydascore/clients/filespool.pydascore/compat.pydascore/config.pydascore/constants.pydascore/core/spool.pydascore/core/summary.pydascore/io/__init__.pydascore/io/core.pydascore/io/dasdae/core.pydascore/io/dasdae/utils.pydascore/io/h5simple/core.pydascore/io/h5simple/utils.pydascore/io/indexer.pydascore/io/rsf/core.pydascore/io/segy/core.pydascore/io/sentek/core.pydascore/io/sentek/utils.pydascore/io/sintela_binary/core.pydascore/io/sintela_binary/utils.pydascore/io/tdms/core.pydascore/io/tdms/utils.pydascore/io/wav/core.pydascore/io/xml_binary/core.pydascore/io/xml_binary/utils.pydascore/utils/display.pydascore/utils/downloader.pydascore/utils/hdf5.pydascore/utils/io.pydascore/utils/misc.pydascore/utils/patch.pydascore/utils/paths.pydascore/utils/progress.pydascore/utils/remote_io.pydocs/changelog.qmddocs/tutorial/configuration.qmddocs/tutorial/file_io.qmddocs/tutorial/spool.qmdpyproject.tomlscripts/_templates/_quarto.ymltests/conftest.pytests/test_clients/test_filespool.pytests/test_io/_common_io_test_utils.pytests/test_io/conftest.pytests/test_io/test_common_io.pytests/test_io/test_dasdae/test_dasdae.pytests/test_io/test_h5simple/test_h5simple.pytests/test_io/test_indexer.pytests/test_io/test_io_core.pytests/test_io/test_remote_common_io.pytests/test_io/test_remote_http.pytests/test_io/test_remote_memory.pytests/test_io/test_rsf/test_rsf.pytests/test_io/test_tdms/test_tdms_utils.pytests/test_io/test_wav/test_wav.pytests/test_io/test_xml_binary/test_xml_binary.pytests/test_utils/test_config.pytests/test_utils/test_display.pytests/test_utils/test_downloader.pytests/test_utils/test_hdf_utils.pytests/test_utils/test_io_utils.pytests/test_utils/test_misc.pytests/test_utils/test_paths.pytests/test_utils/test_progress.py
💤 Files with no reviewable changes (2)
- dascore/init.py
- dascore/io/init.py
✅ Files skipped from review due to trivial changes (9)
- dascore/compat.py
- benchmarks/readme.md
- scripts/_templates/_quarto.yml
- docs/changelog.qmd
- docs/tutorial/file_io.qmd
- tests/test_utils/test_config.py
- dascore/io/tdms/core.py
- tests/test_utils/test_paths.py
- docs/tutorial/configuration.qmd
🚧 Files skipped from review as they are similar to previous changes (13)
- dascore/constants.py
- dascore/clients/dirspool.py
- dascore/io/sintela_binary/core.py
- dascore/io/h5simple/core.py
- tests/test_io/test_common_io.py
- dascore/core/spool.py
- tests/test_io/test_io_core.py
- dascore/io/segy/core.py
- dascore/io/dasdae/core.py
- dascore/io/sentek/core.py
- dascore/clients/filespool.py
- tests/test_io/test_remote_common_io.py
- tests/test_clients/test_filespool.py
d-chambers
force-pushed
the
universal-pathlib
branch
from
April 1, 2026 19:15
a0fab7e to
7c7d73d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (4)
tests/test_io/conftest.py (1)
34-39:⚠️ Potential issue | 🟡 MinorAlso swallow
ConnectionAbortedErrorhere.Windows can raise
ConnectionAbortedErrorfromsuper().copyfile(...), so this still leaves the localhost HTTP fixtures prone to noisy disconnect failures.💡 Proposed fix
def copyfile(self, source, outputfile): """Silence BrokenPipeError/ConnectionResetError from test clients.""" try: return super().copyfile(source, outputfile) - except (BrokenPipeError, ConnectionResetError): + except (BrokenPipeError, ConnectionResetError, ConnectionAbortedError): return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_io/conftest.py` around lines 34 - 39, The except block in copyfile (method copyfile) currently catches BrokenPipeError and ConnectionResetError but misses ConnectionAbortedError (raised on Windows), causing noisy disconnect failures; update the except to include ConnectionAbortedError in the exception tuple handled for super().copyfile(source, outputfile) so that BrokenPipeError, ConnectionResetError, and ConnectionAbortedError are all swallowed and the method returns None on those disconnects.dascore/utils/misc.py (1)
832-833:⚠️ Potential issue | 🟠 MajorKeep the local
stat()probe fallible.Some wrapped or in-memory streams still expose
.namefor display only. Lines 832-833 will raise before the existingtell()/seek()fallback, which breaks callers likedascore/io/tdms/utils.py:180-189anddascore/io/sintela_binary/utils.py:111-115when they pass non-file-backed buffers.💡 Proposed fix
path = getattr(fid, "name", None) if path is not None and is_local_path(path): - file_size = Path(path).stat().st_size - else: - cur = fid.tell() - fid.seek(0, 2) # end - file_size = fid.tell() - fid.seek(cur, 0) + try: + return Path(path).stat().st_size + except (OSError, TypeError, ValueError): + pass + cur = fid.tell() + fid.seek(0, 2) # end + file_size = fid.tell() + fid.seek(cur, 0) return file_size🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dascore/utils/misc.py` around lines 832 - 833, The direct call to Path(path).stat().st_size when path is not None and is_local_path(path) can raise for non-file-backed streams; wrap the stat probe in a try/except (catching OSError/Exception) and only set file_size on success, otherwise leave file_size unset so the existing tell()/seek() fallback runs; refer to the is_local_path check, the file_size variable assignment, and the existing tell()/seek() fallback code to implement the guarded stat call.dascore/io/dasdae/utils.py (1)
413-423:⚠️ Potential issue | 🟠 MajorAvoid deserializing file-controlled attrs with
pickle.This fallback still runs
pickle.loadson bytes read from the HDF5 file. If untrusted DASDAE inputs can reach this path, that keeps arbitrary code execution in the read flow. Please gate it behind an explicit trusted-source path or replace it with a safer decoder.dascore/utils/hdf5.py (1)
551-566:⚠️ Potential issue | 🟡 MinorClean up
_temp_pathif_RemoteH5Writerinitialization fails.Lines 554-566 allocate a temp file and then do remote I/O plus
H5pyFile(...)construction without a cleanup guard. Any exception in that window leaves an orphaned temp file behind.🛠️ Proposed fix
def __init__(self, resource: UPath, mode: str): # pragma: no cover self._resource = resource suffix = resource.suffix or ".h5" fd, temp_name = tempfile.mkstemp(suffix=suffix) os.close(fd) self._temp_path = Path(temp_name) self._closed = False - if mode != "w" and resource.exists(): - with resource.open("rb") as src, self._temp_path.open("wb") as dst: - shutil.copyfileobj(src, dst) - local_mode = ( - "a" - if self._temp_path.exists() and self._temp_path.stat().st_size - else "w" - ) - self._handle = H5pyFile(self._temp_path, mode=local_mode) + try: + if mode != "w" and resource.exists(): + with resource.open("rb") as src, self._temp_path.open("wb") as dst: + shutil.copyfileobj(src, dst) + local_mode = ( + "a" + if self._temp_path.exists() and self._temp_path.stat().st_size + else "w" + ) + self._handle = H5pyFile(self._temp_path, mode=local_mode) + except Exception: + self._temp_path.unlink(missing_ok=True) + raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dascore/utils/hdf5.py` around lines 551 - 566, The __init__ of _RemoteH5Writer can leave an orphaned temp file if any exception occurs between creating self._temp_path and successfully constructing self._handle; wrap the block that does the remote copy and H5pyFile(self._temp_path, ...) construction in a try/except (or try/finally) so that on any exception you unlink/delete self._temp_path (and set any partial state like self._closed) before re-raising the exception; specifically protect the region that uses resource.open(...), shutil.copyfileobj(...), and H5pyFile(...) and ensure cleanup of self._temp_path when initialization fails.
🧹 Nitpick comments (1)
tests/test_io/test_h5simple/test_h5simple.py (1)
78-84: Add explicit h5py-path tests for the other two helper branches.This class now validates h5py for
_get_root_attrs, but_iter_root_arraysand_get_attr_namesalso have separate h5py branches indascore/io/h5simple/utils.py(Line 28-Line 44). Adding direct h5py tests for those would close backend parity and guard regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_io/test_h5simple/test_h5simple.py` around lines 78 - 84, Add two new unit tests mirroring test_get_root_attrs_supports_h5py that explicitly exercise the h5py-specific branches for _iter_root_arrays and _get_attr_names: open an h5py.File in write mode, create root datasets/arrays (for _iter_root_arrays) and several root attributes (for _get_attr_names), call the corresponding helper (_iter_root_arrays and _get_attr_names) with the h5py.File object, and assert the returned iterator/list contains the expected dataset names and attribute names/values; ensure tests use the same tmp_path pattern and names as the existing test to keep them isolated and deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dascore/io/core.py`:
- Around line 711-725: The current broad try/except around path.stat() masks
local filesystem errors and treats them as "updated"; change the logic so only
remote backends get the fallback: if is_local_path(resource) call
Path(resource).stat() directly and let exceptions propagate (so
deleted/inaccessible local paths raise), otherwise use
coerce_to_upath(resource).stat() inside a try/except that catches remote/backend
errors and issues the warnings.warn (as currently) and returns True; reference
coerce_to_upath, is_local_path, Path(...).stat().st_mtime and timestamp to
locate the code to adjust.
In `@dascore/io/dasdae/utils.py`:
- Around line 359-367: PatchSummary.shape is being set to an empty tuple instead
of the actual dataset shape; update the code that builds the PatchSummary
(around group.get("data"), dtype, and PatchSummary(...)) to set shape to the
stored data's shape when data exists — e.g., compute shape =
tuple(data_node.shape) if data_node is not None else () and pass that into
PatchSummary(shape=shape) so summaries reflect the real patch dimensions.
- Around line 344-358: When building dims and coord_dims, treat an empty stored
string as an empty tuple instead of splitting to ('',) — change the logic around
out["dims"] and the unbyte(attrs.get(f"_cdims_{name}", "")) parsing so that if
the string is "" you set dims = () (and similarly coord_dims = ()) otherwise
split into a tuple; update the code paths that use dims and coord_dims (the
tuple assigned to dims before calling separate_coord_info and the coord_dims
passed into _get_coord_summary_from_node) so empty stored dims produce no bogus
empty-string dimension.
In `@dascore/utils/downloader.py`:
- Around line 100-107: _fetch_cached currently places kwargs_tuple (including
original values) into the cached function signature, which fails for unhashable
kwarg values; fix by building a hashable representation for the cache key while
still passing the original dict to the fetcher: inside _fetch_cached convert
kwargs_tuple -> kwargs dict for the call to _get_fetcher(cache_dir).fetch(name,
**kwargs) but derive a separate cache-key component from kwargs using a stable,
hashable transformation (e.g., tuple(sorted((k, repr(v)) for k,v in
kwargs.items())) or frozenset of those pairs) so callers' unhashable values are
not required to be hashable; apply the same change to the other cached wrapper
that also accepts kwargs_tuple.
In `@dascore/utils/hdf5.py`:
- Around line 577-592: The context manager currently always calls close() in
__exit__, causing a remote upload even when the with-block raised; change
__exit__ to only perform the remote upload when no exception occurred (i.e., if
exc_type is None) and otherwise avoid calling close() (instead just close the
local HDF5 handle and remove the temp file to abort the upload). Use the
existing symbols: in __exit__ inspect exc_type, exc, tb; on exception call
self._handle.close() and self._temp_path.unlink(missing_ok=True) (do not call
self.close()), and when exc_type is None call self.close(); ensure you still
return False so original exceptions propagate and any upload failures raise
instead of being suppressed.
In `@dascore/utils/misc.py`:
- Around line 251-252: The local-file fallback currently yields the provided
paths unconditionally in the except NotADirectoryError block, skipping the ext,
timestamp and hidden-name filters that are applied for remote inputs; update the
except block in the same function (the generator used by
DirectoryIndexer.update(paths=...)) to pass the single-file input through the
same filtering logic used for the directory/remote branch (i.e. reuse the
existing filter routine or the same checks for ext, timestamp and hidden-name)
and only yield the path if it passes those filters so local files and remote
files are treated consistently.
In `@dascore/utils/remote_io.py`:
- Around line 81-100: The _warn_remote_cache_download function currently
interpolates the raw resource into user-facing messages which can leak secrets
(e.g., presigned URLs); before composing msg, pass resource through the
project's redaction helper (e.g., sanitize_remote_resource or similar) and use
the sanitized value in the f-strings, keeping the same guidance text and
warnings.warn(stacklevel=4). Do the same sanitization for the other
warning/error messages that interpolate a resource (the other remote-cache
warning/error helpers referenced in the review) so all user-facing logs and
exceptions use the redacted identifier instead of the raw resource.
- Around line 111-127: _download_remote_file currently uses a deterministic
tmp_path (local_path.with_suffix(f"{local_path.suffix}.part")) which causes
races when multiple processes/threads download the same resource; change
tmp_path creation to use a unique per-attempt temporary filename (e.g., append a
short random/uuid or use tempfile.NamedTemporaryFile semantics in the same
directory) so each concurrent downloader writes to its own temp file, keep the
existing replace() to atomically move the successful temp into local_path, and
still unlink the specific temp file in the finally block; update references to
tmp_path in the function accordingly.
In `@tests/test_io/test_dasdae/test_dasdae.py`:
- Around line 52-56: The helper written_dascore_v1_random_indexed currently just
copies the file; after copying (new_path) call index(new_path) so the file is
actually indexed before returning it; update the function to import/resolve and
invoke the index() routine used by the dasdae codebase (keep the same new_path
return) so the downstream tests exercise post-index append/scan behavior.
In `@tests/test_io/test_h5simple/test_h5simple.py`:
- Line 60: Replace the getattr usage in the test assertion to use direct
attribute access: locate the assertion that reads like assert getattr(attrs,
"dims") == "distance,time" in test_h5simple.py and change it to use attrs.dims
instead (i.e., assert attrs.dims == "distance,time") to satisfy the Ruff B009
rule and improve clarity.
In `@tests/test_io/test_remote_http.py`:
- Around line 19-29: The autouse fixture suppresses all UserWarning instances,
hiding unrelated regressions; update the suppress_expected_remote_cache_warnings
fixture to only ignore the exact expected warning text(s) instead of all
UserWarning—use warnings.filterwarnings("ignore", message=RE,
category=UserWarning) or your suppress_warnings helper with a message/match
regex that targets the remote-cache/fallback/mtime warning(s) you expect (and
keep the special-case bypass for
"test_http_hdf5_fallback_warns_once_and_reuses_cached_local_copy"); reference
the fixture name suppress_expected_remote_cache_warnings and any uses of
dc.get_format, dc.scan, dc.read when choosing the exact message regex.
---
Duplicate comments:
In `@dascore/utils/hdf5.py`:
- Around line 551-566: The __init__ of _RemoteH5Writer can leave an orphaned
temp file if any exception occurs between creating self._temp_path and
successfully constructing self._handle; wrap the block that does the remote copy
and H5pyFile(self._temp_path, ...) construction in a try/except (or try/finally)
so that on any exception you unlink/delete self._temp_path (and set any partial
state like self._closed) before re-raising the exception; specifically protect
the region that uses resource.open(...), shutil.copyfileobj(...), and
H5pyFile(...) and ensure cleanup of self._temp_path when initialization fails.
In `@dascore/utils/misc.py`:
- Around line 832-833: The direct call to Path(path).stat().st_size when path is
not None and is_local_path(path) can raise for non-file-backed streams; wrap the
stat probe in a try/except (catching OSError/Exception) and only set file_size
on success, otherwise leave file_size unset so the existing tell()/seek()
fallback runs; refer to the is_local_path check, the file_size variable
assignment, and the existing tell()/seek() fallback code to implement the
guarded stat call.
In `@tests/test_io/conftest.py`:
- Around line 34-39: The except block in copyfile (method copyfile) currently
catches BrokenPipeError and ConnectionResetError but misses
ConnectionAbortedError (raised on Windows), causing noisy disconnect failures;
update the except to include ConnectionAbortedError in the exception tuple
handled for super().copyfile(source, outputfile) so that BrokenPipeError,
ConnectionResetError, and ConnectionAbortedError are all swallowed and the
method returns None on those disconnects.
---
Nitpick comments:
In `@tests/test_io/test_h5simple/test_h5simple.py`:
- Around line 78-84: Add two new unit tests mirroring
test_get_root_attrs_supports_h5py that explicitly exercise the h5py-specific
branches for _iter_root_arrays and _get_attr_names: open an h5py.File in write
mode, create root datasets/arrays (for _iter_root_arrays) and several root
attributes (for _get_attr_names), call the corresponding helper
(_iter_root_arrays and _get_attr_names) with the h5py.File object, and assert
the returned iterator/list contains the expected dataset names and attribute
names/values; ensure tests use the same tmp_path pattern and names as the
existing test to keep them isolated and deterministic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 888420f8-5b13-4d54-ae15-56debf3133c5
📒 Files selected for processing (73)
.github/scripts/cache_test_data.py.gitignorebenchmarks/readme.mdbenchmarks/test_io_benchmarks.pydascore/__init__.pydascore/clients/dirspool.pydascore/clients/filespool.pydascore/compat.pydascore/config.pydascore/constants.pydascore/core/spool.pydascore/core/summary.pydascore/exceptions.pydascore/io/__init__.pydascore/io/core.pydascore/io/dasdae/core.pydascore/io/dasdae/utils.pydascore/io/h5simple/core.pydascore/io/h5simple/utils.pydascore/io/indexer.pydascore/io/rsf/core.pydascore/io/segy/core.pydascore/io/sentek/core.pydascore/io/sentek/utils.pydascore/io/sintela_binary/core.pydascore/io/sintela_binary/utils.pydascore/io/tdms/core.pydascore/io/tdms/utils.pydascore/io/wav/core.pydascore/io/xml_binary/core.pydascore/io/xml_binary/utils.pydascore/utils/display.pydascore/utils/downloader.pydascore/utils/hdf5.pydascore/utils/io.pydascore/utils/misc.pydascore/utils/patch.pydascore/utils/paths.pydascore/utils/progress.pydascore/utils/remote_io.pydocs/changelog.qmddocs/contributing/new_format.qmddocs/tutorial/configuration.qmddocs/tutorial/file_io.qmddocs/tutorial/remote_patches.qmddocs/tutorial/spool.qmdpyproject.tomlscripts/_templates/_quarto.ymltests/conftest.pytests/test_clients/test_filespool.pytests/test_io/_common_io_test_utils.pytests/test_io/conftest.pytests/test_io/test_common_io.pytests/test_io/test_dasdae/test_dasdae.pytests/test_io/test_dasvader/test_dasvader.pytests/test_io/test_h5simple/test_h5simple.pytests/test_io/test_indexer.pytests/test_io/test_io_core.pytests/test_io/test_remote_common_io.pytests/test_io/test_remote_http.pytests/test_io/test_remote_memory.pytests/test_io/test_rsf/test_rsf.pytests/test_io/test_tdms/test_tdms_utils.pytests/test_io/test_wav/test_wav.pytests/test_io/test_xml_binary/test_xml_binary.pytests/test_utils/test_config.pytests/test_utils/test_display.pytests/test_utils/test_downloader.pytests/test_utils/test_hdf_utils.pytests/test_utils/test_io_utils.pytests/test_utils/test_misc.pytests/test_utils/test_paths.pytests/test_utils/test_progress.py
💤 Files with no reviewable changes (2)
- dascore/init.py
- dascore/io/init.py
✅ Files skipped from review due to trivial changes (19)
- .github/scripts/cache_test_data.py
- dascore/compat.py
- .gitignore
- benchmarks/readme.md
- dascore/exceptions.py
- scripts/_templates/_quarto.yml
- dascore/io/tdms/utils.py
- docs/contributing/new_format.qmd
- dascore/utils/display.py
- dascore/constants.py
- tests/test_utils/test_downloader.py
- docs/tutorial/configuration.qmd
- docs/tutorial/file_io.qmd
- dascore/clients/dirspool.py
- docs/tutorial/spool.qmd
- pyproject.toml
- tests/test_utils/test_paths.py
- docs/tutorial/remote_patches.qmd
- tests/test_utils/test_config.py
🚧 Files skipped from review as they are similar to previous changes (23)
- benchmarks/test_io_benchmarks.py
- docs/changelog.qmd
- tests/test_utils/test_progress.py
- dascore/io/sentek/core.py
- dascore/io/sintela_binary/utils.py
- dascore/utils/patch.py
- tests/test_clients/test_filespool.py
- dascore/io/sintela_binary/core.py
- dascore/clients/filespool.py
- dascore/io/wav/core.py
- dascore/io/h5simple/utils.py
- tests/test_utils/test_display.py
- dascore/io/xml_binary/utils.py
- dascore/io/h5simple/core.py
- dascore/core/spool.py
- tests/test_io/test_wav/test_wav.py
- dascore/utils/paths.py
- tests/test_io/test_remote_common_io.py
- tests/test_io/test_remote_memory.py
- tests/test_io/test_io_core.py
- dascore/io/xml_binary/core.py
- dascore/core/summary.py
- tests/test_utils/test_io_utils.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/test_io/conftest.py (1)
34-39:⚠️ Potential issue | 🟡 MinorAlso catch
ConnectionAbortedErrorhere.Line 38 still misses the Windows disconnect mode previously seen in CI (
[WinError 10053]), so this fixture can still fail on client aborts.Suggested diff
def copyfile(self, source, outputfile): """Silence BrokenPipeError/ConnectionResetError from test clients.""" try: return super().copyfile(source, outputfile) - except (BrokenPipeError, ConnectionResetError): + except (BrokenPipeError, ConnectionResetError, ConnectionAbortedError): return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_io/conftest.py` around lines 34 - 39, The copyfile method currently only catches BrokenPipeError and ConnectionResetError; update its exception handling to also include ConnectionAbortedError so client aborts on Windows ([WinError 10053]) are silenced. In the copyfile function (method name copyfile in tests/test_io/conftest.py) add ConnectionAbortedError to the except tuple so the handler becomes except (BrokenPipeError, ConnectionResetError, ConnectionAbortedError): and continue returning None as before.
🧹 Nitpick comments (1)
tests/test_io/conftest.py (1)
237-240: Preserve relative paths in these conversion helpers.Line 238 and Line 252 collapse everything to the basename, so a nested local fixture path like
nested/example_dasdae_event_2.h5gets remapped to/das/example_dasdae_event_2.h5. That’s narrower than the “local fixture path or fetch name” contract in the docstrings.Either preserve the relative path when the input is already a local fixture path, or narrow the helper contract/docstrings to basename-only inputs.
Also applies to: 251-254
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_io/conftest.py` around lines 237 - 240, The helper _convert currently discards subdirectories by using Path(path_or_name).name; instead, keep the relative path: construct p = Path(path_or_name), call ensure_http_fetch_file(p.name) to ensure the file is available, and return http_das_path / p (so nested paths like "nested/example.h5" map to http_das_path/nested/example.h5). Apply the same change to the other conversion helper with the same basename-only pattern so both preserve relative paths while still passing the basename into ensure_http_fetch_file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_io/conftest.py`:
- Around line 213-228: The readiness probe (the loop using probe_url and
urlopen) must be inside the same try/finally that performs cleanup so the
uvicorn thread is always stopped; move the probe and its failure check into the
try block that surrounds the yield so that any pytest.fail() triggers the
finally which sets server.should_exit and joins thread. Ensure you still start
the thread (thread = threading.Thread(target=_run, daemon=True)) before the try,
then inside try run the readiness loop, call pytest.fail() on exhaustion, yield
UPath(f"http://{host}:{port}/das"), and in finally set server.should_exit = True
and thread.join(timeout=5).
- Around line 190-196: Don’t call Path.resolve() on the requested file (which
removes symlink fallback); instead build the candidate path as file_path =
served_root / rel_path (no .resolve()) and perform the containment check with a
string/commonpath comparison (e.g., os.path.commonpath([str(served_root),
str(file_path)]) != str(served_root)) to prevent path traversal while preserving
symlinks used by _link_or_copy; keep the subsequent exists()/is_file() checks
and return response_cls(404) or file_response_cls(file_path) as before.
---
Duplicate comments:
In `@tests/test_io/conftest.py`:
- Around line 34-39: The copyfile method currently only catches BrokenPipeError
and ConnectionResetError; update its exception handling to also include
ConnectionAbortedError so client aborts on Windows ([WinError 10053]) are
silenced. In the copyfile function (method name copyfile in
tests/test_io/conftest.py) add ConnectionAbortedError to the except tuple so the
handler becomes except (BrokenPipeError, ConnectionResetError,
ConnectionAbortedError): and continue returning None as before.
---
Nitpick comments:
In `@tests/test_io/conftest.py`:
- Around line 237-240: The helper _convert currently discards subdirectories by
using Path(path_or_name).name; instead, keep the relative path: construct p =
Path(path_or_name), call ensure_http_fetch_file(p.name) to ensure the file is
available, and return http_das_path / p (so nested paths like
"nested/example.h5" map to http_das_path/nested/example.h5). Apply the same
change to the other conversion helper with the same basename-only pattern so
both preserve relative paths while still passing the basename into
ensure_http_fetch_file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 29c34f61-38da-49e7-9129-b49d03568f34
📒 Files selected for processing (2)
.agents/plans/duck_db_indexer.qmdtests/test_io/conftest.py
✅ Files skipped from review due to trivial changes (1)
- .agents/plans/duck_db_indexer.qmd
|
✅ Documentation built: |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/runtests.yml (1)
96-100: Remove stale commented tmate block.The commented duplicate debug block is now dead config and can be dropped to reduce workflow noise.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/runtests.yml around lines 96 - 100, Remove the stale commented tmate block by deleting the commented lines that declare the "Setup tmate session" step and the use of "mxschmitt/action-tmate@v3" so the duplicate debug block is no longer present in the workflow; ensure no other commented duplicate debug steps remain in runtests.yml.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/actions/load-shared-vars/action.yml:
- Around line 26-30: The shared CI action reduced test coverage by narrowing the
python_test_matrix and test_os_matrix; revert or restore those values so the
action continues to provide full-platform coverage for all consumers—update the
python_test_matrix and test_os_matrix entries in action.yml to include the
original python versions (including 3.11 and 3.12) and all OS entries
(ubuntu-latest, macos-latest, windows-latest), or alternatively move any
experimental matrix narrowing into a workflow-level override so the shared
action remains unchanged.
In @.github/workflows/runtests.yml:
- Around line 73-75: Gate the interactive tmate step ("name: Setup tmate
session" using "mxschmitt/action-tmate@v3") so it only runs under safe
conditions: add a conditional like if: failure() to run on failure-only, set the
action input limit-access-to-actor: true to restrict SSH to the workflow actor,
and (for public repos) move tmate behind a manual trigger by adding
workflow_dispatch with an inputs.debug_enabled flag and change the step
condition to if: ${{ github.event_name == 'workflow_dispatch' &&
inputs.debug_enabled }} so tmate only activates on explicit opt-in.
---
Nitpick comments:
In @.github/workflows/runtests.yml:
- Around line 96-100: Remove the stale commented tmate block by deleting the
commented lines that declare the "Setup tmate session" step and the use of
"mxschmitt/action-tmate@v3" so the duplicate debug block is no longer present in
the workflow; ensure no other commented duplicate debug steps remain in
runtests.yml.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f29dc5ca-346b-49c8-8a3a-949ca6a13268
📒 Files selected for processing (3)
.github/actions/load-shared-vars/action.yml.github/workflows/run_min_dep_tests.yml.github/workflows/runtests.yml
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #645 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 134 137 +3
Lines 12042 12650 +608
==========================================
+ Hits 12042 12650 +608
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
d-chambers
force-pushed
the
universal-pathlib
branch
from
April 2, 2026 19:21
b7d5c93 to
ea83d06
Compare
4 tasks
4 tasks
Merged
This was referenced Jul 20, 2026
Merged
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Add universal-pathlib as a runtime dependency and extend DASCore path handling so str, Path, and UPath inputs work consistently across public IO entry points.
Teach dc.read, dc.scan, dc.spool, dc.get_format, FileSpool, and related internals to preserve remote paths, normalize local-vs-remote behavior, and reject directory-only operations when the backend is not a local
filesystem.
Introduce shared remote IO/cache utilities for:
Update HDF5-based readers to prefer remote-first access and only fall back to a cached local file when a backend, especially plain HTTP, does not support the random-access behavior needed by h5py/PyTables.
Add runtime config support for remote cache settings and related display/progress configuration, plus docs/tutorial updates covering remote patches, remote file IO, and runtime configuration.
Expand test coverage for:
Include follow-up cleanup and stability fixes:
edit: updated according to current branch state.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
Changelog
UPathresources work acrossread,scan,spool, andwrite, including remote backends such asmemory://.historyas a flat string payload; older files stay readable but their original history strings are no longer restored exactly.