Prune dead code and collapse single-implementation abstractions - #759
Conversation
|
Warning Review limit reached
Next review available in: 49 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThis PR removes deprecated summary, configuration, and indexer abstractions; consolidates SQLite dialect and backend interfaces; updates directory indexer integration; and revises exports, documentation, and affected tests. ChangesPublic API and summary cleanup
SQLite index consolidation
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
bug
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #759 +/- ##
===========================================
+ Coverage 48.92% 99.96% +51.04%
===========================================
Files 164 163 -1
Lines 17191 17087 -104
===========================================
+ Hits 8410 17081 +8671
+ Misses 8781 6 -8775
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
74815c4 to
a272837
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
dascore/io/index/indexer.py (2)
252-253: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle potential file deletions during directory traversal.
Files yielded by
_iter_filesystemmight be deleted beforepath.stat()is called (a Time-of-Check to Time-of-Use race), which would raise aFileNotFoundErrorand crash the entire index update.Wrap the
stat()call in atry-exceptblock to gracefully ignore files that disappear during the scan.🛡️ Proposed fix to handle concurrent deletions
- stat = path.stat() - files[self._rel(path)] = (stat.st_mtime_ns, stat.st_size, path) + try: + stat = path.stat() + except OSError: + continue + files[self._rel(path)] = (stat.st_mtime_ns, stat.st_size, path)🤖 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/io/index/indexer.py` around lines 252 - 253, Update the file-statting logic in the filesystem indexing flow around `_iter_filesystem` to catch `FileNotFoundError` from `path.stat()`, skip the disappeared file, and continue scanning the remaining paths without adding an entry to `files`.
155-157: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse a stable hash for persistent file names.
In Python 3,
hash()is randomized per process for strings andPathobjects. Consequently,abs(hash(self.path))will generate a completely different file name in every Python session. If the index map is ever cleared—or checked concurrently by two processes before being updated—a new SQLite file will be created, permanently orphaning the old one.Use a stable hash like
hashlib.sha256to ensure the file name remains consistent across processes.🛠️ Proposed fix using a stable hash
if not directory_writable(self.path): - name = f"_dascore_index_{abs(hash(self.path))}.sqlite3" + digest = hashlib.sha256(str(self.path).encode()).hexdigest()[:16] + name = f"_dascore_index_{digest}.sqlite3" index_path = self.index_map_path.parent / name🤖 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/io/index/indexer.py` around lines 155 - 157, Replace the process-randomized hash used in the Indexer path fallback with a deterministic hashlib.sha256 digest of a stable path representation, and use that digest in the SQLite filename. Update the relevant imports and preserve the existing directory_writable and index_map_path behavior.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@dascore/io/index/indexer.py`:
- Around line 58-65: Update _update_index_map to write the serialized index data
to a temporary file in the same directory as cache_path, then atomically replace
cache_path with that file using Path.replace(). Preserve the existing directory
creation, merge behavior, and returned data while ensuring readers never observe
a partially written cache.
- Around line 34-40: Remove the `@cache` decorator from _get_index_map so each
invocation reads a fresh index map from disk before updates. After removing it,
delete the functools.cache import if no other code uses it, while preserving
_get_index_map’s existing return behavior and mutable-dictionary contract.
---
Outside diff comments:
In `@dascore/io/index/indexer.py`:
- Around line 252-253: Update the file-statting logic in the filesystem indexing
flow around `_iter_filesystem` to catch `FileNotFoundError` from `path.stat()`,
skip the disappeared file, and continue scanning the remaining paths without
adding an entry to `files`.
- Around line 155-157: Replace the process-randomized hash used in the Indexer
path fallback with a deterministic hashlib.sha256 digest of a stable path
representation, and use that digest in the SQLite filename. Update the relevant
imports and preserve the existing directory_writable and index_map_path
behavior.
🪄 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: e94b887c-7f22-49ae-aafe-2b97a67552c3
📒 Files selected for processing (15)
dascore/config.pydascore/core/spool.pydascore/core/summary.pydascore/io/__init__.pydascore/io/core.pydascore/io/index/__init__.pydascore/io/index/backend.pydascore/io/index/dialect.pydascore/io/index/indexer.pydascore/io/index/query.pydascore/io/indexer.pydocs/changelog.qmddocs/notes/coordinate_internals.qmdtests/test_core/test_patch.pytests/test_io/test_io_core.py
💤 Files with no reviewable changes (3)
- dascore/io/init.py
- dascore/io/indexer.py
- dascore/config.py
🚧 Files skipped from review as they are similar to previous changes (10)
- dascore/io/index/init.py
- docs/notes/coordinate_internals.qmd
- docs/changelog.qmd
- tests/test_io/test_io_core.py
- dascore/core/summary.py
- dascore/core/spool.py
- dascore/io/index/dialect.py
- dascore/io/index/backend.py
- dascore/io/core.py
- dascore/io/index/query.py
|
✅ Documentation built: |
a272837 to
e51b6cb
Compare
coderabbitai
Bot
added
codex
and removed
documentation
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
dascore/io/index/indexer.py (2)
96-101: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPrevent
FileNotFoundErrorin concurrent index rebuilds.If multiple processes encounter an invalid index version simultaneously, one may execute the
unlinkbefore another, causing aFileNotFoundErrorwhen the trailing process attempts to unlink the already deleted file.Adding
missing_ok=Truecloses this race window and improves stability under concurrent access.🔒️ Proposed fix
except InvalidIndexVersionError: # The index is a disposable cache and the file already # identified itself as a dascore spool index of another # schema version; rebuild it rather than asking the user to. - self.index_path.unlink() + self.index_path.unlink(missing_ok=True) self._backend = get_backend(self.index_path)🤖 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/io/index/indexer.py` around lines 96 - 101, Update the InvalidIndexVersionError recovery block in the index initialization flow to call self.index_path.unlink with missing_ok=True, allowing concurrent rebuilds to proceed when another process has already removed the cache file. Keep the existing backend recreation behavior unchanged.
155-157: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse a stable hash instead of Python's built-in
hash()for filenames.Python's built-in
hash()for strings and paths uses a randomized seed per interpreter session (introduced in Python 3.3). If two processes initialize the same read-only directory concurrently, they will generate completely different filenames. This leads to orphaned SQLite databases that permanently leak disk space and fails to reliably reuse the cache across process restarts.Use a deterministic hash function like
hashlib.sha256to ensure the same directory always maps to the exact same filename.🐛 Proposed fix
if not directory_writable(self.path): - name = f"_dascore_index_{abs(hash(self.path))}.sqlite3" + hash_hex = hashlib.sha256(str(self.path).encode()).hexdigest()[:16] + name = f"_dascore_index_{hash_hex}.sqlite3" index_path = self.index_map_path.parent / name🤖 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/io/index/indexer.py` around lines 155 - 157, Replace the built-in hash used in the directory_writable fallback within the index path construction with a deterministic hashlib-based digest of self.path. Preserve the existing filename pattern and SQLite suffix while ensuring identical paths produce the same filename across processes and interpreter restarts.
🤖 Prompt for all review comments with 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.
Outside diff comments:
In `@dascore/io/index/indexer.py`:
- Around line 96-101: Update the InvalidIndexVersionError recovery block in the
index initialization flow to call self.index_path.unlink with missing_ok=True,
allowing concurrent rebuilds to proceed when another process has already removed
the cache file. Keep the existing backend recreation behavior unchanged.
- Around line 155-157: Replace the built-in hash used in the directory_writable
fallback within the index path construction with a deterministic hashlib-based
digest of self.path. Preserve the existing filename pattern and SQLite suffix
while ensuring identical paths produce the same filename across processes and
interpreter restarts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 199c4c65-6d0a-486e-b180-8df068303e59
📒 Files selected for processing (15)
dascore/config.pydascore/core/spool.pydascore/core/summary.pydascore/io/__init__.pydascore/io/core.pydascore/io/index/__init__.pydascore/io/index/backend.pydascore/io/index/dialect.pydascore/io/index/indexer.pydascore/io/index/query.pydascore/io/indexer.pydocs/changelog.qmddocs/notes/coordinate_internals.qmdtests/test_core/test_patch.pytests/test_io/test_io_core.py
💤 Files with no reviewable changes (3)
- dascore/io/indexer.py
- dascore/config.py
- dascore/io/init.py
🚧 Files skipped from review as they are similar to previous changes (10)
- dascore/io/index/init.py
- docs/notes/coordinate_internals.qmd
- docs/changelog.qmd
- dascore/io/index/dialect.py
- dascore/core/spool.py
- dascore/io/core.py
- dascore/io/index/backend.py
- tests/test_io/test_io_core.py
- dascore/core/summary.py
- tests/test_core/test_patch.py
Removes production-dead code and two abstraction layers left over from the DuckDB/Parquet backend removal. Dead code: - PatchFileSummary, superseded by PatchSummary and referenced only by its own export and tests. - _load_patch_summary, only reachable from tests; FileResolver._read is the path actually used for summary-driven reloads. - coord_summary_from_data and _normalize_coord_summary_dtype, neither called anywhere in dascore. - The index_query_buffer config option, which lost its only reader when the PyTables indexer was replaced. The numpy import and arbitrary_types_allowed in config.py existed solely for its timedelta64 field and go with it. - A stale TODO in PatchSummary.flat_dump; the indexer has been redone and the flat names are the contract. Duplication in spool.py: - The viz property duplicated the _namespace_attr_errors entry and shadowed it; the mixin now serves the same message. - get_patch_names was bound on both BaseSpool and Spool. Abstractions with one implementation: - AbstractIndexBackend declared the interface SQLIndexBackend already implements in full; SQLIndexBackend is now the base, keeping close() among its per-engine abstract hooks. - BaseDialect/SQLiteDialect collapse into one concrete SQLiteDialect. The dialect stays its own module: query needs it and backend needs query, so moving it into either would close an import cycle. - dascore.io.indexer merges into dascore.io.index.indexer, resolving the confusing near-identical module names. AbstractIndexer was a self-described placeholder with one subclass; Spool.from_directory now tests for DBDirectoryIndexer directly.
Removing _load_patch_summary also removed the only test reaching the empty-spool MissingPatchError guard in _select_patch_from_spool, which that helper had been exercising indirectly. The guard is retained production code, so cover it directly instead. Parametrized over a present and absent source id: the guard must fire before any identity matching in both cases.
Fixes concurrency and correctness issues in the index-map helpers, all pre-existing but surfaced by moving them into io/index/indexer.py. - Drop @cache from _get_index_map. Caching the map per process meant a later _update_index_map would dump this process's stale copy over entries another process had added. Reads now hit disk fresh. - Write _update_index_map atomically (temp file + os.replace). A direct write is not atomic; a concurrent reader hitting a half-written file raises JSONDecodeError, which _get_index_map treats as corruption and deletes the whole map (#508). replace() means readers only ever see a complete file. - Name the read-only-directory fallback index from a sha256 digest of the path, not abs(hash(path)). str/Path hashing is randomized per process (PYTHONHASHSEED), so hash() named a new index every session and orphaned the previous one. - Skip files that disappear between the directory walk and their stat() instead of letting a concurrent deletion crash the whole update. Regression tests pin all four; each was confirmed to fail when its fix is reverted.
56dce5b to
4244f04
Compare
This test asserts no ResourceWarning fires while it garbage-collects its own spool. But gc.collect() also reaps collectable garbage left by earlier tests, and a leaked open SQLite connection emits a ResourceWarning when collected -- which the recording window then caught and blamed on this test. The result was a full-suite-order failure (reproducible on dev independent of this branch; #754 merged with this same test_code job red) that passed in isolation and within its own file. Drain pending garbage with a gc.collect() before opening the recording window, so only the spool under test is collected inside it. The test's intent -- this spool's backend connection closes silently on GC -- is unchanged.
1 participant
Description
Follow-up cleanup on
devfrom a review pass over the branch. No behavior changes intended — this removes production-dead code and two abstraction layers that were left with a single implementation after the DuckDB/Parquet backends were removed.Dead code
PatchFileSummary— superseded byPatchSummary; referenced only by its own export indascore/io/__init__.pyand its tests. Removing it also retires twelve now-unused imports inio/core.py._load_patch_summary— only reachable from tests.FileResolver._read/_resolve_read_spoolis the path actually used for summary-driven reloads.coord_summary_from_dataand_normalize_coord_summary_dtype— neither is called anywhere indascore. The docs note that mentioned them (along with an already-stalecoord_summary_from_full_data) now points atget_coord(...).to_summary().index_query_bufferconfig option — lost its only reader when the PyTables indexer was replaced by SQLite. Thenumpyimport andarbitrary_types_allowedinconfig.pyexisted solely to support itstimedelta64field and go with it.TODOinPatchSummary.flat_dump— the indexer has since been redone, and the flatpath/file_format/file_versionnames are the contract, so the comment is now a statement of intent rather than pending work.Duplication in
spool.pyvizproperty duplicated the_namespace_attr_errors["viz"]message and shadowed it, making the dict entry unreachable.NamespaceOwner.__getattr__now serves the identical message (verified).get_patch_nameswas bound on bothBaseSpoolandSpool; the second binding is redundant.Abstractions with one implementation
AbstractIndexBackendmerged intoSQLIndexBackend. The ABC declared twelve methods thatSQLIndexBackendalready implements in full, so it was pure redeclaration.SQLIndexBackendis now the base;close()joins its per-engine abstract hooks (it was the one method the ABC declared thatSQLIndexBackenddoes not implement). The package now exportsSQLIndexBackendin place ofAbstractIndexBackend.BaseDialect/SQLiteDialectcollapse into one concreteSQLiteDialect. Note the dialect deliberately stays its own module rather than folding intolite.pyas originally sketched:queryimports the dialect andbackendimportsquery, so a dialect living inlite(orbackend) would close aquery → lite → backend → queryimport cycle. That constraint is now recorded in the module docstring.dascore.io.indexermerged intodascore.io.index.indexer, resolving the near-identical module names.AbstractIndexerwas a self-described placeholder with a single subclass;Spool.from_directorynow tests forDBDirectoryIndexerdirectly, and the index-map helpers move alongside their only caller.Net: −483 / +92 lines, one module deleted.
Testing
pytest dascore --doctest-modules, run separately in CI): 142 passed.pre-commitclean on all changed files.spool.vizstill raisesAttributeErrorwith the same message,get_patch_namesstill resolves on a concreteSpool, andSpool.from_directory(indexer)still takes the indexer branch.Changelog
dascore.io.PatchFileSummary; usePatchSummaryandget_coord(...).to_summary().dascore.io.indexermodule with itsAbstractIndexerandDirectoryIndexer; usedascore.io.index.indexer.DBDirectoryIndexer.AbstractIndexBackendis gone —SQLIndexBackendis the base every backend implements.Checklist
I have (if applicable):
Summary by CodeRabbit
PatchFileSummarymodel (and corresponding IO re-export);PatchSummaryis the replacement.index_query_bufferconfig option.Spool.vizaccessor and updated directory indexing behavior for SQL-backed indexers.SQLIndexBackend/SQLite dialect.MissingPatchErrorconsistently.to_summary().