Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion dascore/io/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -720,7 +720,7 @@ def _get_format(
# raise, in which case the format doesn't belong.
func_input = man.get_resource(required_type)
format_version = func(func_input, _pre_cast=True)
except RemoteCacheError: # pragma: no cover -- remote fetch only
except RemoteCacheError:
# A remote fetch failure is a real error, not a "wrong
# format" signal, so it must propagate rather than be
# swallowed by the robustness handler below.
Expand Down
12 changes: 6 additions & 6 deletions dascore/utils/chunk_plan.py
Original file line number Diff line number Diff line change
Expand Up @@ -425,12 +425,12 @@ def _build_members(sub: pd.DataFrame, outputs: pd.DataFrame, name) -> pd.DataFra
continue
lo = max(src1[src_num], chu1[out_num])
hi = min(src2[src_num], chu2[out_num])
if lo > hi: # pragma: no cover -- searchsorted boundary guard
# Sources within a partition are continuous (partitioning
# splits on gaps) and start-corrected, so searchsorted does
# not offer a non-overlapping source in practice; this guards
# against a boundary off-by-one rather than a reachable state.
continue
# Sources within a partition are continuous (partitioning
# splits on gaps) and start-corrected, so searchsorted never
# offers a source which does not overlap the output. Assert it
# rather than skipping: silently dropping a source would lose
# data, and the state cannot be reached from the public API.
assert lo <= hi, f"source {src_num} does not overlap output {out_num}"
Comment on lines +428 to +433

@coderabbitai coderabbitai Bot Jul 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
python -O - <<'PY'
def guard(lo, hi):
    assert lo <= hi
    return "invalid interval accepted"

assert guard(2, 1) == "invalid interval accepted"
PY

Repository: DASDAE/dascore

Length of output: 152


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== locate file =="
git ls-files | rg '(^|/)dascore/utils/chunk_plan\.py$' || true

echo "== relevant source =="
sed -n '400,445p' dascore/utils/chunk_plan.py

echo "== python -O assert semantics demonstration =="
tmpf="$(mktemp)"
cat > "$tmpf" <<'PY'
def guard(lo, hi):
    assert lo <= hi
    return "invalid interval accepted"

print(guard(2, 1))
PY
echo "without -O:"
python3 "$tmpf" || true
echo "with -O:"
python3 -O "$tmpf"
rm "$tmpf"

echo "== inspect pytest/optimize-related config =="
git ls-files | rg '(^|/)(pyproject\.toml|setup\.cfg|tox\.ini|noxfile\.py|pytest\.ini|Makefile)$' || true
for f in $(git ls-files | rg '(^|/)(pyproject\.toml|setup\.cfg|tox\.ini|noxfile\.py|pytest\.ini|Makefile)$'); do
  echo "--- $f ---"
  rg -n "opti|python.O|pytest|assert|noassert|assertions" "$f" || true
done

Repository: DASDAE/dascore

Length of output: 2990


Use an explicit runtime check instead of assert.

assert statements are skipped under python -O, so a non-overlapping lo > hi interval would fall through and append an invalid member row instead of failing.

Proposed fix
-            assert lo <= hi, f"source {src_num} does not overlap output {out_num}"
+            if lo > hi:
+                raise AssertionError(
+                    f"source {src_num} does not overlap output {out_num}"
+                )

Add a regression test for this path with optimization enabled.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Sources within a partition are continuous (partitioning
# splits on gaps) and start-corrected, so searchsorted never
# offers a source which does not overlap the output. Assert it
# rather than skipping: silently dropping a source would lose
# data, and the state cannot be reached from the public API.
assert lo <= hi, f"source {src_num} does not overlap output {out_num}"
# Sources within a partition are continuous (partitioning
# splits on gaps) and start-corrected, so searchsorted never
# offers a source which does not overlap the output. Assert it
# rather than skipping: silently dropping a source would lose
# data, and the state cannot be reached from the public API.
if lo > hi:
raise AssertionError(
f"source {src_num} does not overlap output {out_num}"
)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@dascore/utils/chunk_plan.py` around lines 428 - 433, Replace the assert in
the chunk-plan source/output overlap handling with an explicit runtime check
that raises the appropriate failure when lo > hi, preventing an invalid member
row from being appended under python -O. Add a regression test covering this
non-overlapping interval with optimization enabled.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Keeping the assert, deliberately.

The -O behaviour is real, but this is an internal invariant of the partitioner rather than validation of anything a caller supplies: sources within a partition are continuous (partitioning splits on gaps) and start-corrected, so searchsorted cannot offer a source disjoint from its output. That is the case asserts exist for, and it holds across the full suite (8229 tests).

An explicit raise would need its own coverage exclusion, since the raising line still never executes — which is what this PR set out to remove. A test could only reach it by handing _build_members a state the public API cannot produce, and @codex rightly pushed back on exactly that earlier in this PR (see the _build_members thread), since .agents/agents.md asks for boundary tests rather than implementation-detail tests.

One thing your comment did surface: dropping the old continue means that under -O an impossible state would append an invalid row instead of being skipped. Worth stating plainly rather than leaving implied — though it trades a check on an unreachable state for a clearer statement of the invariant, and skipping silently would have lost a source's data anyway.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Skipped: comment is from another GitHub bot.

unchanged = (
lo == orig_min[src_num]
and hi == orig_max[src_num]
Expand Down
10 changes: 5 additions & 5 deletions dascore/utils/hdf5.py
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ class H5Writer(H5Reader):
class _RemoteH5Writer:
"""Wrap a local h5py file and upload it back to the remote resource."""

def __init__(self, resource: UPath, mode: str): # pragma: no cover
def __init__(self, resource: UPath, mode: str):
self._resource = resource
suffix = resource.suffix or ".h5"
fd, temp_name = tempfile.mkstemp(suffix=suffix)
Expand All @@ -264,10 +264,10 @@ def __init__(self, resource: UPath, mode: str): # pragma: no cover
def __getitem__(self, item):
return self._handle[item]

def __setitem__(self, key, value): # pragma: no cover
def __setitem__(self, key, value):
self._handle[key] = value

def __contains__(self, item): # pragma: no cover
def __contains__(self, item):
return item in self._handle

def commit(self):
Expand Down Expand Up @@ -299,10 +299,10 @@ def _abort(self):
"""Backward-compatible alias for abort()."""
self.abort()

def __enter__(self): # pragma: no cover
def __enter__(self):
return self

def __exit__(self, exc_type, exc, tb): # pragma: no cover
def __exit__(self, exc_type, exc, tb):
if exc_type is None:
self.commit()
else:
Expand Down
21 changes: 21 additions & 0 deletions tests/test_io/test_io_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -514,6 +514,27 @@ def test_other_formats_still_usable(self, broken_ep_manager):
assert len(list(broken_ep_manager.yield_fiberio()))


class TestGetFormatErrors:
"""Errors which must not be mistaken for a format mismatch."""

def test_remote_cache_error_propagates(self, monkeypatch, tmp_path):
"""A remote fetch failure is a real error, not a wrong-format signal.

The loop over FiberIOs swallows exceptions so a reader which does
not recognize a file can be skipped; a cache failure has to escape
that handler instead of being reported as an unknown format.
"""
path = tmp_path / "unfetchable.h5"
path.write_bytes(b"not really an h5 file")

def _raise(*args, **kwargs):
raise RemoteCacheError("cannot fetch this resource")

monkeypatch.setattr(IOResourceManager, "get_resource", _raise)
with pytest.raises(RemoteCacheError, match="cannot fetch this resource"):
dc.get_format(path)


class TestFormatManagerConcurrency:
"""Concurrent plugin loading must never expose a partial registry."""

Expand Down
49 changes: 49 additions & 0 deletions tests/test_utils/test_io_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
from upath import UPath

import dascore as dc
import dascore.utils.hdf5 as hdf5_module
import dascore.utils.remote_io as remote_io
from dascore.config import config_context
from dascore.exceptions import PatchConversionError, RemoteCacheError
Expand Down Expand Up @@ -427,6 +428,54 @@ def test_h5_writer_to_remote_upath_aborts_on_context_error(self):
raise RuntimeError("boom")
assert not path.exists()

def test_h5_writer_remote_context_commits(self):
"""Leaving the context without an error uploads the file."""
path = UPath("memory://dascore/upath-write-commit.h5")
with H5Writer.get_handle(path) as handle:
handle.create_dataset("data", data=[1, 2, 3])
assert path.exists()
with path.open("rb") as raw, h5py.File(raw, "r", driver="fileobj") as reopened:
assert list(reopened["data"][:]) == [1, 2, 3]

def test_h5_writer_remote_append_keeps_existing(self):
"""Reopening an existing remote file downloads it before writing."""
path = UPath("memory://dascore/upath-write-append.h5")
with H5Writer.get_handle(path) as handle:
handle.create_dataset("first", data=[1, 2, 3])
with H5Writer.get_handle(path) as handle:
handle.create_dataset("second", data=[4, 5, 6])
with path.open("rb") as raw, h5py.File(raw, "r", driver="fileobj") as reopened:
assert list(reopened["first"][:]) == [1, 2, 3]
assert list(reopened["second"][:]) == [4, 5, 6]

def test_h5_writer_remote_setitem_and_contains(self):
"""The remote writer proxies item assignment and membership."""
path = UPath("memory://dascore/upath-write-setitem.h5")
with H5Writer.get_handle(path) as handle:
handle["data"] = [1, 2, 3]
assert "data" in handle
assert "missing" not in handle

def test_h5_writer_remote_cleans_up_after_open_failure(self, monkeypatch):
"""A failed local open removes the temp file and raises."""
created = []
real_mkstemp = hdf5_module.tempfile.mkstemp

def _tracking_mkstemp(*args, **kwargs):
file_descriptor, name = real_mkstemp(*args, **kwargs)
created.append(Path(name))
return file_descriptor, name

def _raise(*args, **kwargs):
raise RuntimeError("boom")

monkeypatch.setattr(hdf5_module.tempfile, "mkstemp", _tracking_mkstemp)
monkeypatch.setattr(hdf5_module, "H5pyFile", _raise)
path = UPath("memory://dascore/upath-write-open-failure.h5")
with pytest.raises(RuntimeError, match="boom"):
H5Writer.get_handle(path)
assert created and not created[0].exists()

def test_h5_writer_remote_abort_is_idempotent(self):
"""Remote writer aborts should be safe to call more than once."""
path = UPath("memory://dascore/upath-write-abort-twice.h5")
Expand Down
Loading