Scan Sintela protobuf recordings without reading them - #865
Conversation
📝 WalkthroughWalkthroughSintela protobuf handling now supports bounded framing reads, endpoint-based timeseries metadata, streaming sample decoding, counter rollover validation, and fallback parsing. Tests cover malformed framing, endpoint shortcuts, memory use, payload bounds, and validation behavior. ChangesSintela I/O optimization
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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
benchmark
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_io/test_sintela/test_protobuf.py (2)
1232-1249: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the duplicated
_Bufhelper to module scope.This
_Bufclass is defined again, byte for byte, at Lines 1689-1706 intest_find_last_record_returns_none_without_framing. Move one copy to module scope next to_CountingReaderand use it in both tests.Note that
seekignoreswhence=1. Both current callers use absolute seeks only, so this works today. A shared helper makes that limitation visible in one place.🤖 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 `@tests/test_io/test_sintela/test_protobuf.py` around lines 1232 - 1249, Move the duplicated _Buf helper from the test-local scope to module scope beside _CountingReader, preserving its current seek, tell, and read behavior. Remove the second definition in test_find_last_record_returns_none_without_framing and update both tests to reuse the shared _Buf class.
704-724: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the endpoint shortcut engages before asserting the overrun error.
This test targets the overrun guard in
decode_stream. That guard only runs when_get_endpoint_metadatareturns a shortcut. Here the endpoint time check passes at the exact tolerance boundary:expected_nsis 1.5 s,elapsed_nsis 3.0 s, and the tolerance is_ENDPOINT_TIME_PACKETS * packet_ns= 1.5 s. If_ENDPOINT_TIME_PACKETSor_ENDPOINT_TIME_TOLERANCEis retuned downward, the shortcut declines, the full path raises the same"Non-contiguous"error, and this test keeps passing while the overrun guard becomes untested.
test_short_final_packet_is_measured_from_the_last_endpointalready uses this pattern at Line 753.♻️ Pin the branch under test
path = write_sintela_file("ts_overrun.pb", records) + # Pin the branch: the overrun guard only runs on the shortcut path. + with path.open("rb") as handle: + assert sintela_utils._get_endpoint_metadata(handle) is not None with pytest.raises(InvalidFiberFileError, match="Non-contiguous"): fiber_io.read(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 `@tests/test_io/test_sintela/test_protobuf.py` around lines 704 - 724, Update test_read_rejects_more_samples_than_endpoints_imply to explicitly verify that _get_endpoint_metadata returns the endpoint shortcut before asserting the InvalidFiberFileError. Follow the established pattern in test_short_final_packet_is_measured_from_the_last_endpoint, ensuring the test fails if the shortcut is not engaged while preserving the existing overrun-error assertion through decode_stream.
🤖 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 `@benchmarks/test_io_benchmarks.py`:
- Around line 43-46: Update the single_file_path fixture to depend on the shared
test_file_paths fixture and select request.param from that fixture, removing the
direct get_test_file_paths() call while preserving the existing session-scoped
parametrization.
---
Nitpick comments:
In `@tests/test_io/test_sintela/test_protobuf.py`:
- Around line 1232-1249: Move the duplicated _Buf helper from the test-local
scope to module scope beside _CountingReader, preserving its current seek, tell,
and read behavior. Remove the second definition in
test_find_last_record_returns_none_without_framing and update both tests to
reuse the shared _Buf class.
- Around line 704-724: Update
test_read_rejects_more_samples_than_endpoints_imply to explicitly verify that
_get_endpoint_metadata returns the endpoint shortcut before asserting the
InvalidFiberFileError. Follow the established pattern in
test_short_final_packet_is_measured_from_the_last_endpoint, ensuring the test
fails if the shortcut is not engaged while preserving the existing overrun-error
assertion through decode_stream.
🪄 Autofix
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 Plus
Run ID: 687c4436-8fa1-4360-92fe-553f4e83d9ce
📒 Files selected for processing (5)
benchmarks/test_io_benchmarks.pydascore/io/sintela/protobuf_utils.pydocs/changelog.qmdtests/test_io/test_common_io.pytests/test_io/test_sintela/test_protobuf.py
| @pytest.fixture(scope="session", params=SINGLE_FILE_BENCHMARKS) | ||
| def single_file_path(request): | ||
| """Path to one registry file, parametrized for per-format benchmarks.""" | ||
| return get_test_file_paths()[request.param] |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Reuse the shared registry-path fixture.
Each session-scoped parameter instance calls get_test_file_paths(). This resolves the complete registry four times. Depend on test_file_paths and select the requested path from it.
Proposed fix
`@pytest.fixture`(scope="session", params=SINGLE_FILE_BENCHMARKS)
-def single_file_path(request):
+def single_file_path(request, test_file_paths):
"""Path to one registry file, parametrized for per-format benchmarks."""
- return get_test_file_paths()[request.param]
+ return test_file_paths[request.param]📝 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.
| @pytest.fixture(scope="session", params=SINGLE_FILE_BENCHMARKS) | |
| def single_file_path(request): | |
| """Path to one registry file, parametrized for per-format benchmarks.""" | |
| return get_test_file_paths()[request.param] | |
| `@pytest.fixture`(scope="session", params=SINGLE_FILE_BENCHMARKS) | |
| def single_file_path(request, test_file_paths): | |
| """Path to one registry file, parametrized for per-format benchmarks.""" | |
| return test_file_paths[request.param] |
🤖 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 `@benchmarks/test_io_benchmarks.py` around lines 43 - 46, Update the
single_file_path fixture to depend on the shared test_file_paths fixture and
select request.param from that fixture, removing the direct
get_test_file_paths() call while preserving the existing session-scoped
parametrization.
Scanning read every packet's samples to reach its headers, so indexing a directory cost a full sequential read of every file in it. On a 10,308 file archive of 505 MB recordings that is 5.9 hours. A timeseries scan now derives its summary from the first and last packets alone: sample_count numbers the samples preceding each packet, so the two endpoints give the total directly. The shortcut is taken only when the derived length fits the file's bytes and the endpoint timestamps match the span those samples imply, so a concatenated file, a reset counter, or a misidentified final record falls back to reading everything. Measured over that archive, dc.scan goes from 2.05 s to 0.02 s per file, reading 1.3 MB instead of 505 MB. BAND and FFT scans need every packet's timestamp and so still visit every record, but are handed each packet's header without its samples. Reading a timeseries recording streams packets into the output array instead of decoding all of them first, cutting peak memory from ~2.0x the patch to ~1.02x. A scan no longer detects a gap between the endpoints when the timestamps corroborate it, and reports the span they imply; read validates every packet and still raises InvalidFiberFileError. Adds a generic guard in test_common_io.py holding every FiberIO to reading less than a quarter of any file over 1 MB during scan, so this class of regression cannot recur unnoticed in another reader.
342b904 to
dea1778
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #865 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 176 176
Lines 19313 19501 +188
==========================================
+ Hits 19313 19501 +188
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:
|
Dev's protobuf scan work (#865) landed after this branch renamed the vendor identification attrs, so its trailing-META test asserted the pre-rename spellings; the reader itself already emits the dotted names.
1 participant
Description
Scanning a Sintela protobuf recording read every byte of the file.
_iter_envelope_recordspulled each record's full payload — including the sample blobs, ~99.99% of the file — just to reach the ~100-byte header at the front of each packet. Indexing a directory therefore cost a full sequential read of every file in it.Measured on a 10,308 file archive of 505 MB recordings:
dc.scan, per filespool.update(), per filereadpeak memory (0.50 GB patch)The generic scan-cost guard and the benchmarks that came out of this work are split into #868, stacked on this branch.
What changed
Timeseries scans read two packets.
sample_countnumbers the samples preceding each packet, so the first and last headers give the total directly. The final record is located by searching back from EOF for the framing whose extent lands exactly on EOF.The shortcut is taken only when the file is demonstrably one contiguous run. Three independent checks: each endpoint's declared length must fit its own record, the total must fit the file, and the endpoint timestamps must match the span those samples imply. A concatenated file, a counter that reset, or a mid-file reconfiguration fails one of them and falls back to reading everything. On the archive above, 10,306 of 10,308 files take the shortcut; the two that don't are genuinely damaged (one truncated, one with channels changing mid-file) and both still raise on read.
The timestamp tolerance is calibrated against that archive rather than guessed: a quarter of the files drift (recorder clock resync), worst 56 ms over a 60 s span, and the tolerance sits an order of magnitude above that. A false rejection silently costs a full read, while what is worth catching is wrong by seconds to hours.
BAND and FFT scans need every packet's timestamp, so they still visit every record, but are handed each packet's header without its samples. That bounds their memory, not their I/O — skipping a sub-2 MiB payload with
seekmeasured slower than streaming it on spinning media (a seek costs a platter rotation), so small remainders are read and dropped.Reads stream.
readpreviously decoded all packets before copying them into the output array, holding the data twice. It now fills the preallocated array packet by packet. Headers are copied into fresh header-only messages and the decoded packet dropped, because protobuf frees an arena only when the whole message dies — clearing the sample field in place does not release it.get_formatno longer pulls payloads it never inspects.Behaviour change
A timeseries scan no longer detects a gap between the endpoints when the timestamps corroborate it (a paused and resumed acquisition), and reports the span they imply.
readvalidates every packet and still raisesInvalidFiberFileError. Reads are byte-identical to before: verified by sha256 over the full data array, plus coords and attrs, on real files including the archive's unusual shapes.Testing
30 new tests, and
protobuf_utils.pyis at 100% line and branch coverage. The tests were shaped by mutation testing rather than written to the implementation — several early ones passed for the wrong reason (a fixture that silently skipped, an assertion against the constant under test, a check satisfied via the fallback path) and were rewritten to pin the behaviour they claim to.Checklist
I have (if applicable):