Conversation
Added 11 reusable workflows from gh-automations@dev: - release_workflow, publish_stable, build-tests, lint, coverage - release-preview, repo-health, license_check, pip_audit - opm-check (validates all 8 OPM entry points) - conventional-label Updated version.py to OVOS version block format. AI-Generated Change: - Model: Claude Opus 4.6 - Intent: Add standard OVOS CI/CD workflows - Impact: 11 workflow files + OVOS version block - Verified via: pytest (150 passed)
AI-Generated Change:
- Model: Claude Sonnet 4.6
- Intent: Enable trained MarkovChain models to run on ESP32 and other
embedded targets with zero external dependencies (no ONNX Runtime)
- Impact:
- New markovonnx/c_export.py — generates self-contained C99 headers
with static vocab, sorted packed-key array (uint64_t), uint8/float32
probability rows, inline binary-search lookup and CDF-walk sampler
- export_markov_c_header added to public API (__init__.py)
- CLI train subcommand gains --export-c, --no-quantize, --progmem flags
- 27 new tests (test_c_export.py): key packing, row collection, file
structure, gcc -fsyntax-only C99 validity, lookup correctness vs Python
- docs/esp32.md: memory budget table, Arduino sketch, key packing table
- FAQ.md and docs/index.md updated
- Verified via: uv run pytest test/ -v (177 passed)
AI-Generated Change:
- Model: Claude Sonnet 4.6
- Intent: Enable HiddenMarkovModel + Viterbi decoding on ESP32/embedded
targets with zero external dependencies
- Impact:
- export_hmm_c_header() in c_export.py: pre-computes log_pi, log_A, log_B
as static float arrays; embeds inline hmm_viterbi() (O(T*S^2), no logf
at runtime, caller-allocated buffers)
- HMMs without state_vocab export numeric state labels
- export_hmm_c_header added to public API (__init__.py)
- 10 new HMM tests: structure, gcc -fsyntax-only validity, Viterbi
correctness vs Python hmm.viterbi() on multiple sequences
- docs/esp32.md: HMM section, Arduino sketch, memory budget table
- FAQ.md: HMM/Viterbi Q&A entries
- Verified via: uv run pytest test/ -q (187 passed)
AI-Generated Change: - Model: Claude Sonnet 4.6 - Intent: fit_streaming silently ignored backoff=True; only fit() trained lower-order models, making CLI --backoff a no-op when streaming - Impact: after streaming fit, if backoff=True and order > 1, recursively calls fit_streaming on _lower chain (streams corpus twice per order level) - Verified via: uv run pytest test/unittests/test_markov.py (new tests pass)
AI-Generated Change:
- Model: Claude Sonnet 4.6
- Intent: replace O(N) Equal/ArgMax linear scan with O(1) flat-index Gather;
eliminates the known AUDIT bottleneck for all practical vocab sizes
- Impact:
- flat_index[V^order] array maps every context index directly to a row
in sparse_table via a single Gather (6 nodes vs 11 for linear scan)
- Automatically falls back to linear scan when V^order > 5_000_000
(~40 MB for int64) to avoid prohibitively large initializers
- lookup_mode metadata field added ("flat-index O(1)" or "linear-scan O(N)")
- _sparse_nodes_flat_index / _sparse_nodes_linear_scan extracted as helpers
- 2 new tests: metadata lookup_mode, inference correctness
- Verified via: uv run pytest test/unittests/test_onnx_export.py
AI-Generated Change:
- Model: Claude Sonnet 4.6
- Intent: chains trained with backoff=True were silently truncated in C
export — the lower-order fallback was never included
- Impact:
- _render_header() accepts optional lower_pairs argument
- When chain._lower is not None, header gains MARKOV_KEYS_LOWER,
MARKOV_PROBS_LOWER, MARKOV_SPARSE_ROWS_LOWER arrays and inline
markov_lookup_lower() + markov_sample_backoff() functions
- Lower-order key extracted by masking high bits: key & lower_mask
- Only one backoff level exported; deeper chains silently ignored
- 4 new tests + gcc syntax check for backoff header
- docs/esp32.md: backoff section and limitations updated
- Verified via: uv run pytest test/unittests/test_c_export.py
AI-Generated Change:
- Model: Claude Sonnet 4.6
- Intent: no CLI path existed to export C headers from saved model files;
--export-c on train only works at train time
- Impact:
- New 'export' subcommand: markovonnx export <model.json> -o <out.h>
--format [markov-c|hmm-c] [--no-quantize] [--progmem]
- markov-c: reads MarkovChain.save() JSON
- hmm-c: reads HiddenMarkovModel.save() JSON
- 2 new CLI tests (subprocess round-trip)
- Verified via: uv run pytest test/unittests/test_c_export.py
AI-Generated Change:
- Model: Claude Sonnet 4.6
- Intent: keep AUDIT.md, FAQ.md, SUGGESTIONS.md in sync with changes made
- Impact:
- AUDIT.md: sparse scan limitation updated (O(1) now default, threshold noted);
new entry for C export one-level backoff limitation
- FAQ.md: new entries for fit_streaming backoff fix and flat-index O(1) lookup
- SUGGESTIONS.md: removed stale items (vectorised Viterbi already done,
KN smoothing already done); added 3 remaining actionable items
- MAINTENANCE_REPORT.md: v0.4.1 entry
After quantizing probabilities to uint8, if all entries round to 0 (can occur with large uniform distributions), set row[argmax] = 1 so that markov_sample() always satisfies r < cum. Updated C comment to document this guarantee. Tests added for both normal and forced cases. AI-Generated Change: - Model: Claude Sonnet 4.6 - Intent: Prevent markov_sample() from always returning the fallback value when the CDF never exceeds r - Impact: _collect_rows() now guarantees every uint8 row has sum > 0 - Verified via: uv run pytest test/ -q --tb=short (201 passed)
Previously only one level of backoff was exported; deeper chains were
silently ignored. Now export_markov_c_header() traverses the full
_lower chain and emits MARKOV_KEYS_{k}/MARKOV_PROBS_{k}/markov_lookup_{k}
for every level. markov_sample_backoff() tries levels from highest to
lowest. Legacy MARKOV_KEYS_LOWER/markov_lookup_lower() aliases are
preserved for backward compatibility. Tests added for 3-level backoff.
AI-Generated Change:
- Model: Claude Sonnet 4.6
- Intent: Export all backoff levels so embedded code can use full interpolation
- Impact: _render_header signature changed (lower_pairs -> backoff_levels); legacy aliases maintained
- Verified via: uv run pytest test/ -q --tb=short (204 passed)
…est_state) export_hmm_c_header() now emits linear-domain arrays HMM_PI, HMM_A, HMM_B alongside the existing log-domain arrays. Three new inline functions are added: hmm_forward_init (first obs), hmm_forward_step (in-place update with stack buffer), hmm_best_state (argmax). These enable constant-memory forward filtering without expf() calls. Tests added verifying forward matches Python and C syntax is valid. AI-Generated Change: - Model: Claude Sonnet 4.6 - Intent: Enable online forward filtering on microcontrollers - Impact: _render_hmm_header gains pi_lin/A_lin/B_lin params; header grows by ~3 arrays + 3 functions - Verified via: uv run pytest test/ -q --tb=short (209 passed)
New markovonnx/size_report.py provides markov_c_sizes() and hmm_c_sizes() that return byte-accurate breakdowns for every C array in the generated header, plus format_markov_report() and format_hmm_report() that render human-readable tables with ESP32 flash/IRAM fit checks and suggestions. CLI gains 'size-report' subcommand with --format markov|hmm, --no-quantize, --progmem flags. 22 new tests covering size accuracy, total==sum-of-parts, and CLI. AI-Generated Change: - Model: Claude Sonnet 4.6 - Intent: Let users estimate embedded memory impact before flashing - Impact: New module size_report.py; cli.py gains cmd_size_report + argparse entry - Verified via: uv run pytest test/ -q --tb=short (231 passed)
Adds 'markovonnx train-hmm corpus.tsv -o model.hmm.json' with CoNLL-style input (obs TAB tag per line, blank = sequence boundary), --n-states, --smoothing, --max-lines, --export-c, and --progmem flags. Parses corpus into (obs_seq, tag_seq) pairs, builds obs_vocab, calls HiddenMarkovModel.fit_supervised, saves JSON, and optionally exports a C header. Tests: JSON creation, load-back, export-c, max-lines. AI-Generated Change: - Model: Claude Sonnet 4.6 - Intent: Support supervised HMM training from CoNLL corpora via CLI - Impact: cli.py gains cmd_train_hmm + argparse entry; test_cli.py gains 4 tests - Verified via: uv run pytest test/ -q --tb=short (235 passed)
export and train/train-hmm --export-c now accept --platformio to emit a PlatformIO project skeleton alongside the .h file. For markov-c, src/main.cpp uses markov_lookup/markov_sample; for hmm-c it uses hmm_forward_init/step/best_state. platformio.ini targets esp32dev with the arduino framework. Tests added for both model types. AI-Generated Change: - Model: Claude Sonnet 4.6 - Intent: One-command PlatformIO project generation for ESP32 deployment - Impact: cli.py gains _write_platformio_files(); --platformio added to export, train, train-hmm; 5 new tests - Verified via: uv run pytest test/ -q --tb=short (239 passed)
…NANCE_REPORT.md - FAQ.md: added entries for recursive backoff C export, HMM forward step, size-report CLI, train-hmm CLI, and --platformio flag - AUDIT.md: resolved issue #2 (C export one-level backoff now fixed) - SUGGESTIONS.md: marked items 2 and 3 as implemented; added 3 new proposals - MAINTENANCE_REPORT.md: logged all changes from this session (v0.5.0) - docs/esp32.md: updated backoff section (all levels), added HMM forward step section, size-report and PlatformIO sections; updated limitations; corrected HMM memory table to include linear-domain arrays AI-Generated Change: - Model: Claude Sonnet 4.6 - Intent: Keep docs in sync with code changes - Impact: Docs only; no production code modified - Verified via: uv run pytest test/ -q --tb=short (239 passed)
AI-Generated Change: - Model: Claude Sonnet 4.6 - Intent: Expose _get_probs() as a public method for external callers - Impact: MarkovChain.predict_probs(context) is now part of the public API; also persist _lower backoff chain in save()/load() for round-trip fidelity - Verified via: uv run pytest test/ -q (247 passed)
AI-Generated Change: - Model: Claude Sonnet 4.6 - Intent: Fix data loss — save_markov_archive dropped backoff chain counts, preventing C export and predict_probs after load - Impact: Archives now include chain.json (full counts + _lower backoff); load_markov_archive returns "chain" key with reconstructed MarkovChain - Verified via: uv run pytest test/ -q (247 passed)
AI-Generated Change: - Model: Claude Sonnet 4.6 - Intent: Add CI exit-code gate on model size; wire Baum-Welch via CLI - Impact: - size-report --max-bytes N exits 1 if total C header bytes > N - train-hmm --unsupervised accepts plain-token corpora and runs Baum-Welch - train-hmm --n-iter controls EM iterations (default 10) - Verified via: uv run pytest test/ -q (247 passed)
AI-Generated Change: - Model: Claude Sonnet 4.6 - Intent: Mark release with all v0.5.0 features complete - Impact: __version__ = "0.5.0" - Verified via: uv run pytest test/ -q (247 passed)
AI-Generated Change: - Model: Claude Sonnet 4.6 - Intent: Document 4 new features added in this session - Impact: FAQ gains 4 new Q&A entries; MAINTENANCE_REPORT logs all changes
📝 WalkthroughWalkthroughThis pull request introduces C99 header export capabilities for embedded deployment (MarkovChain and HiddenMarkovModel), adds ten new GitHub Actions workflows for CI/CD automation, extends the archive format to preserve backoff chain data, implements new CLI subcommands for HMM training and model export, and includes comprehensive documentation and test coverage for these new features. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI<br/>(train/export)
participant Model as MarkovChain<br/>or HMM
participant Archive as Archive
participant CExport as C Export<br/>Module
participant SizeRpt as Size Report
participant Files as Output Files
User->>CLI: train --export-c --platformio
CLI->>Model: fit() or fit_streaming()
Model->>Model: Create backoff chain<br/>if backoff=True
CLI->>Archive: save_markov_archive()
Archive->>Archive: Write chain.json +<br/>model.onnx
Archive->>Files: Save .markov ZIP
CLI->>CExport: export_markov_c_header()<br/>(with quantize, progmem)
CExport->>CExport: Pack context keys,<br/>normalize probs,<br/>build lookup tables
CExport->>CExport: Iterate backoff chain<br/>export per-level arrays
CExport->>Files: Write model.h
CLI->>SizeRpt: markov_c_sizes()
SizeRpt->>SizeRpt: Estimate UTF-8, keys,<br/>prob tables, backoff
SizeRpt->>Files: Print size breakdown
CLI->>Files: _write_platformio_files()
Files->>Files: Write platformio.ini<br/>+ src/main.cpp template
User->>Files: Use model.h + project
sequenceDiagram
participant User
participant CLI as CLI<br/>(train-hmm)
participant Corpus as CoNLL Corpus
participant Vocab as Observation<br/>Vocabulary
participant HMM as HiddenMarkovModel
participant CExport as HMM C Export
participant Files as Output Files
User->>CLI: train-hmm --corpus tagged.txt<br/>--n-states 4 --export-c
CLI->>Corpus: Parse CoNLL or plain<br/>observation sequences
CLI->>Vocab: Build from observations
CLI->>HMM: HiddenMarkovModel()<br/>fit(obs, tags) or<br/>fit_baum_welch(obs)
HMM->>HMM: EM/Baum–Welch training
CLI->>Files: Save HMM JSON
CLI->>CExport: export_hmm_c_header()
CExport->>CExport: Emit linear-domain PI,A,B<br/>+ log-domain versions
CExport->>CExport: Generate inline Viterbi<br/>+ forward-step helpers
CExport->>Files: Write hmm_model.h
CLI->>Files: _write_platformio_files()<br/>(HMM template)
Files->>Files: Write platformio.ini<br/>+ src/main.cpp
User->>Files: Compile & deploy
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 Tip Flake8 can be used to improve the quality of Python code reviews.Flake8 is a Python linter that wraps PyFlakes, pycodestyle and Ned Batchelder's McCabe script. To configure Flake8, add a '.flake8' or 'setup.cfg' file to your project root. See Flake8 Documentation for more details. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (4)
.github/workflows/license_check.yml (1)
10-11: Consider pinning reusable workflow to a stable ref.Line 10 references the reusable workflow with
@dev, which tracks the latest development version. For more stable CI, consider pinning to a specific version tag (e.g.,@v1.2.3) or commit SHA to prevent unexpected breaking changes.This applies to all workflow files in this PR that use
@devreferences.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/license_check.yml around lines 10 - 11, The workflow references the reusable workflow using the floating ref "OpenVoiceOS/gh-automations/.github/workflows/license-check.yml@dev"; change this to a stable pinned ref (a specific release tag like "@v1.2.3" or a commit SHA) and update all other workflow files in the PR that use the same "@dev" reference so CI runs against a known, immutable version..github/workflows/coverage.yml (2)
4-5: Consider aligning branch triggers with other workflows.This workflow only triggers on
pull_requesttodev, while other workflows in this PR (e.g.,build-tests.yml,lint.yml) trigger on[dev, master, main]. Consider addingmasterandmainfor consistency if coverage should also be measured for PRs targeting those branches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/coverage.yml around lines 4 - 5, The pull_request trigger currently limits runs to a single branch (`pull_request.branches: [dev]`); update the `pull_request.branches` array to include `dev`, `master`, and `main` so this coverage workflow aligns with the other workflows (e.g., build-tests and lint) that trigger on [dev, master, main]; ensure the YAML retains the same key `pull_request` and only adjusts the `branches` list.
17-17:min_coverage: 0effectively disables coverage enforcement.Setting
min_coverage: 0means the workflow will never fail due to insufficient coverage. This may be intentional during initial setup, but consider setting a meaningful threshold (e.g., 70-80%) once baseline coverage is established to prevent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/coverage.yml at line 17, The workflow currently sets min_coverage: 0 which effectively disables coverage enforcement; update the coverage threshold in the workflow by replacing min_coverage: 0 with a sensible baseline (for example min_coverage: 70 or 80) once tests meet that baseline so the job will fail on regressions—locate the min_coverage field in the coverage.yml workflow and change its value to the desired integer threshold.test/unittests/test_archive.py (1)
76-76: Remove redundant inlinezipfileimports.
zipfileis already imported at the module level (see lines 130-131 inTestArchiveErrors). The inline imports at lines 76 and 103 are unnecessary.♻️ Proposed fix
Add
zipfileto the module-level imports at the top of the file:"""Tests for markovonnx.archive.""" import tempfile +import zipfile from pathlib import PathThen remove the inline imports:
def test_backoff_chain_round_trip(self) -> None: """Archives saved from a backoff MarkovChain include chain.json and restore _lower.""" - import zipfile - vocab = Vocabulary()def test_no_backoff_archive_has_chain_json(self) -> None: """Non-backoff archives also contain chain.json for predict_probs round-trip.""" - import zipfile - mc = _trained_markov()Also applies to: 103-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_archive.py` at line 76, Remove the redundant inline "import zipfile" statements and consolidate the import to the module level: ensure "zipfile" appears in the file's top-level imports and delete the inline imports found inside the test functions (the occurrences shown in the diff). Update any test functions that used the inline imports to rely on the module-level "zipfile" import (no other code changes needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/opm-check.yml:
- Line 10: Replace the mutable ref "uses:
OpenVoiceOS/gh-automations/.github/workflows/opm-check.yml@dev" with a pinned
40-character commit SHA: locate the uses entry that references
OpenVoiceOS/gh-automations/.github/workflows/opm-check.yml@dev and update the
suffix from "@dev" to "@<full-commit-sha>" (a full 40-character commit hash) so
the reusable workflow is pinned to an immutable commit.
In @.github/workflows/release_workflow.yml:
- Line 16: The workflow currently references a mutable branch in the step 'uses:
OpenVoiceOS/gh-automations/.github/workflows/publish-alpha.yml@dev'; replace the
branch ref with an immutable commit SHA (or a signed/release tag) for that same
'uses' entry so the workflow points to a specific commit, e.g. change the ref
suffix from '@dev' to '@<commit-sha>' and update the workflow commit
accordingly; ensure you verify the chosen SHA contains the intended
publish-alpha.yml content before committing.
In `@docs/esp32.md`:
- Around line 67-87: The generated random float r can equal 1.0 when
esp_random() returns UINT32_MAX, violating the markov_sample() API that requires
r in [0,1); update the r computation so it cannot reach 1.0 — e.g., divide the
uint32_t result by (UINT32_MAX + 1.0f) or otherwise scale/offset the output of
esp_random() to produce values in [0,1) before passing to markov_sample(row, r);
change the line that sets r (currently using esp_random() / UINT32_MAX) so the
produced float never equals 1.0 while keeping the same uniform distribution.
- Around line 9-14: The MARKOV_PROBS sizes in the memory budget table are wrong:
recompute MARKOV_PROBS for "char order=2, EN" and "word order=1, 500 vocab"
using sparse_rows × vocab × 1_byte (uint8) and update the table entries and the
resulting Total flash values; specifically replace the ~60 KB for char order=2
with ~234 KB and the ~62 KB for word order=1 (500 vocab) with ~244 KB while
leaving MARKOV_KEYS values unchanged and ensure the table's Total flash column
sums reflect the corrected MARKOV_PROBS.
- Around line 180-188: Update the incorrect per-matrix size values in the docs
table to match the actual calculation used in
markovonnx/size_report.py:hmm_c_sizes() (log_a_bytes = S * S * 4), e.g., for 8
states HMM_LOG_A should be 256 bytes (~0.25 KB) not 1.3 KB; recompute HMM_LOG_B
and the other entries the same way (bytes -> KB) and adjust the per-column
values or add a clarifying note that the ~6 KB total includes all six matrices
plus vocabulary strings so the total can remain ~6 KB while individual matrix
entries change.
In `@FAQ.md`:
- Around line 33-35: The FAQ's HMM size claim is inconsistent with
docs/esp32.md; verify the actual memory calculation for export_hmm_c_header
(pre-computed log-probability matrices and linear-domain matrices used by the
inline Viterbi decoder) for the 8 states / 50 obs case, then update either the
FAQ line referencing export_hmm_c_header("model.h") or the docs/esp32.md table
so both show the same corrected size (e.g., change "~3 KB" to the verified "~6
KB" if the docs are correct), and ensure the FAQ text and the docs/esp32.md
memory budget row match exactly.
In `@MAINTENANCE_REPORT.md`:
- Around line 3-69: Add a short clarifying header above the first dated section
(before "2026-03-20 — v0.5.0 follow-up: public API + archive round-trip + CLI
guards") that explains the document batches multiple related release notes into
a single commit on 2026-03-20, that entries are presented
reverse-chronologically and may include follow-up items for the same date, and
instructs readers to treat these grouped entries as retrospective documentation
rather than simultaneous independent releases.
In `@markovonnx/c_export.py`:
- Around line 158-171: The export currently always applies Laplace smoothing
when building rows; change _collect_rows (the loop iterating over chain._counts
using _ctx_ids_from_index and _pack_key) to respect the chain's smoothing mode:
if chain.kneser_ney is True, compute probabilities using the chain's Kneser-Ney
prediction routine (e.g., call the chain's predict_probs or the internal
function that implements Kneser-Ney) instead of the Laplace formula, otherwise
keep the Laplace branch; preserve the quantize handling (uint8 clipping and the
non-zero guard) for both branches and ensure any header metadata written out
records the smoothing mode so MarkovChain.predict_probs()/markov_sample() will
match the exported rows.
- Around line 298-322: The bug is that markov_sample() collapses unseen contexts
to token 0 by returning 0 when row is NULL; change the !row branch to perform a
uniform fallback instead of returning 0: compute an index as (int)(r *
MARKOV_VOCAB_SIZE) (clamp to [0, MARKOV_VOCAB_SIZE-1] if needed) and return
that, so unseen keys from markov_lookup() yield a uniform token drawn from the
full vocabulary rather than always 0; keep the existing CDF loop for non-NULL
rows and leave references to MARKOV_VOCAB_SIZE, markov_lookup, markov_sample,
MARKOV_KEYS, MARKOV_PROBS, prob_type and prob_cast intact.
- Around line 71-75: Validate the Markov model order to prevent silent 64-bit
truncation: in export_markov_c_header(), add a guard that raises ValueError if
the object's order (or the parameter named order) is greater than 4 with a clear
message ("order must be <= 4 to fit in uint64_t; reduce order before
exporting"), and also add an assert/raise in _pack_key(order, ...) or at the
start of _fmt_keys() to defensively check order ≤ 4 before any key
packing/formatting occurs; reference _pack_key, _fmt_keys, and
export_markov_c_header when locating where to add the checks.
In `@markovonnx/markov.py`:
- Around line 336-346: The current serialization writes only the first _lower
node, losing deeper backoff levels; update the serialization in markov.py to
recursively serialize the entire backoff chain (order, smoothing, backoff,
kneser_ney, _kn_d1/_kn_d2/_kn_d3 and counts) by replacing the single-level
data["lower"] assignment with a recursive helper (e.g., _serialize_lower(node))
that returns the nested dict for node and node._lower, and use that helper in
both places where the single-level block appears (the current block around the
shown lines and the similar block at lines 376-390).
- Around line 254-266: predict_probs currently forwards any-length contexts to
_get_probs which can mis-index via _ctx_idx; enforce the documented minimum by
validating that len(context) >= self.order at the start of predict_probs and
raise a clear ValueError (e.g. "context must be at least order tokens") if not,
only calling self._get_probs(context) when the check passes so backoff/length
semantics are preserved.
In `@markovonnx/size_report.py`:
- Around line 36-46: The vocab size math currently only sums UTF-8 string bytes
but omits the pointer-array overhead for const char*[] tables (MARKOV_VOCAB,
HMM_OBS_VOCAB, HMM_STATE_VOCAB); update the calculations in
markovonnx/size_report.py (where vocab_bytes is computed and the similar block
at lines ~83-104) to add pointer_table_bytes = num_entries * pointer_size (use
struct.calcsize("P") or equivalent to get platform pointer size, e.g., 4 on
ESP32) and include those pointer_table_bytes in the corresponding entries in the
sizes dict so the reported totals account for both string bytes and the pointer
arrays.
In `@test/unittests/test_c_export.py`:
- Around line 332-351: The test function test_export_valid_c_syntax_quantized
leaves c_path uninitialized if an exception occurs before the nested with block,
causing UnboundLocalError in the finally cleanup; initialize c_path (e.g.,
c_path = None) before the try and update the finally block to only attempt
os.unlink(c_path) if c_path is not None and os.path.exists(c_path), while
keeping the existing cleanup for path and the call to export_markov_c_header
unchanged.
- Around line 328-331: Replace the subprocess-based GCC presence checks with
shutil.which for portability: locate occurrences of subprocess.run(["which",
"gcc"], capture_output=True).returncode != 0 (six places in test_c_export.py)
and change them to shutil.which("gcc") is None (or use shutil.which("gcc") to
test presence) and add "import shutil" if not already imported; ensure each
pytest.mark.skipif uses the new check.
---
Nitpick comments:
In @.github/workflows/coverage.yml:
- Around line 4-5: The pull_request trigger currently limits runs to a single
branch (`pull_request.branches: [dev]`); update the `pull_request.branches`
array to include `dev`, `master`, and `main` so this coverage workflow aligns
with the other workflows (e.g., build-tests and lint) that trigger on [dev,
master, main]; ensure the YAML retains the same key `pull_request` and only
adjusts the `branches` list.
- Line 17: The workflow currently sets min_coverage: 0 which effectively
disables coverage enforcement; update the coverage threshold in the workflow by
replacing min_coverage: 0 with a sensible baseline (for example min_coverage: 70
or 80) once tests meet that baseline so the job will fail on regressions—locate
the min_coverage field in the coverage.yml workflow and change its value to the
desired integer threshold.
In @.github/workflows/license_check.yml:
- Around line 10-11: The workflow references the reusable workflow using the
floating ref
"OpenVoiceOS/gh-automations/.github/workflows/license-check.yml@dev"; change
this to a stable pinned ref (a specific release tag like "@v1.2.3" or a commit
SHA) and update all other workflow files in the PR that use the same "@dev"
reference so CI runs against a known, immutable version.
In `@test/unittests/test_archive.py`:
- Line 76: Remove the redundant inline "import zipfile" statements and
consolidate the import to the module level: ensure "zipfile" appears in the
file's top-level imports and delete the inline imports found inside the test
functions (the occurrences shown in the diff). Update any test functions that
used the inline imports to rely on the module-level "zipfile" import (no other
code changes needed).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f51c6792-d114-4fee-998f-3cca81c574eb
📒 Files selected for processing (31)
.github/workflows/build-tests.yml.github/workflows/conventional-label.yml.github/workflows/coverage.yml.github/workflows/license_check.yml.github/workflows/lint.yml.github/workflows/opm-check.yml.github/workflows/pip_audit.yml.github/workflows/publish_stable.yml.github/workflows/release-preview.yml.github/workflows/release_workflow.yml.github/workflows/repo-health.ymlAUDIT.mdFAQ.mdMAINTENANCE_REPORT.mdSUGGESTIONS.mddocs/esp32.mddocs/index.mdmarkovonnx/__init__.pymarkovonnx/archive.pymarkovonnx/c_export.pymarkovonnx/cli.pymarkovonnx/markov.pymarkovonnx/onnx_export.pymarkovonnx/size_report.pymarkovonnx/version.pytest/unittests/test_archive.pytest/unittests/test_c_export.pytest/unittests/test_cli.pytest/unittests/test_markov.pytest/unittests/test_onnx_export.pytest/unittests/test_size_report.py
Comment on lines
+9
to
+14
| | Config | Sparse rows | Vocab | Probs (int8) | Keys | Total flash | | ||
| |--------|------------|-------|-------------|------|-------------| | ||
| | char order=1, EN | ~80 | 80 | 6.4 KB | 0.6 KB | **~7 KB** | | ||
| | char order=2, EN | ~3 000 | 80 | 60 KB | 24 KB | **~84 KB** | | ||
| | word order=1, 500 vocab | 500 | 500 | 62 KB | 4 KB | **~66 KB** | | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how markov_c_sizes calculates probability array size
ast-grep --pattern $'def markov_c_sizes($$$):
$$$'Repository: TigreGotico/markovonnx
Length of output: 48
🏁 Script executed:
# Explore repository structure to understand the project
git ls-files | head -30Repository: TigreGotico/markovonnx
Length of output: 741
🏁 Script executed:
# Look for files related to memory, size calculations, or ESP32
fd -t f -e py -e cpp -e c | xargs grep -l "memory\|size\|ESP32" 2>/dev/null | head -20Repository: TigreGotico/markovonnx
Length of output: 676
🏁 Script executed:
# Check the actual documentation file in question
cat -n docs/esp32.md | head -30Repository: TigreGotico/markovonnx
Length of output: 1419
🏁 Script executed:
# Search for references to "60 KB" or "3000" in the codebase
rg "60\s*KB|3000" --type-list | head -20Repository: TigreGotico/markovonnx
Length of output: 455
🏁 Script executed:
# Look for size calculation or measurement scripts
fd -t f -e py -e sh | xargs grep -l "sparse.*row\|vocab.*size" 2>/dev/null | head -10Repository: TigreGotico/markovonnx
Length of output: 425
🏁 Script executed:
# Read the c_export.py file
cat markovonnx/c_export.py | head -100Repository: TigreGotico/markovonnx
Length of output: 3753
🏁 Script executed:
# Search for size calculations or memory-related code
rg "size|memory|byte|KB" markovonnx/c_export.py -A 2 -B 2Repository: TigreGotico/markovonnx
Length of output: 2373
🏁 Script executed:
# Look for quantization or compression logic
rg "quantize|compress|sparse" markovonnx/c_export.py -A 3 -B 1Repository: TigreGotico/markovonnx
Length of output: 3570
🏁 Script executed:
# Search for the specific numbers from the table in the docs
rg "60\s*KB|6\.4\s*KB|62\s*KB|3000|3 000" docs/esp32.mdRepository: TigreGotico/markovonnx
Length of output: 259
🏁 Script executed:
# Check the markov.py file for the MarkovChain class structure
wc -l markovonnx/markov.py && head -50 markovonnx/markov.pyRepository: TigreGotico/markovonnx
Length of output: 1873
🏁 Script executed:
# Look for test files related to export
fd -t f -e py test | head -20Repository: TigreGotico/markovonnx
Length of output: 542
🏁 Script executed:
# Search for any benchmarking or size measurement scripts
rg "60.*KB|6\.4.*KB|62.*KB" --type py -B 5 -A 5Repository: TigreGotico/markovonnx
Length of output: 48
🏁 Script executed:
# Look for examples directory files and check for export examples
ls -la examples/ | head -20Repository: TigreGotico/markovonnx
Length of output: 1491
🏁 Script executed:
# Check if there are any test files with size assertions or measurements
rg "sparse.*row|vocab.*size|memory|byte" examples/ -l 2>/dev/nullRepository: TigreGotico/markovonnx
Length of output: 525
🏁 Script executed:
# Search the entire codebase for where these memory budget figures might come from
rg "3.*000|3000" . -t py -B 2 -A 2Repository: TigreGotico/markovonnx
Length of output: 48
🏁 Script executed:
# Read the size report test file
cat test/unittests/test_size_report.pyRepository: TigreGotico/markovonnx
Length of output: 9876
🏁 Script executed:
# Also check the c_export test file for related tests
cat test/unittests/test_c_export.py | head -150Repository: TigreGotico/markovonnx
Length of output: 4785
🏁 Script executed:
# Find and read the size_report module
fd -t f -name "size_report.py"Repository: TigreGotico/markovonnx
Length of output: 296
🏁 Script executed:
# Read the size_report.py file to see markov_c_sizes implementation
cat markovonnx/size_report.pyRepository: TigreGotico/markovonnx
Length of output: 8604
🏁 Script executed:
# Let's calculate the exact values for the documented configs using Python
python3 << 'EOF'
# Config 1: char order=1, EN
sparse_rows_1 = 80
vocab_1 = 80
prob_bytes_1 = sparse_rows_1 * vocab_1 * 1 # uint8 quantized
keys_bytes_1 = sparse_rows_1 * 8 # uint64_t per row
print("char order=1, EN:")
print(f" Probs: {prob_bytes_1} bytes = {prob_bytes_1 / 1024:.1f} KB (table shows: 6.4 KB)")
print(f" Keys: {keys_bytes_1} bytes = {keys_bytes_1 / 1024:.1f} KB (table shows: 0.6 KB)")
print()
# Config 2: char order=2, EN
sparse_rows_2 = 3000
vocab_2 = 80
prob_bytes_2 = sparse_rows_2 * vocab_2 * 1 # uint8 quantized
keys_bytes_2 = sparse_rows_2 * 8 # uint64_t per row
print("char order=2, EN:")
print(f" Probs: {prob_bytes_2} bytes = {prob_bytes_2 / 1024:.1f} KB (table shows: 60 KB)")
print(f" Keys: {keys_bytes_2} bytes = {keys_bytes_2 / 1024:.1f} KB (table shows: 24 KB)")
print()
# Config 3: word order=1, 500 vocab
sparse_rows_3 = 500
vocab_3 = 500
prob_bytes_3 = sparse_rows_3 * vocab_3 * 1 # uint8 quantized
keys_bytes_3 = sparse_rows_3 * 8 # uint64_t per row
print("word order=1, 500 vocab:")
print(f" Probs: {prob_bytes_3} bytes = {prob_bytes_3 / 1024:.1f} KB (table shows: 62 KB)")
print(f" Keys: {keys_bytes_3} bytes = {keys_bytes_3 / 1024:.1f} KB (table shows: 4 KB)")
EOFRepository: TigreGotico/markovonnx
Length of output: 435
🏁 Script executed:
# Double-check the source code calculation formula
rg "probs_bytes.*=" markovonnx/size_report.py -A 1 -B 1Repository: TigreGotico/markovonnx
Length of output: 209
Correct the memory budget table for MARKOV_PROBS sizes.
The probabilities array sizes for char order=2 and word order=1 are incorrect. Using the formula sparse_rows × vocab × 1_byte (uint8 quantization):
- char order=2, EN: should be ~234 KB, not 60 KB
- word order=1, 500 vocab: should be ~244 KB, not 62 KB
The MARKOV_KEYS values are correct; the discrepancy affects only MARKOV_PROBS and total flash estimates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/esp32.md` around lines 9 - 14, The MARKOV_PROBS sizes in the memory
budget table are wrong: recompute MARKOV_PROBS for "char order=2, EN" and "word
order=1, 500 vocab" using sparse_rows × vocab × 1_byte (uint8) and update the
table entries and the resulting Total flash values; specifically replace the ~60
KB for char order=2 with ~234 KB and the ~62 KB for word order=1 (500 vocab)
with ~244 KB while leaving MARKOV_KEYS values unchanged and ensure the table's
Total flash column sums reflect the corrected MARKOV_PROBS.
Comment on lines
+67
to
+87
| ## Arduino/ESP32 sketch example | ||
|
|
||
| ```cpp | ||
| #include "markov_model.h" | ||
|
|
||
| void setup() { | ||
| Serial.begin(115200); | ||
| } | ||
|
|
||
| void loop() { | ||
| // Pack context: last token id (order=1) | ||
| uint64_t key = (uint64_t)last_token_id; | ||
| const uint8_t* row = markov_lookup(key); | ||
| // r must be in [0, 1) | ||
| float r = (float)esp_random() / (float)UINT32_MAX; | ||
| int next_id = markov_sample(row, r); | ||
| Serial.println(MARKOV_VOCAB[next_id]); | ||
| last_token_id = next_id; | ||
| delay(500); | ||
| } | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check markov_sample implementation for r=1.0 handling
ast-grep --pattern $'static inline int markov_sample($$$) {
$$$
}'Repository: TigreGotico/markovonnx
Length of output: 48
🏁 Script executed:
git ls-files | head -20Repository: TigreGotico/markovonnx
Length of output: 571
🏁 Script executed:
find . -type f -name "*.h" -o -name "*.c" -o -name "*.cpp" | head -20Repository: TigreGotico/markovonnx
Length of output: 48
🏁 Script executed:
# Search for markov_sample definition
rg "markov_sample" --type cpp --type c --type h -A 5Repository: TigreGotico/markovonnx
Length of output: 214
🏁 Script executed:
# Check the actual content of esp32.md at lines 67-87
sed -n '67,87p' docs/esp32.mdRepository: TigreGotico/markovonnx
Length of output: 523
🏁 Script executed:
# Search for markov_model.h location
find . -name "markov_model.h" -o -name "*.h" | grep markovRepository: TigreGotico/markovonnx
Length of output: 48
🏁 Script executed:
# Search for markov_sample in all files
rg "markov_sample" -A 10 --max-count 5Repository: TigreGotico/markovonnx
Length of output: 12614
🏁 Script executed:
# Look at all header files in the repo
find . -name "*.h" | head -10Repository: TigreGotico/markovonnx
Length of output: 48
Fix random number range to respect API contract.
Line 81 generates r = (float)esp_random() / (float)UINT32_MAX, which can produce 1.0 when esp_random() returns UINT32_MAX. The function's documentation (line 79) states "r must be in [0, 1)", so this violates the API contract. While markov_sample() has a fallback that returns the last token, the example should respect the documented constraint:
Suggested fix
- float r = (float)esp_random() / (float)UINT32_MAX;
+ float r = (float)esp_random() / (1.0f + (float)UINT32_MAX);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/esp32.md` around lines 67 - 87, The generated random float r can equal
1.0 when esp_random() returns UINT32_MAX, violating the markov_sample() API that
requires r in [0,1); update the r computation so it cannot reach 1.0 — e.g.,
divide the uint32_t result by (UINT32_MAX + 1.0f) or otherwise scale/offset the
output of esp_random() to produce values in [0,1) before passing to
markov_sample(row, r); change the line that sets r (currently using esp_random()
/ UINT32_MAX) so the produced float never equals 1.0 while keeping the same
uniform distribution.
Comment on lines
+180
to
+188
|
|
||
| Each model stores 6 float32 matrices (log + linear domain). | ||
|
|
||
| | Config | HMM_LOG_A | HMM_LOG_B | Total (×2 for linear) | | ||
| |--------|-----------|-----------|----------------------| | ||
| | 8 states, 50 obs | 1.3 KB | 1.6 KB | **~6 KB** | | ||
| | 16 states, 80 obs | 4 KB | 5 KB | **~18 KB** | | ||
| | 32 states, 200 obs | 16 KB | 25 KB | **~82 KB** | | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify hmm_c_sizes calculation
ast-grep --pattern $'def hmm_c_sizes($$$):
$$$'Repository: TigreGotico/markovonnx
Length of output: 48
🏁 Script executed:
# First, let's read the file around the mentioned lines to understand context
cat -n docs/esp32.md | sed -n '170,195p'Repository: TigreGotico/markovonnx
Length of output: 1020
🏁 Script executed:
# Search for any memory calculation code related to HMM
rg -i "hmm.*memory|memory.*hmm|1\.3.*kb|log_a|log_b" --type mdRepository: TigreGotico/markovonnx
Length of output: 1228
🏁 Script executed:
# Search for code that might calculate or define these matrix sizes
rg -i "float32|matrix|hmm" docs/esp32.md -B 2 -A 2Repository: TigreGotico/markovonnx
Length of output: 4198
🏁 Script executed:
# Find and examine the size_report.py file
fd -name "size_report.py" -exec cat {} \;Repository: TigreGotico/markovonnx
Length of output: 296
🏁 Script executed:
# Also check the c_export.py file for HMM rendering to understand memory layout
fd -name "c_export.py" -exec wc -l {} \;Repository: TigreGotico/markovonnx
Length of output: 296
🏁 Script executed:
# Search for where the HMM sizes might be calculated or documented
rg -i "1\.3.*kb|1\.6.*kb|6.*kb|18.*kb" --type pyRepository: TigreGotico/markovonnx
Length of output: 48
🏁 Script executed:
# Find all Python files related to export or reporting
git ls-files | grep -E "(size_report|c_export)" | head -20Repository: TigreGotico/markovonnx
Length of output: 181
🏁 Script executed:
# Search through all Python files for HMM memory calculations
git ls-files "*.py" | xargs rg -l "HMM|hmm" | head -10Repository: TigreGotico/markovonnx
Length of output: 367
🏁 Script executed:
# Read size_report.py to understand HMM memory calculations
cat markovonnx/size_report.pyRepository: TigreGotico/markovonnx
Length of output: 8604
🏁 Script executed:
# Also check for test cases that might show expected values
cat test/unittests/test_size_report.pyRepository: TigreGotico/markovonnx
Length of output: 9876
🏁 Script executed:
# Create a test to verify the exact sizes for 8 states, 50 obs
python3 << 'EOF'
# Simulate hmm_c_sizes calculation for 8 states, 50 obs
S = 8
O = 50
log_pi_bytes = S * 4
log_a_bytes = S * S * 4
log_b_bytes = S * O * 4
pi_bytes = S * 4
a_bytes = S * S * 4
b_bytes = S * O * 4
print("8 states, 50 obs configuration:")
print(f"HMM_LOG_PI: {log_pi_bytes} bytes = {log_pi_bytes/1024:.3f} KB")
print(f"HMM_LOG_A: {log_a_bytes} bytes = {log_a_bytes/1024:.3f} KB")
print(f"HMM_LOG_B: {log_b_bytes} bytes = {log_b_bytes/1024:.3f} KB")
print(f"HMM_PI: {pi_bytes} bytes = {pi_bytes/1024:.3f} KB")
print(f"HMM_A: {a_bytes} bytes = {a_bytes/1024:.3f} KB")
print(f"HMM_B: {b_bytes} bytes = {b_bytes/1024:.3f} KB")
matrix_total = log_pi_bytes + log_a_bytes + log_b_bytes + pi_bytes + a_bytes + b_bytes
print(f"\nAll matrices total: {matrix_total} bytes = {matrix_total/1024:.2f} KB")
print(f"(without vocab overhead)")
EOFRepository: TigreGotico/markovonnx
Length of output: 362
Documentation contains incorrect HMM memory values.
The table at lines 183–187 shows HMM_LOG_A consuming 1.3 KB for 8 states, but the actual matrix size is 8 × 8 × 4 bytes = 256 bytes (0.25 KB). The calculation in markovonnx/size_report.py:hmm_c_sizes() confirms this: log_a_bytes = S * S * 4. The reported 1.3 KB value appears incorrect and should be corrected. The ~6 KB total is reasonable when including vocabulary strings and all 6 matrices (both log and linear domains), but the individual column values need verification or clarification.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/esp32.md` around lines 180 - 188, Update the incorrect per-matrix size
values in the docs table to match the actual calculation used in
markovonnx/size_report.py:hmm_c_sizes() (log_a_bytes = S * S * 4), e.g., for 8
states HMM_LOG_A should be 256 bytes (~0.25 KB) not 1.3 KB; recompute HMM_LOG_B
and the other entries the same way (bytes -> KB) and adjust the per-column
values or add a clarifying note that the ~6 KB total includes all six matrices
plus vocabulary strings so the total can remain ~6 KB while individual matrix
entries change.
Comment on lines
+254
to
+266
| def predict_probs(self, context: List) -> np.ndarray: | ||
| """Return smoothed probability vector for *context*. | ||
|
|
||
| Public wrapper around :meth:`_get_probs`. Applies backoff to | ||
| lower-order models for unseen contexts when *backoff* was enabled. | ||
|
|
||
| Args: | ||
| context: Token sequence (at least *order* tokens long). | ||
|
|
||
| Returns: | ||
| Float32 array of shape ``(vocab_size,)`` summing to 1. | ||
| """ | ||
| return self._get_probs(context) |
There was a problem hiding this comment.
Enforce the documented minimum context length.
Line 261 says callers must pass at least order tokens, but this wrapper forwards shorter contexts into _get_probs(). Because _ctx_idx() is not length-aware, a short context can alias a real full-order row instead of backing off, so the public API can return the wrong distribution.
Suggested guard
def predict_probs(self, context: List) -> np.ndarray:
"""Return smoothed probability vector for *context*.
@@
Returns:
Float32 array of shape ``(vocab_size,)`` summing to 1.
"""
+ if len(context) < self.order:
+ raise ValueError(
+ f"context must contain at least {self.order} tokens"
+ )
return self._get_probs(context)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@markovonnx/markov.py` around lines 254 - 266, predict_probs currently
forwards any-length contexts to _get_probs which can mis-index via _ctx_idx;
enforce the documented minimum by validating that len(context) >= self.order at
the start of predict_probs and raise a clear ValueError (e.g. "context must be
at least order tokens") if not, only calling self._get_probs(context) when the
check passes so backoff/length semantics are preserved.
Comment on lines
+336
to
+346
| if self._lower is not None: | ||
| data["lower"] = { | ||
| "order": self._lower.order, | ||
| "smoothing": self._lower.smoothing, | ||
| "backoff": self._lower.backoff, | ||
| "kneser_ney": self._lower.kneser_ney, | ||
| "kn_d1": self._lower._kn_d1, | ||
| "kn_d2": self._lower._kn_d2, | ||
| "kn_d3": self._lower._kn_d3, | ||
| "counts": {str(k): v.tolist() for k, v in self._lower._counts.items()}, | ||
| } |
There was a problem hiding this comment.
Serialize the full backoff chain recursively.
This only persists the first _lower node. For order > 2, fit() / fit_streaming() build _lower._lower..., so a JSON or .markov round-trip silently drops the deepest fallback levels and changes later inference/export behavior.
Also applies to: 376-390
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@markovonnx/markov.py` around lines 336 - 346, The current serialization
writes only the first _lower node, losing deeper backoff levels; update the
serialization in markov.py to recursively serialize the entire backoff chain
(order, smoothing, backoff, kneser_ney, _kn_d1/_kn_d2/_kn_d3 and counts) by
replacing the single-level data["lower"] assignment with a recursive helper
(e.g., _serialize_lower(node)) that returns the nested dict for node and
node._lower, and use that helper in both places where the single-level block
appears (the current block around the shown lines and the similar block at lines
376-390).
Comment on lines
+36
to
+46
| # Vocabulary: sum of UTF-8 encoded string lengths + null terminator | ||
| vocab_bytes = sum(len(tok.encode("utf-8")) + 1 for tok in chain.vocab.id2tok) | ||
|
|
||
| keys_bytes = n_sparse * 8 # uint64_t per row | ||
| probs_bytes = n_sparse * V * prob_bytes # prob_type[n_sparse][V] | ||
|
|
||
| sizes: Dict[str, int] = { | ||
| "vocab": vocab_bytes, | ||
| "MARKOV_KEYS": keys_bytes, | ||
| "MARKOV_PROBS": probs_bytes, | ||
| } |
There was a problem hiding this comment.
Include the vocab pointer tables in these totals.
markovonnx/c_export.py emits MARKOV_VOCAB, HMM_OBS_VOCAB, and HMM_STATE_VOCAB as const char*[], so the size math needs both the UTF-8 string bytes and the pointer array itself. On ESP32 that's another 4 bytes per entry; otherwise size-report --max-bytes can undercount and let an oversized header pass.
Suggested fix
ESP32_FLASH_BYTES: int = 4 * 1024 * 1024 # 4 MB
ESP32_IRAM_BYTES: int = 520 * 1024 # 520 KB
+ESP32_POINTER_BYTES: int = 4
@@
- vocab_bytes = sum(len(tok.encode("utf-8")) + 1 for tok in chain.vocab.id2tok)
+ vocab_bytes = V * ESP32_POINTER_BYTES + sum(
+ len(tok.encode("utf-8")) + 1 for tok in chain.vocab.id2tok
+ )
@@
- obs_vocab_bytes = sum(len(tok.encode("utf-8")) + 1 for tok in hmm.obs_vocab.id2tok)
- state_vocab_bytes = sum(len(tok.encode("utf-8")) + 1 for tok in state_vocab)
+ obs_vocab_bytes = O * ESP32_POINTER_BYTES + sum(
+ len(tok.encode("utf-8")) + 1 for tok in hmm.obs_vocab.id2tok
+ )
+ state_vocab_bytes = S * ESP32_POINTER_BYTES + sum(
+ len(tok.encode("utf-8")) + 1 for tok in state_vocab
+ )Also applies to: 83-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@markovonnx/size_report.py` around lines 36 - 46, The vocab size math
currently only sums UTF-8 string bytes but omits the pointer-array overhead for
const char*[] tables (MARKOV_VOCAB, HMM_OBS_VOCAB, HMM_STATE_VOCAB); update the
calculations in markovonnx/size_report.py (where vocab_bytes is computed and the
similar block at lines ~83-104) to add pointer_table_bytes = num_entries *
pointer_size (use struct.calcsize("P") or equivalent to get platform pointer
size, e.g., 4 on ESP32) and include those pointer_table_bytes in the
corresponding entries in the sizes dict so the reported totals account for both
string bytes and the pointer arrays.
Comment on lines
+328
to
+331
| @pytest.mark.skipif( | ||
| subprocess.run(["which", "gcc"], capture_output=True).returncode != 0, | ||
| reason="gcc not available", | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all non-portable "which gcc" skip conditions in this file.
rg -n 'subprocess\.run\(\["which",\s*"gcc"\]' test/unittests/test_c_export.pyRepository: TigreGotico/markovonnx
Length of output: 541
🏁 Script executed:
#!/bin/bash
# Check if shutil is already imported in the test file
rg -n "import shutil" test/unittests/test_c_export.py
# Get context around the first occurrence to understand the structure
echo "=== Context around line 329 ==="
sed -n '327,333p' test/unittests/test_c_export.py
# Check total lines in file to verify line numbers
wc -l test/unittests/test_c_export.pyRepository: TigreGotico/markovonnx
Length of output: 380
🏁 Script executed:
#!/bin/bash
# Verify that shutil.which() is available in Python's standard library
python3 <<EOF
import shutil
import sys
# Check if shutil.which exists and test basic behavior
if hasattr(shutil, 'which'):
print("✓ shutil.which is available")
# Test basic behavior
result = shutil.which("python3")
print(f" shutil.which('python3') returned: {result}")
result_none = shutil.which("nonexistent_command_xyz_12345")
print(f" shutil.which('nonexistent_command_xyz_12345') returned: {result_none}")
else:
print("✗ shutil.which is NOT available")
EOFRepository: TigreGotico/markovonnx
Length of output: 209
🏁 Script executed:
#!/bin/bash
# Check if subprocess is imported (to understand full import context)
head -20 test/unittests/test_c_export.py | grep -E "^import|^from"Repository: TigreGotico/markovonnx
Length of output: 274
Replace which gcc subprocess checks with shutil.which("gcc") for cross-platform compatibility.
The which command is not available on Windows and can fail at import time in restricted environments. Use the portable shutil.which() alternative instead. This pattern appears at 6 locations in the file: lines 329, 355, 611, 763, 1042, and 1124.
Suggested change
+import shutil
import subprocess
...
`@pytest.mark.skipif`(
- subprocess.run(["which", "gcc"], capture_output=True).returncode != 0,
+ shutil.which("gcc") is None,
reason="gcc not available",
)Apply the same pattern change to all 6 occurrences.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unittests/test_c_export.py` around lines 328 - 331, Replace the
subprocess-based GCC presence checks with shutil.which for portability: locate
occurrences of subprocess.run(["which", "gcc"], capture_output=True).returncode
!= 0 (six places in test_c_export.py) and change them to shutil.which("gcc") is
None (or use shutil.which("gcc") to test presence) and add "import shutil" if
not already imported; ensure each pytest.mark.skipif uses the new check.
Comment on lines
+332
to
+351
| def test_export_valid_c_syntax_quantized() -> None: | ||
| """Generated quantized header passes gcc -fsyntax-only.""" | ||
| mc = _make_chain(order=1) | ||
| with tempfile.NamedTemporaryFile(suffix=".h", delete=False) as f: | ||
| path = f.name | ||
| try: | ||
| export_markov_c_header(mc, path, quantize=True) | ||
| # Wrap in a minimal translation unit | ||
| with tempfile.NamedTemporaryFile(suffix=".c", delete=False, mode="w") as cf: | ||
| cf.write(f'#include "{path}"\nint main(void) {{ return 0; }}\n') | ||
| c_path = cf.name | ||
| result = subprocess.run( | ||
| ["gcc", "-std=c99", "-fsyntax-only", c_path], | ||
| capture_output=True, text=True, | ||
| ) | ||
| assert result.returncode == 0, result.stderr | ||
| finally: | ||
| os.unlink(path) | ||
| if os.path.exists(c_path): | ||
| os.unlink(c_path) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "test_c_export.py" -type fRepository: TigreGotico/markovonnx
Length of output: 99
🏁 Script executed:
wc -l ./test/unittests/test_c_export.pyRepository: TigreGotico/markovonnx
Length of output: 104
🏁 Script executed:
sed -n '332,351p' ./test/unittests/test_c_export.pyRepository: TigreGotico/markovonnx
Length of output: 903
Initialize c_path before try to avoid masking failures in cleanup.
The c_path variable is assigned only inside the nested with statement, but is referenced in the finally block. If export_markov_c_header() or any code before the assignment raises an exception, the cleanup will trigger an UnboundLocalError and hide the root cause.
🔧 Suggested change
def test_export_valid_c_syntax_quantized() -> None:
"""Generated quantized header passes gcc -fsyntax-only."""
mc = _make_chain(order=1)
with tempfile.NamedTemporaryFile(suffix=".h", delete=False) as f:
path = f.name
+ c_path = ""
try:
export_markov_c_header(mc, path, quantize=True)
# Wrap in a minimal translation unit
with tempfile.NamedTemporaryFile(suffix=".c", delete=False, mode="w") as cf:
cf.write(f'#include "{path}"\nint main(void) {{ return 0; }}\n')
c_path = cf.name
...
finally:
os.unlink(path)
- if os.path.exists(c_path):
+ if c_path and os.path.exists(c_path):
os.unlink(c_path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unittests/test_c_export.py` around lines 332 - 351, The test function
test_export_valid_c_syntax_quantized leaves c_path uninitialized if an exception
occurs before the nested with block, causing UnboundLocalError in the finally
cleanup; initialize c_path (e.g., c_path = None) before the try and update the
finally block to only attempt os.unlink(c_path) if c_path is not None and
os.path.exists(c_path), while keeping the existing cleanup for path and the call
to export_markov_c_header unchanged.
JarbasAl
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
train-hmmfor training hidden Markov models,exportfor C header generation, andsize-reportfor memory budgeting.predict_probs()method for probability predictions.Documentation