Sigil-Files #37

Open

benchinnery wants to merge 2 commits from sigil-info into main

pull from: sigil-info

merge into: qoherent:main

qoherent:main

qoherent:viewfix

qoherent:jonny-docs

qoherent:ria-agent-register-default-hub

qoherent:signal-view-fix

qoherent:agent-naming-fix

qoherent:zfp-oss

qoherent:dataset-manager-v2

Conversation 0 Commits 2 Files Changed 14 +755

benchinnery commented 2026-07-21 10:17:05 -04:00

Owner

Copy Link

The generated Sigil Syntax for OSS

The generated Sigil Syntax for OSS

benchinnery added 1 commit 2026-07-21 10:17:05 -04:00

sigil files a473d1bb8d

benchinnery added 1 commit 2026-07-28 10:27:31 -04:00

sigil 0.5 update 1a8bfb2c51

benchinnery changed title from WIP: Sigil-Files to Sigil-Files 2026-07-28 10:28:38 -04:00

G gillian requested review from gillian 2026-07-28 13:37:20 -04:00

G gillian requested changes 2026-07-28 13:47:27 -04:00

G gillian left a comment

Owner

Copy Link

Reviewed the logic in each .sigil file against the actual source and verified the causal/behavioral claims in every module's logic/constraints section. All findings below were independently reproduced against the code, not just inferred.

Approving the docs, but flagging four things below, two of which are real code issues the review surfaced rather than doc issues.

1. signal.sigil — HighLevelGenerators section describes broken functionality

PSKGenerator/QAMGenerator/PAMGenerator are documented as working (record(batch_size, num_bits)), but none of the three can even be constructed:

PSKGenerator(2, 4, RaisedCosineFilter())
QAMGenerator(4, 4, RaisedCosineFilter())
PAMGenerator(2, 4, RaisedCosineFilter())
# ValueError: Block 0 output type 4 does not match block 1 input type [<DataType.BITS: 4>]

Mapper.input_type returns a list-wrapped DataType; upstream blocks like BinarySource.output_type return the bare enum. The type-chain validation the doc credits as a correctness guarantee actually rejects the construction outright, for all three generators identically. No test exercises these classes, so it's silent today. This isn't a doc-wording issue — it's a real bug the doc review surfaced. Suggest filing it separately rather than blocking this PR on it.

2. agent.sigil — TX interlocks are scoped to one execution mode, not "the agent"

The doc states TX safety interlocks (--allow-tx, tx_max_gain_db, tx_max_duration_s, frequency bounds, watchdog) as a property of the agent generally. They're only enforced in the WebSocket streamer's TX path (streamer.py) — AgentConfig.tx_enabled/tx_max_gain_db/tx_max_duration_s/tx_allowed_freq_ranges are referenced nowhere else. The legacy long-poll NodeAgent → TxExecutor path (legacy_executor.py, orchestration/tx_executor.py) has none of them — no gain cap, no duration watchdog, no frequency bound, just a --role tx flag. Given this is RF transmit safety, worth either fixing the doc's scoping or (better) tightening the legacy path to match.

3. sdr.sigil — two driver-coverage overstatements

"USB/network drops are translated to SdrDisconnectedError" , only true for the Pluto driver. translate_disconnect is imported and called only in pluto.py; USRP, HackRF, Blade, RTLSDR, and ThinkRF never call it, so their disconnects propagate raw native exceptions instead.
Identifiers are documented as free-form per-device values including "HackRF/BladeRF serial" but both drivers' init explicitly warn and ignore any identifier passed in. There's no serial-based selection for them at all.

4. Minor wording nits (accurate in spirit, imprecise in detail)

data.sigil: RadioDataset reads are called "lazy" which is true for indexed access, but the bulk .data/.metadata accessors (f["data"][:], f["metadata/metadata"][:]) load the entire HDF5 file into memory.
io.sigil: SigMF is described as namespacing "the rest" of metadata under ria: it actually namespaces every key with no exclusion for ones already mapped to core SigMF keys, so those are duplicated rather than just the remainder covered. It's actually duplicated twice over: once via the ria:-prefixed keys, and again via a whole nested "ria" object (meta_dict["ria"] = metadata).

Everything else checked out exactly against source: root package constraints (numpy/scipy pins, SDR extras, license), the view/viz split, transforms' type/bounds contracts, orchestration's exact start-TX→record→stop-TX→label→QA→save sequence, the server's auth/deploy/inference behavior, the container app runner, the CLI command mapping, remote-control's SSH/ZMQ flow, and the core Recording/annotation/io behavior (including the previously-flagged save_recording and tx_cw latent bugs, which are correctly documented as-is).

