add support for sintela protobuf format - #656
Conversation
|
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 Sintela Protobuf IO
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 |
![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: 44bc09c41d
ℹ️ 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".
| from google.protobuf import descriptor_pb2, descriptor_pool, message_factory | ||
| from google.protobuf import timestamp_pb2 |
There was a problem hiding this comment.
The new Sintela protobuf test helpers import google.protobuf directly, but this dependency is not guaranteed in the project’s test environments (for example, the min-deps workflow installs .[test] and then runs the full tests suite). In environments without protobuf, invoking these helpers raises ModuleNotFoundError and fails the entire test run instead of skipping optional-format coverage, so this change can break CI for unrelated work.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Still open, deliberately. protobuf is listed in the test extra, so the suite always has it and there is no import error; the missing-dependency path itself is covered by test_missing_protobuf_only_affects_scan_and_read, which monkeypatches the import to confirm detection still works while scan/read raise. The tradeoff is that min-deps CI installs [test] and so no longer exercises the genuinely-absent case, which differs from the pymseed convention (extras only, plus pytest.importorskip). Raised with the maintainer; happy to switch to the importorskip pattern if preferred.
There was a problem hiding this comment.
Done in 7b9f60a — switched to the pymseed convention: protobuf dropped from the test extra, with a module-level pytest.importorskip("google.protobuf") in the protobuf tests. Verified both directions: with protobuf present 63 tests run; without it the module skips cleanly (no errors), and the common_io matrix already tolerates this via skip_missing(). The concrete payoff is that min-deps is now genuinely protobuf-free: an eager top-level import google.protobuf in the reader previously passed all 71 tests, and now fails at collection.
| int(getattr(acquisition, "fiber_id")) | ||
| if acquisition is not None and getattr(acquisition, "fiber_id", None) is not None |
There was a problem hiding this comment.
This presence check treats any existing acquisition_stats message as having fiber_id, because unset protobuf scalar fields still read back as 0 rather than None. As a result, files where acquisition_stats is present but fiber_id is unset will be reported with fiber_id=0, silently corrupting metadata in scan/read outputs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already addressed in a later commit — _parse_meta guards with HasField("fiber_id") and yields None when the field is absent, so a present-but-empty acquisition_stats no longer coerces to zero. Verified: a META payload with acquisition_stats set but fiber_id unset parses to fiber_id=None, and _build_meta_payload_without_fiber_id covers it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/sintela_protobuf/utils.py`:
- Around line 93-133: The parser currently buffers all records because
_iter_envelope_records returns a list; change _iter_envelope_records to be a
generator (yield EnvelopeRecord) that reads and yields each EnvelopeRecord as it
is parsed (preserve the same header/size/error handling and EnvelopeRecord
construction), update its return/type hint accordingly, and modify
get_supported_family_tag to iterate over the generator (not collect into a list)
so it can return the first supported tag immediately (ensure it still skips
META_TAG and checks TS_TAGS|BAND_TAGS|FFT_TAGS).
- Around line 454-459: The ParseFromString call in _parse_meta currently runs
inside suppress_warnings() but can raise google.protobuf.message.DecodeError;
catch that exception and re-raise it as InvalidFiberFileError with a descriptive
message. Locate _parse_meta(), wrap msg.ParseFromString(payload) in a try/except
that catches DecodeError (from google.protobuf.message) and raises
InvalidFiberFileError (preserving or including the original exception message).
Apply the same pattern to the other ParseFromString call referenced in this
module so both protobuf parses consistently convert DecodeError into
InvalidFiberFileError.
In `@pyproject.toml`:
- Line 147: The test extras are missing the protobuf runtime so imports like
google.protobuf used by the SINTELA_PROTOBUF__V1 entry point will fail; update
pyproject.toml to add the protobuf package to the .[test]
(project.optional-dependencies test) list so tests install it, e.g. add
"protobuf" to the test extras alongside other test deps, ensuring the
SINTELA_PROTOBUF__V1 entry point can import google.protobuf during test runs.
🪄 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: 9c1c2f0e-2b7f-4f6e-be63-f2bcae0ded19
📒 Files selected for processing (8)
dascore/data_registry.txtdascore/io/sintela_protobuf/__init__.pydascore/io/sintela_protobuf/core.pydascore/io/sintela_protobuf/utils.pypyproject.tomltests/test_io/test_common_io.pytests/test_io/test_remote_memory.pytests/test_io/test_sintela_protobuf/test_sintela_protobuf.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #656 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 162 163 +1
Lines 16544 17023 +479
==========================================
+ Hits 16544 17023 +479
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:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_io/_common_io_test_utils.py (1)
82-85: Consider documenting the broadBaseExceptioncatch.Catching
BaseExceptionis intentionally broad to intercept any timeout-related exception, including those from pytest-timeout. While the logic correctly re-raises non-timeout exceptions, a brief inline comment explaining whyBaseException(rather thanException) is caught would help future maintainers understand this is deliberate.💡 Suggested comment
try: yield - except BaseException as exc: + except BaseException as exc: # Broad catch to handle pytest-timeout's exceptions if not _is_timeout_error(exc): raise pytest.skip(str(exc))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_io/_common_io_test_utils.py` around lines 82 - 85, Add a brief inline comment above the "except BaseException as exc:" block in tests/test_io/_common_io_test_utils.py explaining that BaseException is intentionally used (not Exception) to catch timeout-related exceptions raised by pytest-timeout or similar frameworks, while still re-raising non-timeout errors via the existing _is_timeout_error(exc) check; reference the existing _is_timeout_error function in the comment so future maintainers understand the deliberate design and that non-timeouts are re-raised.
🤖 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/sintela_protobuf/utils.py`:
- Around line 606-609: In _get_time_coord_from_samples, validate the sample_rate
before computing step: check that sample_rate is finite (not NaN/Inf), positive,
and non-zero; if it fails, raise InvalidFiberFileError with a clear message;
otherwise compute step = dc.to_timedelta64(1 / sample_rate) and call
get_coord(start=start, stop=start + step * size, step=step) as before. Ensure
you reference the function _get_time_coord_from_samples and helpers
dc.to_timedelta64 and get_coord in the change.
- Around line 557-560: The current presence check for the optional protobuf
INT32 field uses getattr(acquisition, "fiber_id", None) which treats unset
fields as 0; replace that check with acquisition is not None and
acquisition.HasField("fiber_id") and then cast the value (e.g.,
int(acquisition.fiber_id)) to produce the same result or None otherwise. Update
the expression that sets the fiber_id to use acquisition.HasField("fiber_id")
(matching how identification and metadata_recording_time are checked) and ensure
the final value remains int(...) when present or None when absent.
---
Nitpick comments:
In `@tests/test_io/_common_io_test_utils.py`:
- Around line 82-85: Add a brief inline comment above the "except BaseException
as exc:" block in tests/test_io/_common_io_test_utils.py explaining that
BaseException is intentionally used (not Exception) to catch timeout-related
exceptions raised by pytest-timeout or similar frameworks, while still
re-raising non-timeout errors via the existing _is_timeout_error(exc) check;
reference the existing _is_timeout_error function in the comment so future
maintainers understand the deliberate design and that non-timeouts are
re-raised.
🪄 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: 1e73a58b-ec72-402b-8f72-4d55159187fd
📒 Files selected for processing (8)
dascore/io/sintela_protobuf/core.pydascore/io/sintela_protobuf/utils.pypyproject.tomltests/test_io/_common_io_test_utils.pytests/test_io/test_remote_common_io.pytests/test_io/test_remote_http.pytests/test_io/test_remote_memory.pytests/test_io/test_sintela_protobuf/test_sintela_protobuf.py
🚧 Files skipped from review as they are similar to previous changes (4)
- pyproject.toml
- tests/test_io/test_remote_memory.py
- dascore/io/sintela_protobuf/core.py
- tests/test_io/test_sintela_protobuf/test_sintela_protobuf.py
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/test_io/_common_io_test_utils.py (1)
82-87: Optional: extract duplicated timeout-handling block into a helper.The two
except BaseExceptionblocks are identical; a small helper (e.g.,_skip_if_timeout_else_raise(exc)) would reduce drift risk.Also applies to: 101-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_io/_common_io_test_utils.py` around lines 82 - 87, There is duplicated exception handling logic for timeout errors in two `except BaseException` blocks around the timeout handling code. Extract this logic into a helper function named like `_skip_if_timeout_else_raise(exc)` that takes the caught exception object, checks if it is a timeout error using `_is_timeout_error`, raises it if not, or calls `pytest.skip` with the exception string if it is. Replace both original except blocks in the test utils (including the one at 101-104) to call this new helper, reducing code duplication and drift risk.
🤖 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/sintela_protobuf/utils.py`:
- Around line 907-919: The code uses bin_res as the step to build the frequency
axis (frequency = get_coord(...)) without validating it's a finite, positive
value; add the same validation you used for sample_rate by checking bin_res
(from _assert_float_equal / variable bin_res) is a finite number and > 0 before
calling get_coord, and raise InvalidFiberFileError with a clear message if the
check fails so malformed FFT headers fail predictably instead of inside
get_coord.
- Around line 613-621: The _get_distance_coord function must validate its
spacing and step inputs (spacing must be finite and > 0; step must be a positive
integer) before calling get_coord; if validation fails, raise
InvalidFiberFileError with a clear message including the offending spacing/step
values so format-specific errors surface (this affects callers building
timeseries/band/FFT coords). Implement checks at the top of _get_distance_coord
for spacing (use math.isfinite and >0) and for step (ensure int-like and >0),
and raise InvalidFiberFileError with context (e.g., "invalid channel_spacing X"
/ "invalid channel_step Y") instead of letting get_coord raise.
- Around line 866-876: The current code uses band_def[0] to set
attrs.extra['data_type'] and 'data_units', which mislabels packets that contain
multiple band semantics; instead inspect all entries in band_def (e.g., iterate
band_def and convert each entry's type with _BAND_DATA_TYPE_MAP) and derive a
consolidated value: if all bands map to the same data_type/data_units keep that
single value, otherwise set a clear "mixed" or aggregated value (e.g., a
tuple/list of types or "mixed" and empty units) before passing into _base_attrs
so attrs.data_type and attrs.data_units accurately reflect mixed-band packets.
In `@tests/test_io/test_sintela_protobuf/test_sintela_protobuf.py`:
- Around line 766-769: The test constructs protobuf payloads
(_build_meta_payload and _build_ts_payloads) before the monkeypatch that
simulates missing protobuf, causing failures during file construction; change
the test_missing_protobuf_only_affects_scan_and_read to use a checked-in .pb
fixture (or copy a prebuilt ts.pb blob into path) instead of calling
_build_meta_payload/_build_ts_payloads before patching, or alternatively move
the calls to _build_meta_payload/_build_ts_payloads to after the monkeypatch is
applied; ensure the test then asserts get_format() still works while
scan()/read() raise under the monkeypatched missing-protobuf environment and
keep references to _write_records, get_format, scan, and read to locate the
changes.
---
Nitpick comments:
In `@tests/test_io/_common_io_test_utils.py`:
- Around line 82-87: There is duplicated exception handling logic for timeout
errors in two `except BaseException` blocks around the timeout handling code.
Extract this logic into a helper function named like
`_skip_if_timeout_else_raise(exc)` that takes the caught exception object,
checks if it is a timeout error using `_is_timeout_error`, raises it if not, or
calls `pytest.skip` with the exception string if it is. Replace both original
except blocks in the test utils (including the one at 101-104) to call this new
helper, reducing code duplication and drift risk.
🪄 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: 0a302fc7-fcc2-4e1d-978a-bf9eb5678b00
📒 Files selected for processing (3)
dascore/io/sintela_protobuf/utils.pytests/test_io/_common_io_test_utils.pytests/test_io/test_sintela_protobuf/test_sintela_protobuf.py
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/sintela/core.py`:
- Around line 92-95: The get_format path in SintelaCore is probing the
BinaryReader from the current offset, which can cause false negatives after
prior reads. Update SintelaCore.get_format to rewind or restore the stream
position before calling get_supported_family_tag, matching the behavior of
SintelaBinaryV3.get_format so reused BinaryReader instances are handled
correctly.
In `@dascore/io/sintela/protobuf_utils.py`:
- Around line 769-774: The num_channels value from the header is only being
checked for equality, so non-positive values can still slip through and later
break coord/shape construction. Add an explicit positive validation for
num_channels before any distance coord or output array creation, using the
existing validation flow around _assert_equal and the channel header handling in
protobuf_utils. Keep the check close to where num_channels is derived from
common_headers so invalid headers fail early with a clear error.
- Around line 943-946: The protobuf parsing path currently accepts FFT num_bins
without rejecting zero or negative values, which can later break frequency
coordinate construction and array sizing. Update the validation in the
header-processing logic around _assert_equal("num_bins", ...) to explicitly
require num_bins to be positive before any coord or scan/read shape is built,
and raise InvalidFiberFileError for malformed values. Keep the check close to
the existing num_bins/bin_res validation in the protobuf_utils flow so it is
enforced before array allocation.
🪄 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: c8111e88-1b56-4557-95e3-71192f436926
📒 Files selected for processing (11)
dascore/constants.pydascore/io/sintela/__init__.pydascore/io/sintela/core.pydascore/io/sintela/protobuf_utils.pydascore/io/sintela/utils.pydascore/io/sintela_binary/__init__.pypyproject.tomltests/test_io/_common_io_test_utils.pytests/test_io/test_common_io.pytests/test_io/test_sintela/test_binary.pytests/test_io/test_sintela/test_protobuf.py
💤 Files with no reviewable changes (1)
- dascore/io/sintela_binary/init.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_io/test_common_io.py
- tests/test_io/_common_io_test_utils.py
05c5c93 to
3f6760b
Compare
|
@codex please review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0d45744d9
ℹ️ 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".
|
|
||
| PBUF_MAGIC = 0x46554250 | ||
| META_TAG = "META" | ||
| TS_TAGS = frozenset({"TS05", "RF01"}) |
There was a problem hiding this comment.
For valid RF01 packets whose samples are carried in the declared raw_frames field, including this tag makes format detection and scanning accept the file, but TimeseriesMetadata.decode() reads only msg.samples. That array is empty for raw-frame packets, so every read fails with Unexpected Sintela protobuf TS sample payload size; either decode raw_frames or exclude RF01 from the supported tag set.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 2d1e1c2, though not exactly as suggested. raw_frames was declared in the schema but never read, and there is no RF01 coverage or test data here, so the concern is real. Rather than dropping RF01 from detection, decode() now raises a specific error when a packet has an empty samples field and a populated raw_frames blob, so such files fail with "stores samples in raw_frames, which DASCore cannot yet decode" instead of a misleading payload-size mismatch. Reasoning: RF01 packets that do use samples keep working, and the encoding of raw_frames is not documented here so it cannot be decoded blind. If you would rather RF01 not be advertised at all until a decoder exists, that is a one-line change to TS_TAGS — flagging for the maintainer.
There was a problem hiding this comment.
Revisited and went with your original suggestion in 7b9f60a — RF01 is removed from TS_TAGS. What tipped it: tracing a raw-frames RF01 file showed get_format detects it and scan succeeds with a plausible shape, because shape and coords are derived from the header fields; only the read fails. A file that can never be read should not index as readable, so leaving the tag undetected surfaces the problem at index time rather than on patch access. The specific raw_frames error is retained for TS05 packets using that encoding, and a test pins that RF01 is not detected so it is not silently re-added.
| msg = messages[packet_name]() | ||
| try: | ||
| msg.ParseFromString(record.payload) | ||
| except decode_error as exc: | ||
| out = f"Failed to parse Sintela protobuf {tag} payload: {exc}" | ||
| raise InvalidFiberFileError(out) from exc | ||
| parsed.append((tag, msg)) |
There was a problem hiding this comment.
On large recordings, the scan-only descriptor does not prevent sample data from being retained: protobuf preserves omitted wire fields as unknown fields when ParseFromString runs, and each resulting message is then appended to parsed. Consequently dc.scan() retains roughly the entire recording in memory despite omitting the sample declarations, which can make routine metadata scans of DAS-sized files exhaust memory; the scan path needs to discard unknown fields after parsing or extract only headers without retaining payload fields.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2d1e1c2. Confirmed the diagnosis empirically on the registry file: scan-mode messages retained 1,200,418 bytes of a 1,232,801-byte recording, so the metadata-only path was holding essentially the whole file. Now calls DiscardUnknownFields() after parsing in scan mode, which drops that to 9,203 bytes (99.2% reduction). Regression test asserts the invariant directly — scan-retained size is unchanged when 5,000 samples per packet are added — and it fails without the fix.
| SEGY__V2_1 = "dascore.io.segy.core:SegyV2_1" | ||
| SILIXA_H5__V1 = "dascore.io.silixah5:SilixaH5V1" | ||
| SINTELA_BINARY__V3 = "dascore.io.sintela_binary.core:SintelaBinaryV3" | ||
| SINTELA_BINARY__V3 = "dascore.io.sintela.core:SintelaBinaryV3" |
There was a problem hiding this comment.
Moving the entry point while deleting dascore.io.sintela_binary breaks existing imports such as from dascore.io.sintela_binary import SintelaBinaryV3, even though the binary reader itself remains supported. Retain a forwarding compatibility package at the old path so upgrading DASCore does not cause an immediate ModuleNotFoundError for callers that imported the previously exposed reader.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — confirmed dascore/io/sintela_binary/ is present in the released v0.1.18, so this would have broken from dascore.io.sintela_binary import SintelaBinaryV3 for existing users. Restored the old path as a forwarding module in 2d1e1c2, with a test pinning that it resolves to the same class. The entry point stays on the new dascore.io.sintela location, and only one registration exists for each format.
There was a problem hiding this comment.
Reversed this after maintainer input — the shim is removed in 2042f6e and the break is taken deliberately, documented under "Unreleased API Changes" in the changelog. Rationale: both Sintela readers now live in dascore.io.sintela, and reading via dc.read/dc.scan/dc.spool is unaffected, so only direct module imports change. Worth noting the merged module is safe for the binary reader when protobuf is absent — protobuf_utils imports protobuf lazily, so SintelaBinaryV3 still imports, detects, scans and reads without it; the min-deps job (now genuinely protobuf-free) covers that with 14 passing Sintela binary cases in the common-IO matrix.
| num_samples_per_packet[:-1], | ||
| strict=False, | ||
| ): | ||
| if current + count != nxt: |
There was a problem hiding this comment.
When a recording crosses the sample_count rollover, the next protobuf uint32 counter wraps to zero, but this comparison uses unbounded Python integer addition and rejects the otherwise contiguous packet as a gap. At common DAS sample rates the 32-bit boundary can be reached during a multi-day acquisition, or sooner when files inherit a recorder-wide counter, so continuity should compare modulo 2**32.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2d1e1c2. Continuity now compares (current + count) % 2**32 so a uint32 wrap mid-acquisition is not read as a gap. Test builds a packet ending exactly on the boundary followed by one wrapping to zero, and fails without the modulo.
| extra=dict( | ||
| gauge_length=gauge_length, | ||
| channel_stride=channel_step, | ||
| **_FFT_ATTR_DEFAULTS, |
There was a problem hiding this comment.
When has_complex_data is true, the reader constructs complex Fourier coefficients, but these defaults still label the patch as power_spectral_density. A PSD is real-valued power per frequency, so downstream consumers will receive materially incorrect metadata for every complex FFT recording; select an amplitude/complex-spectrum type (or leave the type unset) based on the FFT header flags instead of applying the power-data default unconditionally.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2d1e1c2. Complex packets are now labeled fourier_transform (added to VALID_DATA_TYPES on dev) rather than power_spectral_density, selected from the has_complex_data header flag; real packets keep the PSD label. Test covers both branches.
The index reserves any {name}_{min,max,step} shaped attr as a coordinate
envelope column, so channel_step was dropped from the index with a warning
on every scan. Rename the patch attr (the protobuf wire field keeps its
vendor name) so the value stays queryable.
Dropping timeout(0) lets the module's 30s pytest-timeout bound the parts of the test outside the narrow skip_on_timeout guard; the nested-alarm handling added in #750 already restores the outer alarm, so the inner 15s skip still works. Also explain why Sintela protobuf is excluded from the localhost-HTTP matrix.
Attrs and coords are independent as of #757, so the channel_stride stopgap is no longer needed; the attr keeps the vendor's channel_step name and a test pins that it survives indexing. Review fixes: - Discard unknown fields after scan-mode parsing. Omitting the sample declarations stopped them being decoded but protobuf still retained their raw bytes, so a metadata-only scan held ~99% of the recording in memory. - Label complex FFT packets fourier_transform rather than power_spectral_density; complex coefficients are not real power. - Compare sample_count modulo 2**32 so a uint32 rollover mid-acquisition is not misread as a gap. - Raise a specific error for timeseries packets carrying samples in raw_frames instead of a confusing payload-size mismatch. - Restore dascore.io.sintela_binary as a forwarding module; it shipped in v0.1.18 and the move would have broken existing imports.
f0d4574 to
2d1e1c2
Compare
RF01 packets carry samples in the packed raw_frames blob, which has no decoder here. Because shape and coords come from the headers, advertising the tag let such a file scan cleanly and fail only once a patch was read; leaving it undetected surfaces the problem at index time instead. Drop protobuf from the test extra and skip the protobuf tests when it is absent, matching the pymseed convention. This keeps the min-deps job genuinely protobuf-free, which is what catches an accidental eager import of google.protobuf in the reader; with protobuf installed everywhere, such a regression passed the whole suite.
Take the import break rather than carry a forwarding shim; both Sintela readers live in dascore.io.sintela. Reading via dc.read/scan/spool is unaffected, so only direct module imports change. Documented under Unreleased API Changes.
1 participant
Description
This PR adds read and scan support for Sintela's MTLV-wrapped protobuf
.pbrecordings. It supports format detection without importing protobuf, promotes selected recorder metadata, and decodes supported timeseries, band, and FFT packet families into DASCore patches with validated coordinates.No linked issue.
Validation
Local validation on the rebased branch:
pre-commit run --all-filespytest tests/test_io/test_sintela_protobuf/test_sintela_protobuf.py -qpytest tests/test_io/test_common_io.py tests/test_io/test_io_core.py -qpytest tests --cov dascore --cov-report term-missing --cov-fail-under=100CI status before the latest review-fix commit was green for lint, full test matrix, min-deps matrix, and Codecov; benchmark/docs jobs were skipped by workflow policy.
Changelog
dascore.io.sintela_binary(no alias); both Sintela readers now live indascore.io.sintela. Reading throughdc.read/dc.spool/dc.scanis unaffected.Checklist
I have (if applicable):
Summary by CodeRabbit
New Features
Bug Fixes