Reviewed the logic in each `.sigil` file against the actual source and verified the causal/behavioral claims in every module's `logic`/`constraints` section. All findings below were independently reproduced against the code, not just inferred. Approving the docs, but flagging four things below, two of which are real code issues the review surfaced rather than doc issues. ## 1. signal.sigil — HighLevelGenerators section describes broken functionality `PSKGenerator`/`QAMGenerator`/`PAMGenerator` are documented as working (`record(batch_size, num_bits)`), but none of the three can even be constructed: ```python PSKGenerator(2, 4, RaisedCosineFilter()) QAMGenerator(4, 4, RaisedCosineFilter()) PAMGenerator(2, 4, RaisedCosineFilter()) # ValueError: Block 0 output type 4 does not match block 1 input type [<DataType.BITS: 4>] ``` Mapper.input_type returns a list-wrapped DataType; upstream blocks like BinarySource.output_type return the bare enum. The type-chain validation the doc credits as a correctness guarantee actually rejects the construction outright, for all three generators identically. No test exercises these classes, so it's silent today. This isn't a doc-wording issue — it's a real bug the doc review surfaced. Suggest filing it separately rather than blocking this PR on it. ## 2. agent.sigil — TX interlocks are scoped to one execution mode, not "the agent" The doc states TX safety interlocks (--allow-tx, tx_max_gain_db, tx_max_duration_s, frequency bounds, watchdog) as a property of the agent generally. They're only enforced in the WebSocket streamer's TX path (streamer.py) — AgentConfig.tx_enabled/tx_max_gain_db/tx_max_duration_s/tx_allowed_freq_ranges are referenced nowhere else. The legacy long-poll NodeAgent → TxExecutor path (legacy_executor.py, orchestration/tx_executor.py) has none of them — no gain cap, no duration watchdog, no frequency bound, just a --role tx flag. Given this is RF transmit safety, worth either fixing the doc's scoping or (better) tightening the legacy path to match. ## 3. sdr.sigil — two driver-coverage overstatements "USB/network drops are translated to SdrDisconnectedError" , only true for the Pluto driver. translate_disconnect is imported and called only in pluto.py; USRP, HackRF, Blade, RTLSDR, and ThinkRF never call it, so their disconnects propagate raw native exceptions instead. Identifiers are documented as free-form per-device values including "HackRF/BladeRF serial" but both drivers' __init__ explicitly warn and ignore any identifier passed in. There's no serial-based selection for them at all. ## 4. Minor wording nits (accurate in spirit, imprecise in detail) data.sigil: RadioDataset reads are called "lazy" which is true for indexed access, but the bulk .data/.metadata accessors (f["data"][:], f["metadata/metadata"][:]) load the entire HDF5 file into memory. io.sigil: SigMF is described as namespacing "the rest" of metadata under ria: it actually namespaces every key with no exclusion for ones already mapped to core SigMF keys, so those are duplicated rather than just the remainder covered. It's actually duplicated twice over: once via the ria:-prefixed keys, and again via a whole nested "ria" object (meta_dict["ria"] = metadata). Everything else checked out exactly against source: root package constraints (numpy/scipy pins, SDR extras, license), the view/viz split, transforms' type/bounds contracts, orchestration's exact start-TX→record→stop-TX→label→QA→save sequence, the server's auth/deploy/inference behavior, the container app runner, the CLI command mapping, remote-control's SSH/ZMQ flow, and the core Recording/annotation/io behavior (including the previously-flagged save_recording and tx_cw latent bugs, which are correctly documented as-is).

All checks were successful

Hide all checks

Build Sphinx Docs Set / Build Docs (pull_request) Successful in 22s

Details

Build Project / Build Project (3.10) (pull_request) Successful in 1m10s

Details

Build Project / Build Project (3.11) (pull_request) Successful in 1m5s

Details

Build Project / Build Project (3.12) (pull_request) Successful in 1m2s

Details

Test with tox / Test with tox (3.11) (pull_request) Successful in 9m17s

Details

Test with tox / Test with tox (3.10) (pull_request) Successful in 14m19s

Details

Test with tox / Test with tox (3.12) (pull_request) Successful in 8m56s

Details

This pull request doesn't have enough approvals yet. 0 of 1 approvals granted.

You are not authorized to merge this pull request.

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin sigil-info:sigil-info

git checkout sigil-info

Sign in to join this conversation.

Reviewers

No reviewers

Ggillian

Labels

Clear labels

  

CI/CD 🛠️

This issue relates to our automated testing, building, and publishing processes

  

WIP 🚧

Someone is currently worked on this issue

  

bug 🐞

There is a problem, error, or unexpected behaviour

  

code quality 🧹

This issues pertains to code quality improvements or tech debt reduction

  

data collection 🗃️

This issue involves machine learning data collection or filing

  

dependency 📦

This issue pertains to dependency changes or deprecation warnings

  

do not merge 🚫

This pull request is not to be merged

  

documentation 📝

Improvements or additions to documentation or tutorials

  

duplicate 🔄

This issue or pull request already exists

  

enhancement ✨

This issue is important and will require attention soon, but does not pose any immediate risks

  

good first issue 🌱

A good first issue for newcomers

  

help wanted 👋

Additional development support is required

  

inference 🧠

This issue involves designing or improving inference solutions

  

invalid 🛑

Something is wrong with this issue or it not does conform to project guidelines

  

machine learning 🚀

This issue pertains to and may require expertise in machine learning

  

priority

high🔥

This issue is critical and requires immediate attention to prevent further consequence or escalation

  

priority

low 🍃

  

priority

medium 🌟

This issue is important and will require attention soon, but does not pose any immediate risks

  

quantum ⚛️

This issue pertains to and may require expertise in quantum technology

  

question ❓

Additional information is required

  

radio 📡

This issue involves radio interface components and may requires access to radio hardware

  

sprintable 🏃‍♂️

This issue could be resolved in a single sprint

  

test 🧪

This issue pertains to our testing efforts

  

wont fix ❌

We've chosen not to address this issue

No Label

CI/CD 🛠️

WIP 🚧

bug 🐞

code quality 🧹

data collection 🗃️

dependency 📦

do not merge 🚫

documentation 📝

duplicate 🔄

enhancement ✨

good first issue 🌱

help wanted 👋

inference 🧠

invalid 🛑

machine learning 🚀

priority

high🔥

priority

low 🍃

priority

medium 🌟

quantum ⚛️

question ❓

radio 📡

sprintable 🏃‍♂️

test 🧪

wont fix ❌

Milestone

Clear milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

No Assignees

2 Participants

G

Notifications

Subscribe

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: qoherent/ria-toolkit-oss#37

No description provided.