Document transformer depth classifier architecture#37
Conversation
Reviewer's GuideThis PR formalizes the Transformer-Ordinal DepthClassifierOnnx via a new ADR, updates the lag-complexity design to make it the default depth provider with an ONNX MLP fallback, and refreshes provider trade-offs and roadmap tasks to incorporate quantization, calibration, and deterministic tokenization. Class diagram for DepthClassifierOnnx and fallback integrationclassDiagram
class DepthEstimator {
<<trait>>
+estimate(text: String): f32
}
class DepthClassifierOnnx {
+session: OrtSession
+input_names: Vec<String>
+output_name: String
+estimate(text: String): f32
}
class DepthClassifierMlpOnnx {
+session: OrtSession
+input_names: Vec<String>
+output_name: String
+estimate(features: Vec<f32>): f32
}
DepthClassifierOnnx --|> DepthEstimator
DepthClassifierMlpOnnx --|> DepthEstimator
class DepthHeuristic {
+estimate(features: Vec<f32>): f32
}
DepthHeuristic --|> DepthEstimator
class Sigma {
+normalize(depth: f32): f32
}
class Complexity {
+total: f32
+scope: f32
+depth: f32
+ambiguity: f32
}
DepthClassifierOnnx --> Sigma
DepthClassifierMlpOnnx --> Sigma
DepthHeuristic --> Sigma
Sigma --> Complexity
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
WalkthroughAdd an optional ONNX Transformer‑ordinal depth and ambiguity classifier with SHA‑256 artefact verification behind a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client as DepthProvider Client
participant Tok as Tokeniser (deterministic)
participant Prov as Provider Impl
participant ORT as ONNX Runtime
participant Ord as Ordinal Aggregator
participant Cal as Calibration
Client->>Prov: request_depth(input_text)
Prov->>Tok: tokenize(input_text)
Tok-->>Prov: input_ids, attention_mask
Prov->>ORT: run(model.onnx, inputs)
ORT-->>Prov: logits (float32[K])
Prov->>Ord: aggregate(logits)
Ord-->>Prov: depth_raw (EV or mid-bin)
Prov->>Cal: apply_calibration(depth_raw)
Cal-->>Prov: depth_calibrated
Prov-->>Client: depth_scalar + metrics/traces
alt MLP fallback enabled
Prov->>ORT: run(mlp_fallback.onnx, features)
ORT-->>Prov: depth_scalar
Prov->>Cal: optional_calibration
end
rect rgba(230,245,255,0.5)
note right of ORT: Deterministic CPU inference, artefact SHA‑256 verification before load
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `docs/adr-depth-classifier-onnx.md:180` </location>
<code_context>
+- **Quantisation:** dynamic or static INT8 for linear layers; FP32 output.
+- **Output:** scalar raw depth to Sigma.
+
+## 14. Testing Strategy
+
+- **Unit:** tokeniser determinism; ordinal head probability semantics;
</code_context>
<issue_to_address>
**issue (review_instructions):** Require explicit behavioural and unit test coverage for all new features and changes described in this ADR.
The ADR outlines a new architecture and fallback for depth estimation, including integration, calibration, quantisation, and error handling. You must provide concrete behavioural and unit tests for each provider, including golden trace, property, and cross-platform tests, as described. Ensure these tests are implemented and referenced in the codebase.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 2
<location> `docs/adr-depth-classifier-onnx.md:1` </location>
<code_context>
+# Architecture Decision Record: DepthClassifierOnnx — Model Selection & Architecture for the Cognitive Clutch
+
+**Status:** Accepted (Revised) **Date:** 24 September 2025 **Owner:** LAG
</code_context>
<issue_to_address>
**issue (review_instructions):** Some paragraphs and bullet points in this file exceed the 80-column wrapping requirement for Markdown prose.
Please ensure that all paragraphs and bullet points are wrapped to 80 columns for improved readability and consistency with documentation standards.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/lag-complexity-function-design.md (3)
428-447: Fix heading case to sentence case.Change “Feature Engineering (Depth)” and “Feature Engineering (Ambiguity)” to sentence case per style guide.
-##### Feature Engineering (Depth) +##### Feature engineering (depth) -##### Feature Engineering (Ambiguity) +##### Feature engineering (ambiguity)Also applies to: 519-543
176-181: Standardize to OED -ize spellings.Enforce “normalize/normalization”, “parameterize/parameterization”, and related -ize forms throughout, while retaining “analyse/analyses” (-yse) and “behaviour/colour” (-our). Current usage mixes -ise and -ize.
Also applies to: 616-622, 623-632, 733-742
1156-1219: Bring the Rust example in line with doctest guidance or mark as non-compiling.The example constructs
Complexityvia private fields, which will not compile as a doctest. Either:
- Add a minimal
fn main() -> Result<()>example with public constructor/accessors and assertions, hiding boilerplate with#, or- Mark the block as
rust,ignoreand add a one‑line preface that it is illustrative.Apply the minimal change below now and follow up with a compiled doctest later.
-```rust +// Illustrative example (pseudocode) +```rust,ignore use lag_complexity::api::{Complexity, ComplexityFn}; use lag_complexity::config::ScoringConfig; use lag_complexity::providers::{AmbiguityEstimator, DepthEstimator, EmbeddingProvider}; @@ - Ok(Complexity { - total, - scope, - depth, - ambiguity, - }) + // Assume a public constructor or builder in real code: + Ok(Complexity::from_components(scope, depth, ambiguity, total)) } // ... trace() implementation ... }</blockquote></details> </blockquote></details>📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
docs/adr-depth-classifier-onnx.md(1 hunks)docs/lag-complexity-function-design.md(17 hunks)🧰 Additional context used
📓 Path-based instructions (3)
docs/**/*.{rs,md}
📄 CodeRabbit inference engine (docs/rust-doctest-dry-guide.md)
docs/**/*.{rs,md}: In code fences, explicitly specify the rust language (```rust) for doctest code blocks
Use assert!, assert_eq!, and assert_ne! in doctests to verify outcomes
Avoid using unwrap()/expect() in examples; handle errors idiomatically
For fallible examples, write an explicit fn main() -> Result<...> and hide the boilerplate lines with #
Optionally use the (()) shorthand at the end of a code block to make main return Result; ensure no whitespace within (())
Use hidden lines (#) to hide setup, use statements, and boilerplate in doctest examples
Prefer no_run for examples with undesirable side effects (network, FS, GUI) so they compile but don’t execute
Use should_panic for examples intended to panic; optionally assert specific messages
Use compile_fail for examples that should not compile, but be aware of its brittleness across compiler versions
Avoid ignore for Rust examples; reserve it for pseudocode or non-Rust blocks
Use edition20xx doctest attribute when demonstrating edition-specific syntax or behaviorFiles:
docs/adr-depth-classifier-onnx.mddocs/lag-complexity-function-design.mddocs/**/*.md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
docs/**/*.md: Use British English (OED) spelling in documentation
The word “outwith” is acceptable in documentation
Keep US spelling when it appears in API names (e.g.,color)
Use the Oxford comma in lists
Treat company names as collective nouns (e.g., “Lille Industries are …”)
Write headings in sentence case
Use Markdown headings (#,##,###, …) in order without skipping levels
Follow markdownlint recommendations
Use standard Markdown syntax for code blocks and lists
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text
Use-for first-level bullets and renumber ordered lists when items change
Prefer inline links[text](url)or angle-bracket autolinks<url>
Ensure blank lines before and after bulleted lists and fenced code blocks
Ensure tables have a delimiter line below the header row
Expand uncommon acronyms on first use (e.g., Continuous Integration (CI))
Wrap paragraphs at 80 columns
Wrap code (in docs) at 120 columns
Do not wrap tables in documentation
Use footnotes referenced with[^label]
Include Mermaid diagrams where they add clarity
Embed images withand provide brief, descriptive alt text
Add a short descriptive sentence before each Mermaid diagram for screen readers
docs/**/*.md: Use docs/ markdown as reference and keep it updated with decisions and changes
Documentation must use en-GB-oxendict spelling and grammar (LICENSE naming unchanged)Files:
docs/adr-depth-classifier-onnx.mddocs/lag-complexity-function-design.md**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown with make markdownlint
Run make fmt to format Markdown files and fix table markup after documentation changes
Validate Mermaid diagrams in Markdown via make nixie
Wrap Markdown paragraphs and bullet points at 80 columns
Wrap code blocks at 120 columns
Do not wrap tables and headings
Use dashes (-) for list bullets
Use GitHub-flavoured Markdown footnotes ([^1])Files:
docs/adr-depth-classifier-onnx.mddocs/lag-complexity-function-design.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/adr-depth-classifier-onnx.mddocs/lag-complexity-function-design.md🔍 Remote MCP Ref
Concise additional context for reviewing PR #37
Files changed: new ADR docs/adr-depth-classifier-onnx.md and updated docs/lag-complexity-function-design.md (docs-only PR). [PR page]
ADR — concrete technical specs to review:
- IO/signature: input_ids and attention_mask (int64) → logits_ord (float32[batch,K]); depth derived via expected value or mid-bin mapping from ordinal outputs.
- Model/backbone: lightweight DistilBERT-class encoder with multi-layer pooling → ordinal head (cumulative/threshold encoding).
- ONNX details: target opset 17; minimal ONNX graph described; deterministic Rust tokeniser required; optional calibration via Mul/Add nodes.
- Quantisation & perf targets: post-training static INT8 quantisation planned; p95 latency target ≤ 10 ms on CPU; acceptance goal ≤ 1% accuracy degradation vs FP32.
- Fallback & rollout: Fixed-Feature MLP (MLP-Log) fallback behind feature flag; rollout plan, calibration/validation steps, tracing/metrics, provider trait integration and versioning metadata included.
Design doc updates: "ONNX Classifier" replaced with "ONNX Transformer-Ordinal"; added "ONNX MLP Fallback" row in provider capabilities table; depth & ambiguity feature-engineering sections updated to align with the ADR.
PR metadata / CI targets: make fmt, make lint, make test, make markdownlint, make nixie listed in PR. Branch codex/add-adr-for-depthclassifieronnx-and-design-update → main; author: leynos.
Reviewer checkpoints (facts from docs to verify during review)
- Confirm provider trait interfaces and feature-flag wiring referenced in ADR exist or will be added in code.
- Confirm ONNX export/conversion and CI/tooling support opset 17 and static INT8 quantisation (and whether any tooling changes are required).
- Confirm availability/versioning of deterministic Rust tokeniser/vocab artifacts referenced.
Sources used
- PR page: #37
- ADR file: docs/adr-depth-classifier-onnx.md (raw): https://raw.githubusercontent.com/leynos/lag-complexity/codex/add-adr-for-depthclassifieronnx-and-design-update/docs/adr-depth-classifier-onnx.md
- Updated design doc (raw): https://raw.githubusercontent.com/leynos/lag-complexity/codex/add-adr-for-depthclassifieronnx-and-design-update/docs/lag-complexity-function-design.md
- Repository root: https://github.com/leynos/lag-complexity
- Search queries used: Ref documentation searches for repository/model terms.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (3)
docs/adr-depth-classifier-onnx.md (2)
99-104: Confirm dual calibration paths are implemented and observable.Document the exact telemetry and configuration flags when calibration is in‑graph (Mul/Add) versus applied in Rust. Ensure both paths produce identical results in golden tests.
60-68: Architecture and rollout plan read well.The default Transformer‑Ordinal path with INT8 targets and the feature‑flagged MLP fallback are well‑articulated and production‑oriented.
Add links to benchmark CI jobs once available to close the loop with the stated p95 goals.
Also applies to: 146-153, 195-203
docs/lag-complexity-function-design.md (1)
472-494: Link the ADR with a stable relative path.Keep the relative link to the ADR; it resolves correctly from docs/. No change required.
Ensure the ADR filename remains stable in future renames; otherwise, update this link.
| - **Integration fit:** implements `TextProcessor<Output = f32>`; clean wiring | ||
| into Complexity → Sigma → Weights → Schedule; full tracing and metrics. | ||
| - **Calibration & monotonicity:** outputs align with ordered difficulty/step |
There was a problem hiding this comment.
🧩 Analysis chain
Verify trait/feature wiring and toolchain support.
Confirm the following exist on the branch:
- TextProcessor trait and DepthEstimator alias returning f32.
- onnx feature flag wiring DepthClassifierOnnx behind ORT.
- ORT crate pinned to a version supporting opset 17.
- Deterministic Rust tokeniser (tokenizers) and pinned vocab artefacts.
Run:
Also applies to: 156-170, 160-162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate trait and aliases =="
rg -nP --type=rust -C2 '\btrait\s+TextProcessor\b|type\s+DepthEstimator\b|type\s+EmbeddingProvider\b'
echo "== Check feature flags in Cargo.toml =="
rg -nP -C3 '^\[features\]|onnx|provider-(api|candle|tch)|rayon' Cargo.toml
echo "== Check ORT crate and version =="
rg -nP -C2 '^\s*ort\s*=\s*' Cargo.toml || true
rg -nP -C2 'onnx|onnxruntime' Cargo.lock 2>/dev/null || true
echo "== Check tokenizer usage and artefacts =="
rg -nP --type=rust -C3 '\btokeni[zs]er|\btokenizers\b'
fd -t f -i 'tokenizer.json|vocab.json|merges.txt|spiece.model' || true
echo "== Search for quantisation pipeline mentions =="
rg -nP -C2 'quantis|quantiz|int8|calibration|qdq|static quant'Length of output: 1449
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== providers.rs (first 140 lines) =="
if [ -f src/providers.rs ]; then
sed -n '1,140p' src/providers.rs || true
else
echo "src/providers.rs not found"
fi
echo
echo "== Search for DepthClassifierOnnx symbol =="
rg -nP --type=rust -C2 'DepthClassifierOnnx' || true
echo
echo "== Search for DepthClassifier (any mention) =="
rg -nP --type=rust -C2 '\bDepthClassifier\b' || true
echo
echo "== Search for onnx/ort/onnxruntime across repo =="
rg -nP --hidden -S -n 'onnx|ort\b|onnxruntime|onnxrt' || true
echo
echo "== Search for onnx/ort mentions in any Cargo.toml =="
if command -v fd >/dev/null 2>&1; then
fd -HI Cargo.toml || true
rg -nP --hidden -S 'onnx|ort\b|onnxruntime|onnxrt' **/Cargo.toml || true
else
find . -name Cargo.toml -print || true
rg -nP --hidden -S 'onnx|ort\b|onnxruntime|onnxrt' $(find . -name Cargo.toml 2>/dev/null) || true
fi
echo
echo "== Workspace Cargo.toml files (list) =="
if command -v fd >/dev/null 2>&1; then
fd -HI Cargo.toml || true
else
find . -name Cargo.toml -print || true
fi
echo
echo "== Search for tokenizers usage =="
rg -nP --hidden -S '\btokeni[sz]ers?\b|\btokeni[sz]er\b' || true
echo
echo "== Search for tokenizer artefact files =="
if command -v fd >/dev/null 2>&1; then
fd -HI -e json -e txt -e model -e spm -e vocab 'tokenizer.json' 'vocab.json' 'merges.txt' 'spiece.model' || true
else
find . -type f \( -iname tokenizer.json -o -iname vocab.json -o -iname merges.txt -o -iname spiece.model \) -print || true
fi
echo
echo "== Check top-level Cargo.lock for onnx/ort =="
if [ -f Cargo.lock ]; then
rg -nP 'onnx|ort\b|onnxruntime|onnxrt' Cargo.lock || true
else
echo "No top-level Cargo.lock"
fiLength of output: 14608
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Top-level Cargo.toml =="
sed -n '1,240p' Cargo.toml || true
echo
echo "== All Cargo.toml files =="
if command -v fd >/dev/null 2>&1; then
fd -HI Cargo.toml -x sed -n '1,240p' {} \; || true
else
find . -name Cargo.toml -print -exec sed -n '1,240p' {} \; || true
fi
echo
echo "== Search for 'onnx' and 'ort' in manifest and sources =="
rg -nP --hidden -S 'onnx|ort\b|onnxruntime|onnxrt' || true
echo
echo "== Search for cfg(feature = \"onnx\") usages =="
rg -nP --type=rust -n 'cfg\(.*feature.*onnx.*\)|cfg\(feature\s*=\s*\"onnx\"' || true
echo
echo "== Search for DepthClassifierOnnx/AmbiguityClassifierOnnx symbols =="
rg -nP --type=rust -n '\bDepthClassifierOnnx\b|\bAmbiguityClassifierOnnx\b' || true
echo
echo "== Show workspace members (if any) =="
rg -nP --hidden -n 'workspace\s*=' Cargo.toml || true
rg -nP --hidden -n 'members\s*=' Cargo.toml || trueLength of output: 17373
Implement ONNX feature wiring, add ORT dependency and include deterministic tokeniser artefacts
- Confirmed: TextProcessor trait and DepthEstimator alias present in src/providers.rs (lines ~12–38).
- Missing: no
onnxfeature in Cargo.toml, nocfg(feature = "onnx")gating, and no DepthClassifierOnnx / AmbiguityClassifierOnnx symbols found. - Missing: no
ortcrate in Cargo.toml or Cargo.lock — add an optionalortdependency and pin to an ONNX Runtime release that supports opset 17; validate runtime/opset compatibility at model load. - Missing: no Rust
tokenizersusage or vocabulary artefacts (tokenizer.json, vocab.json, merges.txt, spiece.model) in the repo — include a deterministic tokeniser and commit/pin vocab artefacts with SHA‑256 checksums. - Action: add
onnxfeature to Cargo.toml, gate ONNX providers behind it, implement DepthClassifierOnnx/AmbiguityClassifierOnnx, add/pinortdependency, and bundle/verify model and tokeniser artefacts at load.
🤖 Prompt for AI Agents
In docs/adr-depth-classifier-onnx.md around lines 39 to 41: the repo documents
an ONNX depth classifier but the implementation and wiring are missing — add an
"onnx" Cargo feature in Cargo.toml, add an optional, version‑pinned ort
dependency (select an ONNX Runtime release that supports opset 17) and gate all
ONNX provider code behind cfg(feature = "onnx"), implement DepthClassifierOnnx
and AmbiguityClassifierOnnx types and hook them into the existing
TextProcessor/DepthEstimator wiring, implement runtime validation of the model's
opset/version at model load and fail fast if incompatible, and include
deterministic tokenizer artifacts (tokenizer.json / vocab.json / merges.txt /
spiece.model) committed to the repo with SHA‑256 checksums and verify those
checksums at load time before initializing the tokenizer and model.
| - **Provider type:** `DepthClassifierOnnx { session, input_name(s), output_name | ||
| }`. | ||
| - **Trait:** implements `TextProcessor<Output = f32>`; returns scalar raw depth | ||
| (expected steps or mapped mid-bin). | ||
| - **Tokenisation:** performed in Rust with a pinned vocabulary; deterministic | ||
| and locale-safe. | ||
| - **Error handling:** ONNX Runtime errors mapped to `Error::Inference` with | ||
| model path, opset, and checksum in diagnostics. | ||
| - **Tracing & metrics:** instrument `process()`; export latency histograms, | ||
| error counters, and token length/batch size tags; integrate with existing | ||
| observability per the design document. | ||
| - **Configuration:** model path, enablement flags, quantisation variant | ||
| selection, and optional in-graph calibration switch; Sigma/Weights/Schedule | ||
| remain unchanged. | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename fields for plurality and clarity.
Use pluralised field names for collections:
- input_names: Vec
- output_names: Vec
Apply this change in the doc and the struct definition to avoid ambiguity at integration time.
-**Provider type:** `DepthClassifierOnnx { session, input_name(s), output_name }`.
+**Provider type:** `DepthClassifierOnnx { session, input_names, output_names }`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - **Provider type:** `DepthClassifierOnnx { session, input_name(s), output_name | |
| }`. | |
| - **Trait:** implements `TextProcessor<Output = f32>`; returns scalar raw depth | |
| (expected steps or mapped mid-bin). | |
| - **Tokenisation:** performed in Rust with a pinned vocabulary; deterministic | |
| and locale-safe. | |
| - **Error handling:** ONNX Runtime errors mapped to `Error::Inference` with | |
| model path, opset, and checksum in diagnostics. | |
| - **Tracing & metrics:** instrument `process()`; export latency histograms, | |
| error counters, and token length/batch size tags; integrate with existing | |
| observability per the design document. | |
| - **Configuration:** model path, enablement flags, quantisation variant | |
| selection, and optional in-graph calibration switch; Sigma/Weights/Schedule | |
| remain unchanged. | |
| - **Provider type:** `DepthClassifierOnnx { session, input_names, output_names }`. | |
| - **Trait:** implements `TextProcessor<Output = f32>`; returns scalar raw depth | |
| (expected steps or mapped mid-bin). | |
| - **Tokenisation:** performed in Rust with a pinned vocabulary; deterministic | |
| and locale-safe. | |
| - **Error handling:** ONNX Runtime errors mapped to `Error::Inference` with | |
| model path, opset, and checksum in diagnostics. | |
| - **Tracing & metrics:** instrument `process()`; export latency histograms, | |
| error counters, and token length/batch size tags; integrate with existing | |
| observability per the design document. | |
| - **Configuration:** model path, enablement flags, quantisation variant | |
| selection, and optional in-graph calibration switch; Sigma/Weights/Schedule | |
| remain unchanged. |
🤖 Prompt for AI Agents
In docs/adr-depth-classifier-onnx.md around lines 156 to 170, rename the
singular field names to plural: change input_name(s) to input_names: Vec<String>
and output_name to output_names: Vec<String> in the ADR text and ensure the
corresponding Rust struct definition uses input_names: Vec<String> and
output_names: Vec<String>. Update all references/examples/tests/config snippets
in the repo to the plural names and, in the Rust struct, add serde aliases
(e.g., #[serde(rename = "input_name", alias = "input_name")]) if backward
compatibility with existing serialized data is required. Ensure compilation and
docs build by running tests and regenerating any examples or schema outputs that
reference these fields.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
docs/adr-depth-classifier-onnx.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
docs/**/*.{rs,md}
📄 CodeRabbit inference engine (docs/rust-doctest-dry-guide.md)
docs/**/*.{rs,md}: In code fences, explicitly specify the rust language (```rust) for doctest code blocks
Use assert!, assert_eq!, and assert_ne! in doctests to verify outcomes
Avoid using unwrap()/expect() in examples; handle errors idiomatically
For fallible examples, write an explicit fn main() -> Result<...> and hide the boilerplate lines with #
Optionally use the (()) shorthand at the end of a code block to make main return Result; ensure no whitespace within (())
Use hidden lines (#) to hide setup, use statements, and boilerplate in doctest examples
Prefer no_run for examples with undesirable side effects (network, FS, GUI) so they compile but don’t execute
Use should_panic for examples intended to panic; optionally assert specific messages
Use compile_fail for examples that should not compile, but be aware of its brittleness across compiler versions
Avoid ignore for Rust examples; reserve it for pseudocode or non-Rust blocks
Use edition20xx doctest attribute when demonstrating edition-specific syntax or behavior
Files:
docs/adr-depth-classifier-onnx.md
docs/**/*.md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
docs/**/*.md: Use British English (OED) spelling in documentation
The word “outwith” is acceptable in documentation
Keep US spelling when it appears in API names (e.g.,color)
Use the Oxford comma in lists
Treat company names as collective nouns (e.g., “Lille Industries are …”)
Write headings in sentence case
Use Markdown headings (#,##,###, …) in order without skipping levels
Follow markdownlint recommendations
Use standard Markdown syntax for code blocks and lists
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text
Use-for first-level bullets and renumber ordered lists when items change
Prefer inline links[text](url)or angle-bracket autolinks<url>
Ensure blank lines before and after bulleted lists and fenced code blocks
Ensure tables have a delimiter line below the header row
Expand uncommon acronyms on first use (e.g., Continuous Integration (CI))
Wrap paragraphs at 80 columns
Wrap code (in docs) at 120 columns
Do not wrap tables in documentation
Use footnotes referenced with[^label]
Include Mermaid diagrams where they add clarity
Embed images withand provide brief, descriptive alt text
Add a short descriptive sentence before each Mermaid diagram for screen readers
docs/**/*.md: Use docs/ markdown as reference and keep it updated with decisions and changes
Documentation must use en-GB-oxendict spelling and grammar (LICENSE naming unchanged)
Files:
docs/adr-depth-classifier-onnx.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown with make markdownlint
Run make fmt to format Markdown files and fix table markup after documentation changes
Validate Mermaid diagrams in Markdown via make nixie
Wrap Markdown paragraphs and bullet points at 80 columns
Wrap code blocks at 120 columns
Do not wrap tables and headings
Use dashes (-) for list bullets
Use GitHub-flavoured Markdown footnotes ([^1])
Files:
docs/adr-depth-classifier-onnx.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/adr-depth-classifier-onnx.md
🔍 Remote MCP Ref
Summary of additional context relevant to reviewing PR #37
-
Files changed: new ADR docs/adr-depth-classifier-onnx.md added; docs/lag-complexity-function-design.md updated to make "ONNX Transformer-Ordinal" the default and add an "ONNX MLP Fallback" row.
-
ADR concrete specs to verify:
- IO / signature: inputs input_ids and attention_mask (int64) → logits_ord (float32[batch,K]); depth computed by expected value or mid-bin mapping from ordinal outputs.
- Model: DistilBERT-class lightweight encoder + ordinal head (cumulative/threshold encoding). Deterministic Rust tokenizer required.
- ONNX targets: opset 17; minimal graph described; optional calibration via Mul/Add nodes.
- Quantisation & perf targets: post-training static INT8 quantisation planned; p95 CPU latency target ≤ 10 ms; acceptance goal ≤ 1% accuracy degradation vs FP32.
- Fallback & rollout: feature-flagged Fixed-Feature MLP fallback (DepthClassifierMlpOnnx / MLP-Log) behind a feature flag, rollout plan, tracing/metrics, calibration/validation and versioning notes included.
-
Reviewer checkpoints surfaced by the docs (items to confirm in code/CI/tooling):
- Provider trait interfaces and feature-flag wiring referenced by ADR actually exist or are planned.
- CI/tooling support for ONNX opset 17 and post-training static INT8 quantisation (are converters/calibration steps and required tool versions available in repo/CI).
- Availability/versioning of the deterministic Rust tokenizer/vocab artifacts referenced.
-
CI / checks listed in PR: make fmt, make lint, make test, make markdownlint, make nixie.
Sources used
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (7)
docs/adr-depth-classifier-onnx.md (7)
199-203: Verify ONNX feature gating, ORT dependency, and provider wiring exist in code.Ensure the
onnxCargo feature, optionalortdependency (opset 17‑ready), andDepthClassifierOnnximplementation are present and gated.Run:
#!/bin/bash set -euo pipefail echo "== Cargo.toml features and ORT dependency ==" rg -nP -C3 '^\[features\]|onnx\b' Cargo.toml || true rg -nP -C2 '^\s*ort\s*=\s*' Cargo.toml || true echo echo "== cfg(feature = \"onnx\") usage in Rust sources ==" rg -nP --type=rust -n 'cfg\(.*feature.*onnx.*\)' echo echo "== Provider symbols ==" rg -nP --type=rust -n '\bDepthClassifierOnnx\b|\bDepthClassifierMlpOnnx\b' echo echo "== Tokeniser usage and artefacts ==" rg -nP --type=rust -n '\btokeni[sz]ers?\b|\bTokenizer\b' || true fd -HI -t f 'tokenizer.json' 'vocab.json' 'merges.txt' 'spiece.model' || true
16-21: Standardise to OED -ize spellings (normalization/optimization/quantization).Align spellings with en‑GB‑oxendict: use -ize/-ization while retaining -our where applicable.
Apply this diff:
- independent signals (e.g., depth, ambiguity) are computed, normalised via + independent signals (e.g., depth, ambiguity) are computed, normalized via- compatible with Sigma normalisation and threshold schedules defined in the + compatible with Sigma normalization and threshold schedules defined in the- Commit to a near-term optimisation plan (post-training static INT8 + Commit to a near-term optimization plan (post-training static INT8- - **Static INT8 quantisation:** apply post-training static quantisation (weights + - **Static INT8 quantization:** apply post-training static quantization (weights- ## 11. Quantisation & Performance Targets + ## 11. Quantization & Performance Targets - - **Quantisation:** post-training static INT8 for Transformer and head; retain + - **Quantization:** post-training static INT8 for Transformer and head; retain- - Respects provider abstraction and returns a scalar depth compatible with - Sigma normalisation and weighting. + - Respects provider abstraction and returns a scalar depth compatible with + Sigma normalization and weighting.Also applies to: 29-31, 66-67, 109-111, 149-154, 219-221
23-33: Rewrite headings in sentence case per docs guidelines.Enforce sentence case across all headings.
Apply this diff:
-## 2. Problem Statement +## 2. Problem statement -## 3. Decision Drivers +## 3. Decision drivers -## 4. Options Considered +## 4. Options considered -## 5. Decision +## 5. Decision -## 6. Selected Architecture — Transformer-Ordinal +## 6. Selected architecture — Transformer‑ordinal -### I/O Signature +### I/O signature -### Ordinal Head +### Ordinal head -### Backbone & Representation +### Backbone & representation -### Graph Notes +### Graph notes -## 7. Short-Term Optimisations (Committed) +## 7. Short‑term optimizations (committed) -## 8. Long-Term Roadmap — Hybrid Fusion +## 8. Long‑term roadmap — hybrid fusion -## 9. Training & Labelling (Offline) +## 9. Training & labelling (offline) -## 10. Calibration +## 10. Calibration -## 11. Quantisation & Performance Targets +## 11. Quantization & performance targets -## 12. Integration Details +## 12. Integration details -## 13. Fixed-Feature Fallback — MLP-Log +## 13. Fixed‑feature fallback — MLP‑log -## 14. Testing Strategy +## 14. Testing strategy -## 15. Rollout Plan +## 15. Rollout plan -## 16. Risks & Mitigations +## 16. Risks & mitigations -## 17. Compliance with LAG Complexity Design +## 17. Compliance with LAG Complexity design -## 18. Operational Details & Versioning +## 18. Operational details & versioning -## 19. Open Questions +## 19. Open questions -### Appendix A — Minimal ONNX Graph Summary (Transformer-Ordinal) +### Appendix A — Minimal ONNX graph summary (Transformer‑ordinal) -### Appendix B — Acceptance Criteria +### Appendix B — Acceptance criteriaAlso applies to: 34-47, 48-61, 62-70, 71-79, 80-91, 92-98, 99-106, 107-114, 115-127, 128-140, 141-147, 148-155, 156-172, 173-181, 182-196, 197-204, 205-216, 217-225, 226-235, 236-243, 245-252, 253-261
75-79: Clarify batching, padding, and truncation semantics in the I/O signature.Specify max_seq_len, pad/truncate policy, and batch dynamism to prevent preprocessing drift across providers.
Apply this diff:
- Inputs: `input_ids: int64[batch, seq]`, `attention_mask: int64[batch, seq]` - (dynamic axes). + (dynamic batch). + - Sequence length: `seq = max_seq_len` (fixed). Truncate sequences from the right if longer. + - Padding: pad on the right to `max_seq_len` using `pad_token_id`; `attention_mask` uses 1 for real tokens, 0 for padding (int64). + - Batch: `batch` is dynamic; variable batch sizes are supported in one inference. - Outputs: `logits_ord: float32[batch, K]` where `K` is the number of ordered depth thresholds.
82-91: Disambiguate ordinal expectation mapping and index range.Define the exact step range (0..K or 1..K), threshold inclusivity, and the mid‑bin representative to avoid off‑by‑one and calibration drift.
Apply this diff:
- Cumulative/threshold encoding: head `k` predicts `P(depth > τ_k)` via `sigmoid`. - Scalar depth for the Cognitive Clutch is computed as either: - - **Expected value:** `E[steps] = Σ_k σ(logits_ord[k])`, optionally mapped to - a step scale; or + - **Expected value (1..K scale):** `E[steps] = 1 + Σ_{k=1..K-1} σ(logits_ord[k])` + where head `k` models `P(steps > k)`. Use this directly as the raw step + expectation on a 1..K scale. - **Mid-bin mapping:** count of heads above threshold → representative step - value. + value. Use bin centres: `rep_k = (lower_k + upper_k)/2` with `lower_1 = 1`, + `upper_K = K`. Map `count = Σ 1[σ(logits_ord[k]) ≥ 0.5]` to `rep_count`. - This scalar is returned as the provider’s **raw depth**, then normalised by Sigma and combined per the design document.
158-162: Rename provider fields to plural for clarity (collections).Use
input_namesandoutput_namesto remove ambiguity at integration time.Apply this diff:
-- **Provider type:** `DepthClassifierOnnx { session, input_name(s), output_name - }`. +- **Provider type:** `DepthClassifierOnnx { session, input_names, output_names }`. - **Trait:** implements `TextProcessor<Output = f32>`; returns scalar raw depth (expected steps or mapped mid-bin).
226-235: Pin ORT versions and enforce artefact checksums at load (fail closed).Document the crate and minimum runtime versions and mandate SHA‑256 verification for model and tokeniser artefacts.
Apply this diff:
## 18. Operational details & versioning @@ - **Filename & checksum:** `depth_transformer_ordinal.onnx` (and `depth_mlp_log.onnx` fallback) with SHA-256 recorded and verified at load. +- **ONNX Runtime (ORT):** pin `ort` crate to a version that supports opset 17 + (e.g., `ort = "=<X.Y.Z>"`) and document the minimum ONNX Runtime shared‑library + version required in production. +- **Checksum enforcement:** verify SHA‑256 for both the model and tokeniser vocab + artefacts at start‑up; fail closed on mismatch and surface the checksum in diagnostics. - **Versioning:** semantic version in ONNX metadata; minor for calibration changes, patch for weight updates without interface changes, major for I/O shape changes.
Adopt en-GB -ize spellings, document ORT runtime pinning and artefact checks, and fix design doc footnotes, links, and provider terminology.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
docs/lag-complexity-function-design.md (7)
1362-1362: Use sentence case for section heading.Change the heading to follow sentence case convention per the coding guidelines.
-- **Acceptance Criteria:** +- **Acceptance criteria:**
1388-1388: Use sentence case for section heading.Change the heading to follow sentence case convention per the coding guidelines.
-- **Acceptance Criteria:** +- **Acceptance criteria:**
1402-1402: Use sentence case for section heading.Change the heading to follow sentence case convention per the coding guidelines.
-- **Tasks:** +- **Tasks:**
1411-1411: Use sentence case for section heading.Change the heading to follow sentence case convention per the coding guidelines.
-- **Acceptance Criteria:** +- **Acceptance criteria:**
1424-1424: Use sentence case for section heading.Change the heading to follow sentence case convention per the coding guidelines.
-- **Tasks:** +- **Tasks:**
1445-1445: Use sentence case for section heading.Change the heading to follow sentence case convention per the coding guidelines.
-- **Tasks:** +- **Tasks:**
1455-1455: Use sentence case for section heading.Change the heading to follow sentence case convention per the coding guidelines.
-- **Acceptance Criteria:** +- **Acceptance criteria:**
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
docs/adr-depth-classifier-onnx.md(1 hunks)docs/lag-complexity-function-design.md(20 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
docs/**/*.{rs,md}
📄 CodeRabbit inference engine (docs/rust-doctest-dry-guide.md)
docs/**/*.{rs,md}: In code fences, explicitly specify the rust language (```rust) for doctest code blocks
Use assert!, assert_eq!, and assert_ne! in doctests to verify outcomes
Avoid using unwrap()/expect() in examples; handle errors idiomatically
For fallible examples, write an explicit fn main() -> Result<...> and hide the boilerplate lines with #
Optionally use the (()) shorthand at the end of a code block to make main return Result; ensure no whitespace within (())
Use hidden lines (#) to hide setup, use statements, and boilerplate in doctest examples
Prefer no_run for examples with undesirable side effects (network, FS, GUI) so they compile but don’t execute
Use should_panic for examples intended to panic; optionally assert specific messages
Use compile_fail for examples that should not compile, but be aware of its brittleness across compiler versions
Avoid ignore for Rust examples; reserve it for pseudocode or non-Rust blocks
Use edition20xx doctest attribute when demonstrating edition-specific syntax or behavior
Files:
docs/adr-depth-classifier-onnx.mddocs/lag-complexity-function-design.md
docs/**/*.md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
docs/**/*.md: Use British English (OED) spelling in documentation
The word “outwith” is acceptable in documentation
Keep US spelling when it appears in API names (e.g.,color)
Use the Oxford comma in lists
Treat company names as collective nouns (e.g., “Lille Industries are …”)
Write headings in sentence case
Use Markdown headings (#,##,###, …) in order without skipping levels
Follow markdownlint recommendations
Use standard Markdown syntax for code blocks and lists
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text
Use-for first-level bullets and renumber ordered lists when items change
Prefer inline links[text](url)or angle-bracket autolinks<url>
Ensure blank lines before and after bulleted lists and fenced code blocks
Ensure tables have a delimiter line below the header row
Expand uncommon acronyms on first use (e.g., Continuous Integration (CI))
Wrap paragraphs at 80 columns
Wrap code (in docs) at 120 columns
Do not wrap tables in documentation
Use footnotes referenced with[^label]
Include Mermaid diagrams where they add clarity
Embed images withand provide brief, descriptive alt text
Add a short descriptive sentence before each Mermaid diagram for screen readers
docs/**/*.md: Use docs/ markdown as reference and keep it updated with decisions and changes
Documentation must use en-GB-oxendict spelling and grammar (LICENSE naming unchanged)
Files:
docs/adr-depth-classifier-onnx.mddocs/lag-complexity-function-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown with make markdownlint
Run make fmt to format Markdown files and fix table markup after documentation changes
Validate Mermaid diagrams in Markdown via make nixie
Wrap Markdown paragraphs and bullet points at 80 columns
Wrap code blocks at 120 columns
Do not wrap tables and headings
Use dashes (-) for list bullets
Use GitHub-flavoured Markdown footnotes ([^1])
Files:
docs/adr-depth-classifier-onnx.mddocs/lag-complexity-function-design.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/adr-depth-classifier-onnx.mddocs/lag-complexity-function-design.md
🔍 Remote MCP Ref
Summary of additional, review-relevant facts found
-
ADR main choices and concrete specs
- Model: DistilBERT-class lightweight encoder + ordinal (Transformer‑Ordinal) head; outputs logits_ord (float32[batch,K]) and depth computed via expected value or mid‑bin mapping.
- ONNX target: opset 17; graph constrained (tokeniser → encoder → ordinal head), optional Mul/Add calibration nodes.
- I/O signatures: input_ids and attention_mask (int64) in, logits_ord (float32) out.
- Quantisation & perf targets: post‑training static INT8 quantisation planned; p95 CPU inference target ≤ 10 ms; acceptance goal ≤ 1% accuracy degradation vs FP32.
- Tokenization: deterministic Rust tokenizer required; tokenizer/vocab artifacts are called out as versioned artefacts in ADR.
-
Fallback, rollout, and integration
- Fallback: feature‑flagged ONNX MLP fallback provider (DepthClassifierMlpOnnx / “MLP‑Log”) behind a feature flag; explicit rollout, tracing, metrics, and calibration/validation paths stated.
- Design doc updates: docs/lag-complexity-function-design.md makes “ONNX Transformer‑Ordinal” the default provider and adds “ONNX MLP Fallback” with updated provider trade‑offs and acceptance/rollout notes.
-
CI / checks and PR metadata
- PR: Document transformer depth classifier architecture (PR #37). Author: leynos. Branch: codex/add-adr-for-depthclassifieronnx-and-design-update → main.
- Listed checks to run locally/CI: make fmt, make lint, make test, make markdownlint, make nixie.
Reviewer checkpoints (concrete items to verify in repo/CI/tooling)
- Confirm provider trait interfaces, feature‑flag wiring, and new provider names (ONNX Transformer‑Ordinal, DepthClassifierMlpOnnx) exist or are planned in code.,
- Verify presence/version of deterministic Rust tokenizer artifacts (vocab/tokenizer) and where they will be stored/loaded.
- Validate CI/tooling support for ONNX opset 17 and static INT8 post‑training quantisation (converter versions, calibration tool availability in CI).
- Check performance/accuracy test harnesses referenced (p95 latency measurement, FP32→INT8 accuracy drift tests) exist in tests or benchmark CI.
- Ensure rollout/tracing/metrics hooks mentioned in ADR map to existing observability/tracing frameworks in the repo.
Sources
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (15)
docs/lag-complexity-function-design.md (12)
89-91: LGTM!The revision enhances clarity by explicitly stating that the crate serves not just as an implementation detail but as a foundational architectural choice. The emphasis on testability, maintainability, and scalability aligns well with production engineering principles.
472-500: Document matches external tools findings on Transformer-Ordinal architecture.The updated provider description accurately reflects the ADR specifications: DistilBERT-class encoder with ordinal regression head, deterministic Rust tokenisation, ONNX opset 17, and SHA-256 verification for artefacts. The performance targets (p95 ≤ 10 ms CPU) and post-training quantisation roadmap align with the committed optimisation plan.
544-549: LGTM!The implementation notes provide clear technical details about shared normalisation, conservative singularisation via text utils, word-boundary matching with regex, and Laplace smoothing. The deferred antecedent resolution to model-based providers is a sensible architectural choice.
555-560: LGTM!The runtime and artefact controls match the ADR specifications, including shared
ortdependency, checksum verification, and fail-closed behaviour for mismatched digests.
587-592: Provider trade-offs table accurately reflects updated architecture.The table correctly positions ONNX Transformer-Ordinal as the production default with high accuracy and low latency (~5-10ms), while the ONNX MLP fallback addresses tokenisation-constrained environments. The use cases and dependencies align with the ADR.
719-724: Correct API reference aligns with trait design.The update to "calling
process()on the embedding, depth, and ambiguity providers" accurately reflects theTextProcessortrait method rather than the previous placeholder names. This addresses the past review comment and ensures clarity.
736-741: Library justification is well-reasoned.The explanation of choosing
mokaoverdashmapclearly articulates the caching-specific benefits (LRU/LFU policies, TTL/TTI) whilst acknowledgingdashmap's general-purpose utility.
799-802: Security guidance is comprehensive.The PII scrubbing hook documentation appropriately references Presidio-inspired solutions whilst maintaining platform-agnostic guidance for compliance in regulated domains.
1330-1339: LGTM!The import path for
ortho_configis correctly documented as importing fromortho-configon crates.io, with the precedence order clearly stated. The configuration layering follows best practices.
1376-1382: Implementation details align with ADR commitments.The tasks correctly specify training and exporting both the Transformer-Ordinal depth model and INT8 variant, alongside the MLP fallback. The mention of calibration switches, deterministic tokenisation, and SHA-256 verification matches the ADR specifications.
866-866: Use sentence case for section heading.Change "Behavioural properties" to follow sentence case convention per the coding guidelines.
-##### Behavioural properties +##### Behavioural propertiesLikely an incorrect or invalid review comment.
1374-1374: Use sentence case for section heading.Change the heading to follow sentence case convention per the coding guidelines.
-- **Tasks:** +- **Tasks:**Likely an incorrect or invalid review comment.
docs/adr-depth-classifier-onnx.md (3)
1-269: Comprehensive ADR for Transformer-Ordinal architecture.This ADR thoroughly documents the architectural decision for the DepthClassifierOnnx provider. The structure follows standard ADR format with clear problem statement, decision drivers, evaluated options, and selected solution. The technical specifications align with the updates in the main design document.
75-78: Specify batch and sequence dimension constraints.Add explicit constraints for maximum sequence length, padding/truncation policy, and batch dimension behaviour to prevent preprocessing inconsistencies across platforms.
In docs/adr-depth-classifier-onnx.md around lines 75-78, specify the maximum sequence length (e.g., max_seq_len = 512), declare whether the batch dimension supports dynamic shapes or is fixed, clarify padding policy (pad token ID and pad_side left/right), truncation policy (truncate from left or right), and confirm that attention_mask uses 1/0 integers matching the input dtype. Update the I/O signature to include these constraints for unambiguous preprocessing.
158-159: Use plural field names for collections.Change singular field references to plural forms for clarity and consistency with collection types.
-**Provider type:** `DepthClassifierOnnx { session, input_name(s), output_name -}`. +**Provider type:** `DepthClassifierOnnx { session, input_names, output_names }`.
| #### Computational Overhead: | ||
|
|
||
| #### Computational Overhead |
There was a problem hiding this comment.
🧹 Nitpick
Use sentence case for section heading.
Change "Computational Overhead" to follow sentence case convention per the coding guidelines.
-#### Computational Overhead
+#### Computational overhead📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #### Computational Overhead | |
| #### Computational overhead |
🤖 Prompt for AI Agents
In docs/lag-complexity-function-design.md around line 1014, the section heading
"Computational Overhead" uses title case; change it to sentence case per
guidelines by editing the heading text to "Computational overhead" (only the
first word capitalized) and ensure any linked anchors or references remain
consistent after the change.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/lag-complexity-function-design.md (2)
428-447: Fix heading capitalisation.Follow the sentence-case heading convention in docs and update this heading accordingly.
-##### Feature Engineering (Depth) +##### Feature engineering (depth)
525-548: Fix heading capitalisation.Keep headings in sentence case per documentation standards and update this one.
-##### Feature Engineering (Ambiguity) +##### Feature engineering (ambiguity)docs/adr-depth-classifier-onnx.md (1)
1-273: Wrap paragraphs at 80 columns per docs style.Reflow prose to satisfy markdownlint and repository style. Headings are fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.toml(2 hunks)docs/adr-depth-classifier-onnx.md(1 hunks)docs/lag-complexity-function-design.md(22 hunks)src/lib.rs(1 hunks)src/providers/mod.rs(1 hunks)src/providers/onnx/aggregation.rs(1 hunks)src/providers/onnx/artefact.rs(1 hunks)src/providers/onnx/classifier.rs(1 hunks)src/providers/onnx/config.rs(1 hunks)src/providers/onnx/errors.rs(1 hunks)src/providers/onnx/mod.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use functions and composition; extract reusable logic; prefer generators/comprehensions and declarative code when readable
Small, single-responsibility functions; obey command/query segregation
Name things precisely; boolean names prefixed with is/has/should
Use consistent en-GB-oxendict spelling/grammar in comments (except external API names)
Keep file size manageable: no code file >400 lines; break up large switches/dispatch; move large test data to external files
Fix warnings emitted during tests in code rather than silencing them
Extract helper functions when functions are too long; adhere to separation of concerns and CQRS
Group many parameters into well-named structs
For large error returns, consider using Arc to reduce returned data
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless absolutely necessary and document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditional criteria with more than two branches
Do not silence lints except as last resort
Lint suppressions must be tightly scoped and include a clear reason
Prefer #[expect] over #[allow] for lint management
Use conditional compilation (#[cfg]/#[cfg_attr]) for functions unused under specific features
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than backslash newline escapes
Prefer single-line function forms where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Use an opaque error (eyre::Report) only at the app boundary for logging; not in public APIs
Never export eyre::Report from libraries; convert to domain enums at API boundaries and to eyre only in main() or top-level async task
Files:
src/providers/mod.rssrc/providers/onnx/config.rssrc/providers/onnx/aggregation.rssrc/providers/onnx/classifier.rssrc/providers/onnx/mod.rssrc/providers/onnx/artefact.rssrc/providers/onnx/errors.rssrc/lib.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
src/providers/mod.rssrc/providers/onnx/config.rssrc/providers/onnx/aggregation.rssrc/providers/onnx/classifier.rssrc/providers/onnx/mod.rssrc/providers/onnx/artefact.rssrc/providers/onnx/errors.rssrc/lib.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.rs: Function documentation must include clear examples; test docs should omit redundant examples
Every module must begin with a module-level //! comment explaining purpose and utility
Document public APIs using Rustdoc (///) so cargo doc can generate docs
Files:
src/providers/mod.rssrc/providers/onnx/config.rssrc/providers/onnx/aggregation.rssrc/providers/onnx/classifier.rssrc/providers/onnx/mod.rssrc/providers/onnx/artefact.rssrc/providers/onnx/errors.rssrc/lib.rs
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Cargo.toml: Use explicit version ranges; keep dependencies up-to-date
Mandate caret requirements for all dependencies (e.g., "1.2.3")
Prohibit wildcard (*) or open-ended (>=) version specifiers; allow ~ only with specific, documented reason
Files:
Cargo.toml
docs/**/*.{rs,md}
📄 CodeRabbit inference engine (docs/rust-doctest-dry-guide.md)
docs/**/*.{rs,md}: In code fences, explicitly specify the rust language (```rust) for doctest code blocks
Use assert!, assert_eq!, and assert_ne! in doctests to verify outcomes
Avoid using unwrap()/expect() in examples; handle errors idiomatically
For fallible examples, write an explicit fn main() -> Result<...> and hide the boilerplate lines with #
Optionally use the (()) shorthand at the end of a code block to make main return Result; ensure no whitespace within (())
Use hidden lines (#) to hide setup, use statements, and boilerplate in doctest examples
Prefer no_run for examples with undesirable side effects (network, FS, GUI) so they compile but don’t execute
Use should_panic for examples intended to panic; optionally assert specific messages
Use compile_fail for examples that should not compile, but be aware of its brittleness across compiler versions
Avoid ignore for Rust examples; reserve it for pseudocode or non-Rust blocks
Use edition20xx doctest attribute when demonstrating edition-specific syntax or behavior
Files:
docs/adr-depth-classifier-onnx.mddocs/lag-complexity-function-design.md
docs/**/*.md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
docs/**/*.md: Use British English (OED) spelling in documentation
The word “outwith” is acceptable in documentation
Keep US spelling when it appears in API names (e.g.,color)
Use the Oxford comma in lists
Treat company names as collective nouns (e.g., “Lille Industries are …”)
Write headings in sentence case
Use Markdown headings (#,##,###, …) in order without skipping levels
Follow markdownlint recommendations
Use standard Markdown syntax for code blocks and lists
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text
Use-for first-level bullets and renumber ordered lists when items change
Prefer inline links[text](url)or angle-bracket autolinks<url>
Ensure blank lines before and after bulleted lists and fenced code blocks
Ensure tables have a delimiter line below the header row
Expand uncommon acronyms on first use (e.g., Continuous Integration (CI))
Wrap paragraphs at 80 columns
Wrap code (in docs) at 120 columns
Do not wrap tables in documentation
Use footnotes referenced with[^label]
Include Mermaid diagrams where they add clarity
Embed images withand provide brief, descriptive alt text
Add a short descriptive sentence before each Mermaid diagram for screen readers
docs/**/*.md: Use docs/ markdown as reference and keep it updated with decisions and changes
Documentation must use en-GB-oxendict spelling and grammar (LICENSE naming unchanged)
Files:
docs/adr-depth-classifier-onnx.mddocs/lag-complexity-function-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown with make markdownlint
Run make fmt to format Markdown files and fix table markup after documentation changes
Validate Mermaid diagrams in Markdown via make nixie
Wrap Markdown paragraphs and bullet points at 80 columns
Wrap code blocks at 120 columns
Do not wrap tables and headings
Use dashes (-) for list bullets
Use GitHub-flavoured Markdown footnotes ([^1])
Files:
docs/adr-depth-classifier-onnx.mddocs/lag-complexity-function-design.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/adr-depth-classifier-onnx.mddocs/lag-complexity-function-design.md
🧬 Code graph analysis (3)
src/providers/onnx/classifier.rs (1)
src/providers/onnx/mod.rs (2)
new(26-30)new(55-59)
src/providers/onnx/mod.rs (2)
src/providers/onnx/classifier.rs (1)
new(30-91)src/providers/mod.rs (1)
process(32-32)
src/providers/onnx/artefact.rs (1)
src/providers/onnx/classifier.rs (1)
new(30-91)
🔍 Remote MCP Ref
Summary — additional review-relevant facts (concise)
-
ADR concrete specs to verify in code/CI:
- Model: DistilBERT-class Transformer‑Ordinal encoder + ordinal head; ONNX opset 17 target; I/O: input_ids & attention_mask (int64) → logits_ord (float32[K]). Verify these exact names/types in any model artefact loading and ONNX session code.
- Quantisation & perf targets: planned post‑training static INT8 quantisation; p95 CPU inference target ≤ 10 ms; acceptance ≤ 1% FP32→INT8 accuracy degradation. Confirm CI/tooling supports opset17→INT8 (converter/calibration tool versions) and that benchmarks exist or are planned.
- Tokeniser artefacts: deterministic Rust tokenizer required; tokenizer/vocab must be versioned artefacts (SHA‑256 checked) and loaded consistently with model. Verify OnnxArtefact checksum logic and where artefacts are stored/expected.
-
Code/API wiring items to check (present in this PR):
- Feature gating: new "onnx" feature (Cargo.toml) and conditional pub re-exports (src/lib.rs, src/providers/mod.rs). Ensure feature names and pub exports match docs and downstream consumers.
- New public types/modules to review: OnnxArtefact, OnnxClassifierConfig, OnnxTextClassifier (classifier.rs), OrdinalAggregation (aggregation.rs), OnnxClassifierError and wrappers, DepthClassifierOnnx and AmbiguityClassifierOnnx wrappers + TextProcessor impls. Check visibility, error conversions, and API ergonomics.
- Artefact checksum: compute_sha256/normalise_hex + verify -> maps errors to OnnxClassifierError::Io/ChecksumMismatch. Confirm tests cover edge cases (non‑existent file, differing hex normalization).
-
Behavioural/validation checks encoded in new modules:
- OrdinalAggregation::validate enforces MidBin bin_midpoints.len() == head_count + 1; OnnxTextClassifier validates head count vs outputs and sequence length constraints; logistic monotonicity test present. Ensure these runtime checks and error messages match ADR acceptance criteria.
- OnnxTextClassifier::predict flow: tokenise → build tensors → run session → convert logits→ probs → aggregate. Confirm handling of types (int64 inputs), padding/truncation, and tensor shape expectations match ONNX graph exported per ADR.
-
Testing / CI notes to confirm:
- PR lists local checks (make fmt, lint, test, markdownlint, nixie). Confirm CI pipeline runs these and add/enable any ONNX/quantisation tooling or benchmark jobs required for performance/INT8 validation.
Sources consulted
- PR metadata and changed files (PR #37).
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (10)
src/providers/mod.rs (1)
7-9: Gate ONNX provider behind feature — LGTM.The module boundary is clean and correctly feature-gated.
src/lib.rs (1)
26-31: Feature-gated re-exports — LGTM.The public surface matches the new module layout and remains hidden when the feature is disabled.
docs/adr-depth-classifier-onnx.md (1)
88-95: Lock ordinal expectation mapping semantics.State explicitly the chosen step range and mapping to avoid off-by-one drift; specify whether the returned scalar is in 0..=K or 1..=K and how it is scaled before Sigma.
src/providers/onnx/config.rs (1)
9-9: Align terminology with API name “tokenizer”.Prefer “tokenizer” when referencing the crate/API name.
- /// Tokeniser artefact consumed by `tokenizers`. + /// Tokenizer artefact consumed by `tokenizers`.Likely an incorrect or invalid review comment.
src/providers/onnx/classifier.rs (3)
124-131: Validate ort TensorRef::from_array_view shape type expectationsEnsure the shape argument type matches the API. If the API expects i64 dims, pass [i64; 2] not [usize; 2].
Run this query to confirm the exact signature:
In crate ort 0.5.0-alpha2, what is the exact signature of value::TensorRef::from_array_view? What types are accepted for the shape dimensions?
178-205: Ordinal aggregation implementation looks correctLogistic conversion per head and both ExpectedValue and MidBin paths align with ordinal semantics; validation guards are in place.
Add parameterised tests for both aggregators across boundary cases (0 heads, all probs < threshold, all probs ≥ threshold).
1-1: Remove session Mutex to avoid serialising inferenceReplace
Arc<Mutex<Session>>withArc<Session>, drop the lock andSessionPoisonedbranch. Apply this diff:-use std::sync::{Arc, Mutex}; +use std::sync::Arc; @@ pub struct OnnxTextClassifier { - session: Arc<Mutex<Session>>, + session: Arc<Session>, @@ Ok(Self { - session: Arc::new(Mutex::new(session)), + session: Arc::new(session), @@ - let mut session = self - .session - .lock() - .map_err(|_| OnnxClassifierError::SessionPoisoned)?; + // Session is thread-safe; no lock required.Verify that
Session::runin ort v0.5.0-alpha2 takes&self, not&mut self. Applies to lines 13–22, 82–90, 133–137.src/providers/onnx/mod.rs (3)
20-31: Constructor docs are solid; keep mapping minimalThe wrapper constructors correctly delegate and map errors.
Add unit tests asserting error mapping invariants (OnnxClassifierError variants map to the correct wrapper variants).
Also applies to: 49-60
33-42: Trait implementation is minimal and correctForwarding process() to inner.predict() is appropriate here.
Also applies to: 62-71
1-1: Remove redundant file-scope feature gate
Parent modulesrc/providers/mod.rsalready gatespub mod onnxwith#[cfg(feature = "onnx")].Likely an incorrect or invalid review comment.
| ort = { version = "2.0.0-rc.10", optional = true, default-features = false, features = ["download-binaries", "ndarray", "std"] } | ||
| tokenizers = { version = "0.22.1", optional = true, default-features = false, features = ["fancy-regex"] } |
There was a problem hiding this comment.
Remove download-binaries and prefer load-dynamic for reproducible builds.
Avoid network downloads during builds; gate runtime discovery via ORT’s load-dynamic instead. Keep download-binaries only behind a non-default, explicitly opt-in feature.
Apply this diff:
-ort = { version = "2.0.0-rc.10", optional = true, default-features = false, features = ["download-binaries", "ndarray", "std"] }
+ort = { version = "2.0.0-rc.10", optional = true, default-features = false, features = ["load-dynamic", "ndarray", "std"] }Add an opt-in feature (outside this hunk):
[features]
# ...
onnx-download = ["onnx", "ort/download-binaries"]🤖 Prompt for AI Agents
In Cargo.toml around lines 25-26, remove "download-binaries" from the ort
feature list and replace runtime discovery to prefer load-dynamic; then add a
new opt-in feature entry under [features] named onnx-download that includes
"onnx" and "ort/download-binaries" so binary downloads are only enabled when
explicitly opted in and default builds remain reproducible.
| regex = "1.10" | ||
| reqwest = { version = "0.11", default-features = false, features = ["blocking", "json", "rustls-tls"], optional = true } | ||
| ort = { version = "2.0.0-rc.10", optional = true, default-features = false, features = ["download-binaries", "ndarray", "std"] } | ||
| tokenizers = { version = "0.22.1", optional = true, default-features = false, features = ["fancy-regex"] } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Drop unnecessary tokenizers feature to reduce compile time and deps.
Remove fancy-regex unless specifically required at runtime; it materially increases compile time.
-tokenizers = { version = "0.22.1", optional = true, default-features = false, features = ["fancy-regex"] }
+tokenizers = { version = "0.22.1", optional = true, default-features = false }🤖 Prompt for AI Agents
In Cargo.toml around line 26, the tokenizers dependency includes the
"fancy-regex" feature which increases compile time; remove the fancy-regex
feature from the tokenizers entry (either delete the features = ["fancy-regex"]
clause or remove "fancy-regex" from the features list) so the dependency uses
the default/declared minimal features; save the file and run cargo build to
verify no runtime-only functionality depended on fancy-regex was broken.
| yaml = ["cli", "figment/yaml", "dep:serde_yaml"] | ||
| toml = ["cli", "figment/toml", "dep:toml"] | ||
| provider-api = ["dep:reqwest"] | ||
| onnx = ["dep:ort", "dep:tokenizers", "dep:sha2", "dep:ndarray"] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Confirm ndarray is required at runtime for ONNX path.
If ndarray is not used in the ONNX provider code path, remove it from the onnx feature to trim the dependency graph.
🤖 Prompt for AI Agents
In Cargo.toml around line 37, the onnx feature currently enables "dep:ndarray"
but the reviewer asks to confirm whether ndarray is required at runtime for the
ONNX provider; inspect the ONNX provider code paths gated by the "onnx" feature
to see if any public or runtime code directly depends on ndarray (including
re-exports, function signatures, or type aliases). If ndarray is not used at
runtime, remove "dep:ndarray" from the onnx feature entry in Cargo.toml and run
cargo build/test to ensure no compile errors; if it is required, leave it and
annotate why (or move it to a more specific feature) so the dependency graph is
justified. Ensure any changes are reflected in feature docs and CI.
| - Inputs: `input_ids: int64[batch, seq]`, `attention_mask: int64[batch, seq]`. | ||
| The Rust tokeniser right-pads and right-truncates every request to a fixed | ||
| `seq = max_seq_len` (512 tokens for the launch build). `batch` is dynamic in | ||
| the ONNX graph, but the provider currently executes single-query batches. | ||
| - Outputs: `logits_ord: float32[batch, K]` where `K` is the number of ordered | ||
| depth thresholds. The runtime reads the first batch element and discards the | ||
| rest, preserving compatibility with future batched inference. | ||
|
|
There was a problem hiding this comment.
Unify I/O notation: fix seq to max_seq_len as used elsewhere.
Keep sequence length fixed to max_seq_len across the doc to avoid ambiguity.
-- Inputs: `input_ids: int64[batch, seq]`, `attention_mask: int64[batch, seq]`.
+- Inputs: `input_ids: int64[batch, max_seq_len]`, `attention_mask: int64[batch, max_seq_len]`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - Inputs: `input_ids: int64[batch, seq]`, `attention_mask: int64[batch, seq]`. | |
| The Rust tokeniser right-pads and right-truncates every request to a fixed | |
| `seq = max_seq_len` (512 tokens for the launch build). `batch` is dynamic in | |
| the ONNX graph, but the provider currently executes single-query batches. | |
| - Outputs: `logits_ord: float32[batch, K]` where `K` is the number of ordered | |
| depth thresholds. The runtime reads the first batch element and discards the | |
| rest, preserving compatibility with future batched inference. | |
| - Inputs: `input_ids: int64[batch, max_seq_len]`, `attention_mask: int64[batch, max_seq_len]`. | |
| The Rust tokeniser right-pads and right-truncates every request to a fixed | |
| `seq = max_seq_len` (512 tokens for the launch build). `batch` is dynamic in | |
| the ONNX graph, but the provider currently executes single-query batches. | |
| - Outputs: `logits_ord: float32[batch, K]` where `K` is the number of ordered | |
| depth thresholds. The runtime reads the first batch element and discards the | |
| rest, preserving compatibility with future batched inference. |
🤖 Prompt for AI Agents
In docs/adr-depth-classifier-onnx.md around lines 75 to 82, the Inputs section
uses `seq` ambiguously; change the I/O notation to explicitly fix sequence
length to `seq = max_seq_len` (512) to match the rest of the document and the
Rust tokenizer behavior, i.e., state Inputs as `input_ids: int64[batch,
max_seq_len]`, `attention_mask: int64[batch, max_seq_len]`, and clarify that
batch is dynamic in the ONNX graph while runtime executes single-query batches;
keep Outputs unchanged but ensure any mention of `seq` elsewhere uses
`max_seq_len`.
| - **Runtime pinning:** the `ort` crate is fixed to the `2.0.x` series (bundling | ||
| ONNX Runtime 1.18), ensuring the shipped runtime matches the opset required | ||
| by the artefacts. | ||
| - **Error handling:** ONNX Runtime errors mapped to `Error::Inference` with | ||
| model path, opset, and checksum in diagnostics. | ||
| - **Tracing & metrics:** instrument `process()`; export latency histograms, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Align text with manifest: avoid bundling ORT via downloads by default.
Clarify that the crate uses load-dynamic by default and only uses download-binaries behind an opt-in feature (see Cargo.toml change).
🤖 Prompt for AI Agents
In docs/adr-depth-classifier-onnx.md around lines 169 to 174, the runtime
pinning paragraph incorrectly implies ORT is bundled by default; update the text
to state that the crate uses the ORT "load-dynamic" backend by default and that
the "download-binaries" behavior is an opt-in Cargo feature (per Cargo.toml), so
avoid wording that suggests downloading/bundling ORT binaries unless the feature
is enabled; adjust the bullet to explicitly mention default load-dynamic
behavior and note the opt-in feature name for clarity.
| /// Errors produced by the shared ONNX classifier implementation. | ||
| #[derive(Debug, Error)] | ||
| pub enum OnnxClassifierError { | ||
| #[error("failed to read artefact at {path}: {source}")] | ||
| Io { | ||
| path: PathBuf, | ||
| #[source] | ||
| source: std::io::Error, | ||
| }, | ||
| #[error("artefact at {path} expected SHA-256 {expected} but found {actual}")] | ||
| ChecksumMismatch { | ||
| path: PathBuf, | ||
| expected: String, | ||
| actual: String, | ||
| }, | ||
| #[error("failed to load tokenizer from {path}: {source}")] | ||
| LoadTokenizer { | ||
| path: PathBuf, | ||
| #[source] | ||
| source: tokenizers::Error, | ||
| }, | ||
| #[error("failed to configure tokenizer truncation: {0}")] | ||
| ConfigureTruncation(#[source] tokenizers::Error), | ||
| #[error("failed to construct ONNX session builder: {0}")] | ||
| CreateSessionBuilder(#[source] ort::Error), | ||
| #[error("failed to create ONNX session: {0}")] | ||
| CreateSession(#[source] ort::Error), | ||
| #[error("classifier requires at least one input name")] | ||
| MissingInputNames, | ||
| #[error("classifier requires at least {expected} input names but found {actual}")] | ||
| InsufficientInputNames { expected: usize, actual: usize }, | ||
| #[error("classifier requires at least one output name")] | ||
| MissingOutputNames, | ||
| #[error("classifier requires max_sequence_length > 0")] | ||
| ZeroSequenceLength, | ||
| #[error("classifier requires ordinal_head_count > 0")] | ||
| ZeroHeadCount, | ||
| #[error("tokenizer produced sequence of length {actual} but expected {expected}")] | ||
| SequenceLength { expected: usize, actual: usize }, | ||
| #[error("failed to encode text: {0}")] | ||
| Encode(#[source] tokenizers::Error), | ||
| #[error("failed to convert encoded text into tensor: {0}")] | ||
| EncodeTensor(#[source] ort::Error), | ||
| #[error("session mutex was poisoned by a previous panic")] | ||
| SessionPoisoned, | ||
| #[error("failed to run inference: {0}")] | ||
| Inference(#[source] ort::Error), | ||
| #[error("ONNX output \"{name}\" missing from session results")] | ||
| OutputMissing { name: String }, | ||
| #[error("ONNX output \"{name}\" has {actual} logits but expected {expected}")] | ||
| UnexpectedLogitCount { | ||
| name: String, | ||
| expected: usize, | ||
| actual: usize, | ||
| }, | ||
| #[error("mid-bin mapping requires {expected} bins but found {actual}")] | ||
| InvalidBinCount { expected: usize, actual: usize }, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Mark error enums non-exhaustive and include context for session/inference errors.
Future-proof the public API and improve diagnostics by adding #[non_exhaustive] and including the model path for CreateSession/Infernce.
-#[derive(Debug, Error)]
-pub enum OnnxClassifierError {
+#[derive(Debug, Error)]
+#[non_exhaustive]
+pub enum OnnxClassifierError {
@@
- #[error("failed to create ONNX session: {0}")]
- CreateSession(#[source] ort::Error),
+ #[error("failed to create ONNX session for {path}: {source}")]
+ CreateSession {
+ path: PathBuf,
+ #[source]
+ source: ort::Error,
+ },
@@
- #[error("failed to run inference: {0}")]
- Inference(#[source] ort::Error),
+ #[error("failed to run inference on {path}: {source}")]
+ Inference {
+ path: PathBuf,
+ #[source]
+ source: ort::Error,
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Errors produced by the shared ONNX classifier implementation. | |
| #[derive(Debug, Error)] | |
| pub enum OnnxClassifierError { | |
| #[error("failed to read artefact at {path}: {source}")] | |
| Io { | |
| path: PathBuf, | |
| #[source] | |
| source: std::io::Error, | |
| }, | |
| #[error("artefact at {path} expected SHA-256 {expected} but found {actual}")] | |
| ChecksumMismatch { | |
| path: PathBuf, | |
| expected: String, | |
| actual: String, | |
| }, | |
| #[error("failed to load tokenizer from {path}: {source}")] | |
| LoadTokenizer { | |
| path: PathBuf, | |
| #[source] | |
| source: tokenizers::Error, | |
| }, | |
| #[error("failed to configure tokenizer truncation: {0}")] | |
| ConfigureTruncation(#[source] tokenizers::Error), | |
| #[error("failed to construct ONNX session builder: {0}")] | |
| CreateSessionBuilder(#[source] ort::Error), | |
| #[error("failed to create ONNX session: {0}")] | |
| CreateSession(#[source] ort::Error), | |
| #[error("classifier requires at least one input name")] | |
| MissingInputNames, | |
| #[error("classifier requires at least {expected} input names but found {actual}")] | |
| InsufficientInputNames { expected: usize, actual: usize }, | |
| #[error("classifier requires at least one output name")] | |
| MissingOutputNames, | |
| #[error("classifier requires max_sequence_length > 0")] | |
| ZeroSequenceLength, | |
| #[error("classifier requires ordinal_head_count > 0")] | |
| ZeroHeadCount, | |
| #[error("tokenizer produced sequence of length {actual} but expected {expected}")] | |
| SequenceLength { expected: usize, actual: usize }, | |
| #[error("failed to encode text: {0}")] | |
| Encode(#[source] tokenizers::Error), | |
| #[error("failed to convert encoded text into tensor: {0}")] | |
| EncodeTensor(#[source] ort::Error), | |
| #[error("session mutex was poisoned by a previous panic")] | |
| SessionPoisoned, | |
| #[error("failed to run inference: {0}")] | |
| Inference(#[source] ort::Error), | |
| #[error("ONNX output \"{name}\" missing from session results")] | |
| OutputMissing { name: String }, | |
| #[error("ONNX output \"{name}\" has {actual} logits but expected {expected}")] | |
| UnexpectedLogitCount { | |
| name: String, | |
| expected: usize, | |
| actual: usize, | |
| }, | |
| #[error("mid-bin mapping requires {expected} bins but found {actual}")] | |
| InvalidBinCount { expected: usize, actual: usize }, | |
| } | |
| /// Errors produced by the shared ONNX classifier implementation. | |
| #[derive(Debug, Error)] | |
| #[non_exhaustive] | |
| pub enum OnnxClassifierError { | |
| #[error("failed to read artefact at {path}: {source}")] | |
| Io { | |
| path: PathBuf, | |
| #[source] | |
| source: std::io::Error, | |
| }, | |
| #[error("artefact at {path} expected SHA-256 {expected} but found {actual}")] | |
| ChecksumMismatch { | |
| path: PathBuf, | |
| expected: String, | |
| actual: String, | |
| }, | |
| #[error("failed to load tokenizer from {path}: {source}")] | |
| LoadTokenizer { | |
| path: PathBuf, | |
| #[source] | |
| source: tokenizers::Error, | |
| }, | |
| #[error("failed to configure tokenizer truncation: {0}")] | |
| ConfigureTruncation(#[source] tokenizers::Error), | |
| #[error("failed to construct ONNX session builder: {0}")] | |
| CreateSessionBuilder(#[source] ort::Error), | |
| #[error("failed to create ONNX session for {path}: {source}")] | |
| CreateSession { | |
| path: PathBuf, | |
| #[source] | |
| source: ort::Error, | |
| }, | |
| #[error("classifier requires at least one input name")] | |
| MissingInputNames, | |
| #[error("classifier requires at least {expected} input names but found {actual}")] | |
| InsufficientInputNames { expected: usize, actual: usize }, | |
| #[error("classifier requires at least one output name")] | |
| MissingOutputNames, | |
| #[error("classifier requires max_sequence_length > 0")] | |
| ZeroSequenceLength, | |
| #[error("classifier requires ordinal_head_count > 0")] | |
| ZeroHeadCount, | |
| #[error("tokenizer produced sequence of length {actual} but expected {expected}")] | |
| SequenceLength { expected: usize, actual: usize }, | |
| #[error("failed to encode text: {0}")] | |
| Encode(#[source] tokenizers::Error), | |
| #[error("failed to convert encoded text into tensor: {0}")] | |
| EncodeTensor(#[source] ort::Error), | |
| #[error("session mutex was poisoned by a previous panic")] | |
| SessionPoisoned, | |
| #[error("failed to run inference on {path}: {source}")] | |
| Inference { | |
| path: PathBuf, | |
| #[source] | |
| source: ort::Error, | |
| }, | |
| #[error("ONNX output \"{name}\" missing from session results")] | |
| OutputMissing { name: String }, | |
| #[error("ONNX output \"{name}\" has {actual} logits but expected {expected}")] | |
| UnexpectedLogitCount { | |
| name: String, | |
| expected: usize, | |
| actual: usize, | |
| }, | |
| #[error("mid-bin mapping requires {expected} bins but found {actual}")] | |
| InvalidBinCount { expected: usize, actual: usize }, | |
| } |
🤖 Prompt for AI Agents
In src/providers/onnx/errors.rs around lines 5 to 62, mark the public error enum
as #[non_exhaustive] and extend the CreateSession and Inference variants to
include the model path (e.g. model: PathBuf) so errors carry context about which
model caused the failure; update their error messages to include the model/path
field and keep the existing source ort::Error as #[source]; ensure imports
(PathBuf) remain available and adjust any call sites that construct these
variants to pass the model path.
| #[error("mid-bin mapping requires {expected} bins but found {actual}")] | ||
| InvalidBinCount { expected: usize, actual: usize }, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add an explicit validation error for decision-threshold bounds.
Validate MidBin.decision_threshold ∈ [0.0, 1.0] with a dedicated error.
#[error("mid-bin mapping requires {expected} bins but found {actual}")]
InvalidBinCount { expected: usize, actual: usize },
+ #[error("decision threshold must be within [{min}, {max}] but was {actual}")]
+ InvalidDecisionThreshold { min: f32, max: f32, actual: f32 },🤖 Prompt for AI Agents
In src/providers/onnx/errors.rs around lines 60-62, add a new thiserror enum
variant that represents an out-of-range decision-threshold (e.g.,
DecisionThresholdOutOfBounds { value: f32 }) with an #[error("decision_threshold
must be in [0.0, 1.0], got {value}")] message; update any places that validate
MidBin.decision_threshold to return this specific error when the value is < 0.0
or > 1.0 so callers get a dedicated, clear validation error.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/lag-complexity-function-design.md (2)
438-439: Switch to en‑GB spelling (“penalise”).Align with docs spelling guidelines.
Apply this diff:
- - **Dependency Chains:** It will penalize sequences of prepositional phrases, + - **Dependency Chains:** It will penalise sequences of prepositional phrases,
601-607: Use en‑GB spelling in the section heading.Rename “Normalization” → “Normalisation”.
Apply this diff:
-### Normalization (sigma) +### Normalisation (sigma) - -The raw scores produced by the various providers—embedding variance, heuristic +The raw scores produced by the various providers—embedding variance, heuristic counts, or model outputs—exist on different, arbitrary scales. The `Sigma` - normalization module transforms these raw scores into a consistent, comparable + normalisation module transforms these raw scores into a consistent, comparable
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
docs/lag-complexity-function-design.md(22 hunks)src/providers/onnx/aggregation.rs(1 hunks)src/providers/onnx/errors.rs(1 hunks)src/providers/onnx/mod.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use functions and composition; extract reusable logic; prefer generators/comprehensions and declarative code when readable
Small, single-responsibility functions; obey command/query segregation
Name things precisely; boolean names prefixed with is/has/should
Use consistent en-GB-oxendict spelling/grammar in comments (except external API names)
Keep file size manageable: no code file >400 lines; break up large switches/dispatch; move large test data to external files
Fix warnings emitted during tests in code rather than silencing them
Extract helper functions when functions are too long; adhere to separation of concerns and CQRS
Group many parameters into well-named structs
For large error returns, consider using Arc to reduce returned data
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless absolutely necessary and document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditional criteria with more than two branches
Do not silence lints except as last resort
Lint suppressions must be tightly scoped and include a clear reason
Prefer #[expect] over #[allow] for lint management
Use conditional compilation (#[cfg]/#[cfg_attr]) for functions unused under specific features
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than backslash newline escapes
Prefer single-line function forms where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Use an opaque error (eyre::Report) only at the app boundary for logging; not in public APIs
Never export eyre::Report from libraries; convert to domain enums at API boundaries and to eyre only in main() or top-level async task
Files:
src/providers/onnx/mod.rssrc/providers/onnx/errors.rssrc/providers/onnx/aggregation.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
src/providers/onnx/mod.rssrc/providers/onnx/errors.rssrc/providers/onnx/aggregation.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.rs: Function documentation must include clear examples; test docs should omit redundant examples
Every module must begin with a module-level //! comment explaining purpose and utility
Document public APIs using Rustdoc (///) so cargo doc can generate docs
Files:
src/providers/onnx/mod.rssrc/providers/onnx/errors.rssrc/providers/onnx/aggregation.rs
docs/**/*.{rs,md}
📄 CodeRabbit inference engine (docs/rust-doctest-dry-guide.md)
docs/**/*.{rs,md}: In code fences, explicitly specify the rust language (```rust) for doctest code blocks
Use assert!, assert_eq!, and assert_ne! in doctests to verify outcomes
Avoid using unwrap()/expect() in examples; handle errors idiomatically
For fallible examples, write an explicit fn main() -> Result<...> and hide the boilerplate lines with #
Optionally use the (()) shorthand at the end of a code block to make main return Result; ensure no whitespace within (())
Use hidden lines (#) to hide setup, use statements, and boilerplate in doctest examples
Prefer no_run for examples with undesirable side effects (network, FS, GUI) so they compile but don’t execute
Use should_panic for examples intended to panic; optionally assert specific messages
Use compile_fail for examples that should not compile, but be aware of its brittleness across compiler versions
Avoid ignore for Rust examples; reserve it for pseudocode or non-Rust blocks
Use edition20xx doctest attribute when demonstrating edition-specific syntax or behavior
Files:
docs/lag-complexity-function-design.md
docs/**/*.md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
docs/**/*.md: Use British English (OED) spelling in documentation
The word “outwith” is acceptable in documentation
Keep US spelling when it appears in API names (e.g.,color)
Use the Oxford comma in lists
Treat company names as collective nouns (e.g., “Lille Industries are …”)
Write headings in sentence case
Use Markdown headings (#,##,###, …) in order without skipping levels
Follow markdownlint recommendations
Use standard Markdown syntax for code blocks and lists
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text
Use-for first-level bullets and renumber ordered lists when items change
Prefer inline links[text](url)or angle-bracket autolinks<url>
Ensure blank lines before and after bulleted lists and fenced code blocks
Ensure tables have a delimiter line below the header row
Expand uncommon acronyms on first use (e.g., Continuous Integration (CI))
Wrap paragraphs at 80 columns
Wrap code (in docs) at 120 columns
Do not wrap tables in documentation
Use footnotes referenced with[^label]
Include Mermaid diagrams where they add clarity
Embed images withand provide brief, descriptive alt text
Add a short descriptive sentence before each Mermaid diagram for screen readers
docs/**/*.md: Use docs/ markdown as reference and keep it updated with decisions and changes
Documentation must use en-GB-oxendict spelling and grammar (LICENSE naming unchanged)
Files:
docs/lag-complexity-function-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown with make markdownlint
Run make fmt to format Markdown files and fix table markup after documentation changes
Validate Mermaid diagrams in Markdown via make nixie
Wrap Markdown paragraphs and bullet points at 80 columns
Wrap code blocks at 120 columns
Do not wrap tables and headings
Use dashes (-) for list bullets
Use GitHub-flavoured Markdown footnotes ([^1])
Files:
docs/lag-complexity-function-design.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/lag-complexity-function-design.md
🧬 Code graph analysis (1)
src/providers/onnx/mod.rs (2)
src/providers/onnx/classifier.rs (1)
new(30-91)src/providers/mod.rs (1)
process(32-32)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (10)
src/providers/onnx/aggregation.rs (2)
41-55: LGTM: threshold and bin count validation are correct.Inclusive bounds on [0, 1] and
head_count + 1mapping length are enforced.
1-1: Add a module-level doc comment.Document the module’s purpose and primary types per repository guidelines.
Apply this diff:
+//! Ordinal head aggregation strategies and validation. use super::errors::OnnxClassifierError;src/providers/onnx/mod.rs (1)
13-14: Keep the internal classifier unexported.Do not re-export
OnnxTextClassifier. Current state complies.docs/lag-complexity-function-design.md (2)
1001-1013: Use sentence case for headings.Lowercase the second words to follow style.
Apply this diff:
-#### Provider Latency +#### Provider latency @@ -#### Computational Overhead +#### Computational overhead
482-496: Switch to en‑GB spelling in the tokenisation paragraph and roadmap.Standardise “Tokenisation/Tokeniser/quantisation/optimisation”.
Apply this diff:
-- **Tokenization & determinism:** Text is tokenized in Rust using a pinned +- **Tokenisation & determinism:** Text is tokenised in Rust using a pinned vocabulary to guarantee platform parity. The ONNX graph omits - tokenization-specific nodes, pins opset 17, and includes optional affine + tokenisation-specific nodes, pins opset 17, and includes optional affine calibration so that outputs remain deterministic across environments. - **Runtime & artefact controls:** The crate exposes an optional `onnx` feature that pulls in a pinned `ort = "2.0"` dependency (bundling ONNX Runtime 1.18). Provider start-up recomputes SHA-256 digests for `depth_transformer_ordinal.onnx`, the INT8 variant, the MLP fallback, and the - tokenizer artefacts before constructing inference sessions. Any mismatch + tokeniser artefacts before constructing inference sessions. Any mismatch raises an error and the provider fails closed, preventing partially loaded state. -- **Performance roadmap:** Post-training static INT8 quantization and - intermediate-layer pooling ablations are part of the committed optimization +- **Performance roadmap:** Post-training static INT8 quantisation and + intermediate-layer pooling ablations are part of the committed optimisation plan. These keep CPU latency below 10 ms p95 while shrinking artefact size without sacrificing calibration.src/providers/onnx/errors.rs (5)
60-63: LGTM: dedicated threshold-bounds error is present.
InvalidDecisionThresholdcleanly communicates configuration faults.
66-80: LGTM: wrapper errors are non‑exhaustive and transparent.Public wrappers map inner errors correctly and are semver‑safe.
5-7: Mark the public error enum as non‑exhaustive.Future‑proof the API against adding variants.
Apply this diff:
/// Errors produced by the shared ONNX classifier implementation. -#[derive(Debug, Error)] +#[derive(Debug, Error)] +#[non_exhaustive] pub enum OnnxClassifierError {
28-31: Include model path context in CreateSession errors.Carry the model path for clearer diagnostics.
Apply this diff:
- #[error("failed to construct ONNX session builder: {0}")] + #[error("failed to construct ONNX session builder: {0}")] CreateSessionBuilder(#[source] ort::Error), - #[error("failed to create ONNX session: {0}")] - CreateSession(#[source] ort::Error), + #[error("failed to create ONNX session for {path}: {source}")] + CreateSession { + path: PathBuf, + #[source] + source: ort::Error, + },
50-51: Include model path context in Inference errors.Improve traceability of inference failures.
Apply this diff:
- #[error("failed to run inference: {0}")] - Inference(#[source] ort::Error), + #[error("failed to run inference on {path}: {source}")] + Inference { + path: PathBuf, + #[source] + source: ort::Error, + },
| ExpectedValue { | ||
| /// Additive offset applied after summing the probabilities. | ||
| base: f32, | ||
| /// Multiplicative factor applied to the summed probabilities. | ||
| scale: f32, | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify ExpectedValue calibration constraints.
State whether negative scale is supported (flips orientation) and whether base/scale must be finite; optionally validate them.
| impl OrdinalAggregation { | ||
| /// Validates the ordinal aggregation configuration. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns `InvalidBinCount` when the mid-bin mapping length does not match `head_count + 1`. | ||
| /// Returns `InvalidDecisionThreshold` when the decision threshold falls outside `[0.0, 1.0]`. | ||
| pub fn validate(&self, head_count: usize) -> Result<(), OnnxClassifierError> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add Rustdoc examples to validate().
Provide a minimal example that compiles under docs to meet the “examples required” guideline.
Apply this diff:
/// Validates the ordinal aggregation configuration.
///
/// # Errors
///
/// Returns `InvalidBinCount` when the mid-bin mapping length does not match `head_count + 1`.
/// Returns `InvalidDecisionThreshold` when the decision threshold falls outside `[0.0, 1.0]`.
+ ///
+ /// # Examples
+ /// ```rust
+ /// # #[cfg(feature = "onnx")]
+ /// # {
+ /// use lag_complexity::providers::onnx::OrdinalAggregation;
+ ///
+ /// let ok = OrdinalAggregation::MidBin {
+ /// bin_midpoints: vec![0.0, 1.0],
+ /// decision_threshold: 0.5,
+ /// };
+ /// assert!(ok.validate(1).is_ok());
+ ///
+ /// let bad = OrdinalAggregation::MidBin {
+ /// bin_midpoints: vec![0.0, 1.0, 2.0],
+ /// decision_threshold: -0.1,
+ /// };
+ /// assert!(bad.validate(2).is_err());
+ /// # }
+ /// ```
pub fn validate(&self, head_count: usize) -> Result<(), OnnxClassifierError> {🤖 Prompt for AI Agents
In src/providers/onnx/aggregation.rs around lines 26 to 33, add a Rustdoc
example for validate() that compiles under docs by inserting the provided
example block: wrap the example in #[cfg(feature = "onnx")] and a block, show
construction of a valid OrdinalAggregation::MidBin and assert
validate(head_count) is Ok, then construct an invalid MidBin and assert
validate(...) is Err; place the example between the doc comment start and the
function signature so it appears in the /// doc comment immediately above pub fn
validate(&self, head_count: usize) -> Result<(), OnnxClassifierError>.
| use std::path::PathBuf; | ||
|
|
||
| use thiserror::Error; | ||
|
|
||
| /// Errors produced by the shared ONNX classifier implementation. | ||
| #[derive(Debug, Error)] | ||
| pub enum OnnxClassifierError { | ||
| #[error("failed to read artefact at {path}: {source}")] | ||
| Io { | ||
| path: PathBuf, | ||
| #[source] | ||
| source: std::io::Error, | ||
| }, | ||
| #[error("artefact at {path} expected SHA-256 {expected} but found {actual}")] | ||
| ChecksumMismatch { | ||
| path: PathBuf, | ||
| expected: String, | ||
| actual: String, | ||
| }, | ||
| #[error("failed to load tokenizer from {path}: {source}")] | ||
| LoadTokenizer { | ||
| path: PathBuf, | ||
| #[source] | ||
| source: tokenizers::Error, | ||
| }, | ||
| #[error("failed to configure tokenizer truncation: {0}")] | ||
| ConfigureTruncation(#[source] tokenizers::Error), | ||
| #[error("failed to construct ONNX session builder: {0}")] | ||
| CreateSessionBuilder(#[source] ort::Error), | ||
| #[error("failed to create ONNX session: {0}")] | ||
| CreateSession(#[source] ort::Error), | ||
| #[error("classifier requires at least one input name")] | ||
| MissingInputNames, | ||
| #[error("classifier requires at least {expected} input names but found {actual}")] | ||
| InsufficientInputNames { expected: usize, actual: usize }, | ||
| #[error("classifier requires at least one output name")] | ||
| MissingOutputNames, | ||
| #[error("classifier requires max_sequence_length > 0")] | ||
| ZeroSequenceLength, | ||
| #[error("classifier requires ordinal_head_count > 0")] | ||
| ZeroHeadCount, | ||
| #[error("tokenizer produced sequence of length {actual} but expected {expected}")] | ||
| SequenceLength { expected: usize, actual: usize }, | ||
| #[error("failed to encode text: {0}")] | ||
| Encode(#[source] tokenizers::Error), | ||
| #[error("failed to convert encoded text into tensor: {0}")] | ||
| EncodeTensor(#[source] ort::Error), | ||
| #[error("session mutex was poisoned by a previous panic")] | ||
| SessionPoisoned, | ||
| #[error("failed to run inference: {0}")] | ||
| Inference(#[source] ort::Error), | ||
| #[error("ONNX output \"{name}\" missing from session results")] | ||
| OutputMissing { name: String }, | ||
| #[error("ONNX output \"{name}\" has {actual} logits but expected {expected}")] | ||
| UnexpectedLogitCount { | ||
| name: String, | ||
| expected: usize, | ||
| actual: usize, | ||
| }, | ||
| #[error("mid-bin mapping requires {expected} bins but found {actual}")] | ||
| InvalidBinCount { expected: usize, actual: usize }, | ||
| #[error("decision threshold must be within [{min}, {max}] but was {actual}")] | ||
| InvalidDecisionThreshold { min: f32, max: f32, actual: f32 }, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Update call sites for new error payloads.
Pass the model path when constructing CreateSession/Inference.
Apply this in src/providers/onnx/classifier.rs:
// builder creation unchanged
let mut builder = Session::builder()
.map_err(OnnxClassifierError::CreateSessionBuilder)?;
let model_path = config.model.path.clone();
let session = builder
.commit_from_file(&model_path)
.map_err(|source| OnnxClassifierError::CreateSession { path: model_path.clone(), source })?;Wrap inference errors similarly:
// when calling session.run(...) inside predict():
.map_err(|source| OnnxClassifierError::Inference { path: model_path.clone(), source })🏁 Script executed:
#!/bin/bash
rg -n 'enum OnnxClassifierError' -C3 src/providers/onnx/errors.rs
rg -n 'CreateSession' -C3 src/providers/onnx/errors.rs
rg -n 'Inference' -C3 src/providers/onnx/errors.rsLength of output: 1117
Run search on call sites in classifier.rs for session creation and inference errors:
rg -n 'Session::builder' -C5 src/providers/onnx/classifier.rs
rg -n 'commit_from_file' -C5 src/providers/onnx/classifier.rs
rg -n 'session.run' -C5 src/providers/onnx/classifier.rsExtend CreateSession and Inference error variants to include model path
- Change
CreateSession(#[source] ort::Error)insrc/providers/onnx/errors.rsto
CreateSession { path: PathBuf, source: ort::Error } - Change
Inference(#[source] ort::Error)to
Inference { path: PathBuf, source: ort::Error } - Update calls in
src/providers/onnx/classifier.rs:
let session = builder
.commit_from_file(&model_path)
.map_err(|source| OnnxClassifierError::CreateSession { path: model_path.clone(), source })?;
…
.map_err(|source| OnnxClassifierError::Inference { path: model_path.clone(), source })🤖 Prompt for AI Agents
In src/providers/onnx/errors.rs lines 1-64, the CreateSession and Inference enum
variants must be changed from tuple variants to structs carrying the model
PathBuf and the ort::Error; update the variants to CreateSession { path:
PathBuf, #[source] source: ort::Error } and Inference { path: PathBuf, #[source]
source: ort::Error } and update their #[error(...)] messages to include the path
(e.g. "failed to create ONNX session at {path}: {source}" and "failed to run
inference on {path}: {0}" or similar referencing the named source), then update
every call site in src/providers/onnx/classifier.rs that maps ort errors into
these variants to construct the struct form (map_err(|source|
OnnxClassifierError::CreateSession { path: model_path.clone(), source }) and
map_err(|source| OnnxClassifierError::Inference { path: model_path.clone(),
source })), ensuring model_path is cloned or otherwise moved appropriately and
imports/type annotations still resolve.
There was a problem hiding this comment.
Gates Failed
(1 file with Large Method)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| classifier.rs | 1 advisory rule | 10.00 → 9.61 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
| fn run_session( | ||
| &self, | ||
| encoded: &EncodedInput, | ||
| type_ids_name: Option<&str>, | ||
| ) -> Result<(String, Vec<f32>), OnnxClassifierError> { | ||
| let ids_tensor = TensorRef::from_array_view(( | ||
| [1usize, self.max_sequence_length], | ||
| encoded.ids.as_slice(), | ||
| )) | ||
| .map_err(OnnxClassifierError::EncodeTensor)?; | ||
| let attention_tensor = TensorRef::from_array_view(( | ||
| [1usize, self.max_sequence_length], | ||
| encoded.attention.as_slice(), | ||
| )) | ||
| .map_err(OnnxClassifierError::EncodeTensor)?; | ||
|
|
||
| let mut session = self | ||
| .session | ||
| .lock() | ||
| .map_err(|_| OnnxClassifierError::SessionPoisoned)?; | ||
|
|
||
| let input_ids_name = self | ||
| .input_names | ||
| .first() | ||
| .ok_or(OnnxClassifierError::MissingInputNames)? | ||
| .as_str(); | ||
| let attention_mask_name = self | ||
| .input_names | ||
| .get(1) | ||
| .ok_or(OnnxClassifierError::InsufficientInputNames { | ||
| expected: 2, | ||
| actual: self.input_names.len(), | ||
| })? | ||
| .as_str(); | ||
|
|
||
| let outputs = match type_ids_name { | ||
| Some(type_ids_name) => { | ||
| let type_ids_tensor = TensorRef::from_array_view(( | ||
| [1usize, self.max_sequence_length], | ||
| encoded.type_ids.as_slice(), | ||
| )) | ||
| .map_err(OnnxClassifierError::EncodeTensor)?; | ||
| session | ||
| .run(ort::inputs! { | ||
| input_ids_name => ids_tensor, | ||
| attention_mask_name => attention_tensor, | ||
| type_ids_name => type_ids_tensor, | ||
| }) | ||
| .map_err(OnnxClassifierError::Inference)? | ||
| } | ||
| None => session | ||
| .run(ort::inputs! { | ||
| input_ids_name => ids_tensor, | ||
| attention_mask_name => attention_tensor, | ||
| }) | ||
| .map_err(OnnxClassifierError::Inference)?, | ||
| }; | ||
|
|
||
| let output_name = self | ||
| .output_names | ||
| .first() | ||
| .ok_or(OnnxClassifierError::MissingOutputNames)? | ||
| .clone(); | ||
| let logits_value = | ||
| outputs | ||
| .get(&output_name) | ||
| .ok_or_else(|| OnnxClassifierError::OutputMissing { | ||
| name: output_name.clone(), | ||
| })?; | ||
| let (_, logits) = logits_value | ||
| .try_extract_tensor::<f32>() | ||
| .map_err(OnnxClassifierError::Inference)?; | ||
| Ok((output_name, logits.to_vec())) | ||
| } |
There was a problem hiding this comment.
❌ New issue: Large Method
OnnxTextClassifier.run_session has 70 lines, threshold = 70
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/lag-complexity-function-design.md (2)
453-463: Make the Rust example self‑contained to pass doctests.Avoid importing non-existent helpers in doctests under docs/. Define a hidden local helper so the snippet compiles and asserts the intended behaviour.
Apply this diff to the example block:
```rust - # fn main() -> Result<(), regex::Error> { + # fn main() -> Result<(), regex::Error> { use regex::Regex; - use lag_complexity::heuristics::text::substring_count_regex; + # // Hidden: local helper mirroring the public API semantics + # fn substring_count_regex(hay: &str, re: &Regex) -> usize { + # if re.as_str().is_empty() { 0 } else { re.find_iter(hay).count() } + # } let empty = Regex::new("")?; assert_eq!(substring_count_regex("hay", &empty), 0); # Ok(()) # }As per coding guidelines --- `1014-1019`: **Use sentence case in the heading.** Align with heading style. Apply this diff: ```diff -#### Computational Overhead +#### Computational overheadAs per coding guidelines
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
docs/lag-complexity-function-design.md(22 hunks)src/providers/onnx/classifier.rs(1 hunks)src/providers/onnx/config.rs(1 hunks)src/providers/onnx/mod.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use functions and composition; extract reusable logic; prefer generators/comprehensions and declarative code when readable
Small, single-responsibility functions; obey command/query segregation
Name things precisely; boolean names prefixed with is/has/should
Use consistent en-GB-oxendict spelling/grammar in comments (except external API names)
Keep file size manageable: no code file >400 lines; break up large switches/dispatch; move large test data to external files
Fix warnings emitted during tests in code rather than silencing them
Extract helper functions when functions are too long; adhere to separation of concerns and CQRS
Group many parameters into well-named structs
For large error returns, consider using Arc to reduce returned data
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless absolutely necessary and document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditional criteria with more than two branches
Do not silence lints except as last resort
Lint suppressions must be tightly scoped and include a clear reason
Prefer #[expect] over #[allow] for lint management
Use conditional compilation (#[cfg]/#[cfg_attr]) for functions unused under specific features
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than backslash newline escapes
Prefer single-line function forms where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Use an opaque error (eyre::Report) only at the app boundary for logging; not in public APIs
Never export eyre::Report from libraries; convert to domain enums at API boundaries and to eyre only in main() or top-level async task
Files:
src/providers/onnx/config.rssrc/providers/onnx/classifier.rssrc/providers/onnx/mod.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
src/providers/onnx/config.rssrc/providers/onnx/classifier.rssrc/providers/onnx/mod.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.rs: Function documentation must include clear examples; test docs should omit redundant examples
Every module must begin with a module-level //! comment explaining purpose and utility
Document public APIs using Rustdoc (///) so cargo doc can generate docs
Files:
src/providers/onnx/config.rssrc/providers/onnx/classifier.rssrc/providers/onnx/mod.rs
docs/**/*.{rs,md}
📄 CodeRabbit inference engine (docs/rust-doctest-dry-guide.md)
docs/**/*.{rs,md}: In code fences, explicitly specify the rust language (```rust) for doctest code blocks
Use assert!, assert_eq!, and assert_ne! in doctests to verify outcomes
Avoid using unwrap()/expect() in examples; handle errors idiomatically
For fallible examples, write an explicit fn main() -> Result<...> and hide the boilerplate lines with #
Optionally use the (()) shorthand at the end of a code block to make main return Result; ensure no whitespace within (())
Use hidden lines (#) to hide setup, use statements, and boilerplate in doctest examples
Prefer no_run for examples with undesirable side effects (network, FS, GUI) so they compile but don’t execute
Use should_panic for examples intended to panic; optionally assert specific messages
Use compile_fail for examples that should not compile, but be aware of its brittleness across compiler versions
Avoid ignore for Rust examples; reserve it for pseudocode or non-Rust blocks
Use edition20xx doctest attribute when demonstrating edition-specific syntax or behavior
Files:
docs/lag-complexity-function-design.md
docs/**/*.md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
docs/**/*.md: Use British English (OED) spelling in documentation
The word “outwith” is acceptable in documentation
Keep US spelling when it appears in API names (e.g.,color)
Use the Oxford comma in lists
Treat company names as collective nouns (e.g., “Lille Industries are …”)
Write headings in sentence case
Use Markdown headings (#,##,###, …) in order without skipping levels
Follow markdownlint recommendations
Use standard Markdown syntax for code blocks and lists
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text
Use-for first-level bullets and renumber ordered lists when items change
Prefer inline links[text](url)or angle-bracket autolinks<url>
Ensure blank lines before and after bulleted lists and fenced code blocks
Ensure tables have a delimiter line below the header row
Expand uncommon acronyms on first use (e.g., Continuous Integration (CI))
Wrap paragraphs at 80 columns
Wrap code (in docs) at 120 columns
Do not wrap tables in documentation
Use footnotes referenced with[^label]
Include Mermaid diagrams where they add clarity
Embed images withand provide brief, descriptive alt text
Add a short descriptive sentence before each Mermaid diagram for screen readers
docs/**/*.md: Use docs/ markdown as reference and keep it updated with decisions and changes
Documentation must use en-GB-oxendict spelling and grammar (LICENSE naming unchanged)
Files:
docs/lag-complexity-function-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown with make markdownlint
Run make fmt to format Markdown files and fix table markup after documentation changes
Validate Mermaid diagrams in Markdown via make nixie
Wrap Markdown paragraphs and bullet points at 80 columns
Wrap code blocks at 120 columns
Do not wrap tables and headings
Use dashes (-) for list bullets
Use GitHub-flavoured Markdown footnotes ([^1])
Files:
docs/lag-complexity-function-design.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/lag-complexity-function-design.md
🧬 Code graph analysis (2)
src/providers/onnx/classifier.rs (1)
src/providers/onnx/mod.rs (2)
new(59-63)new(120-124)
src/providers/onnx/mod.rs (2)
src/providers/onnx/classifier.rs (1)
new(68-129)src/providers/mod.rs (1)
process(32-32)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (19)
docs/lag-complexity-function-design.md (6)
89-91: Good clarification on modularity benefits.Keep this strengthened rationale; it aligns the doc with the architecture goals.
428-446: Solid heading and content for the depth heuristic.The sentence-case heading and crisp bullets read well and follow the guidelines.
719-724: Correct trait API reference.The updated wording now matches the
process()trait method and parallelisation plan.
736-741: Accept “specialized” under en‑GB‑oxendict.Retain the -ize form; it conforms to the project’s en‑GB-oxendict guidance.
1001-1013: Good: provider latency section is clear and scoped.Leave this as is; it matches the benchmarking protocol.
1341-1347: LGTM: sentence‑case acceptance criteria.Keep the normalised heading; it matches the style rules.
src/providers/onnx/classifier.rs (5)
222-263: Validate logits length againsthead_countand keepOrdinalAggregationmapping.Aggregation logic and validation look correct.
104-111: Do not hard‑codepad_type_id; make it configurable.Hard‑coding
pad_type_id: 0is brittle across tokenisers and pair sequences.Apply these diffs:
tokenizer.with_padding(Some(PaddingParams { strategy: PaddingStrategy::Fixed(config.max_sequence_length), direction: PaddingDirection::Right, pad_to_multiple_of: None, pad_id: config.pad_id, - pad_type_id: 0, + pad_type_id: config.pad_type_id, pad_token: config.pad_token.clone(), }));And extend the config (see config.rs comment for the added field).
1-11: Add module‑level docs explaining inputs/outputs and invariants.Document the module purpose, required input names (2 or 3), output shape, max sequence length, and head count.
Apply this diff at the top:
+//! ONNX-backed ordinal text classifier. +//! +//! Loads a tokeniser and ONNX `Session`, enforces fixed `max_sequence_length`, +//! and aggregates ordinal logits (`head_count` thresholds) to a scalar depth/ambiguity score. +//! Expects 2 or 3 inputs in order: `input_ids`, `attention_mask`, and optional `token_type_ids`.As per coding guidelines
131-145: Document theencode(..., true)requirement.State explicitly that special tokens must be added for BERT‑like models.
Apply this diff:
/// Runs inference for a single input string and returns the aggregated ordinal score. /// /// # Errors /// /// Returns encoding, inference, or aggregation errors when tokenisation fails, the ONNX runtime errors, or the logits do not match the configured head count. + /// + /// Note: `tokenizer.encode(input, /* add_special_tokens = */ true)` is required for + /// BERT‑class models to ensure correct `[CLS]/[SEP]` placement and attention masks. pub fn predict(&self, input: &str) -> Result<f32, OnnxClassifierError> {As per coding guidelines
168-203: Validate input count and feedtoken_type_idsonly when expected.Guard against unexpected input arity and avoid silently ignoring extra inputs.
Apply this diff:
- let input_ids_name = self - .input_names - .first() - .ok_or(OnnxClassifierError::MissingInputNames)? - .as_str(); - let attention_mask_name = self - .input_names - .get(1) - .ok_or(OnnxClassifierError::InsufficientInputNames { - expected: 2, - actual: self.input_names.len(), - })? - .as_str(); - - let outputs = match type_ids_name { - Some(type_ids_name) => { + let outputs = match self.input_names.as_slice() { + [ids_name, attention_name] => { + session + .run(ort::inputs! { + ids_name.as_str() => ids_tensor, + attention_name.as_str() => attention_tensor, + }) + .map_err(OnnxClassifierError::Inference)? + } + [ids_name, attention_name, type_ids_name] => { let type_ids_tensor = TensorRef::from_array_view(( [1usize, self.max_sequence_length], encoded.type_ids.as_slice(), )) .map_err(OnnxClassifierError::EncodeTensor)?; session .run(ort::inputs! { - input_ids_name => ids_tensor, - attention_mask_name => attention_tensor, - type_ids_name => type_ids_tensor, + ids_name.as_str() => ids_tensor, + attention_name.as_str() => attention_tensor, + type_ids_name.as_str() => type_ids_tensor, }) .map_err(OnnxClassifierError::Inference)? } - None => session - .run(ort::inputs! { - input_ids_name => ids_tensor, - attention_mask_name => attention_tensor, - }) - .map_err(OnnxClassifierError::Inference)?, + names => { + return Err(OnnxClassifierError::InsufficientInputNames { + expected: 2, + actual: names.len(), + }); + } };src/providers/onnx/config.rs (1)
1-1: Good: add module‑level docs.Keep the concise purpose statement.
src/providers/onnx/mod.rs (7)
1-12: Good module surface and re‑exports.The module‑level docs, feature surface, and public re‑exports are coherent.
16-23: LGTM: wrapper type and API.Keep the façade pattern and hidden inner classifier.
31-58: Useful no_run example.The example demonstrates expected config shape and usage without side effects.
66-75: Correct trait implementation and error mapping.Retain this delegation; it cleanly maps classifier errors.
77-86: LGTM: ambiguity wrapper mirrors depth wrapper.Maintain symmetry across providers.
92-119: Useful no_run example.Keep this; it rounds out the docs for both providers.
127-136: Correct trait implementation and error mapping.No changes required.
| - `DepthClassifierOnnx`: Enabled by the `onnx` feature, this provider is the | ||
| default production path for depth estimation. It loads a compact | ||
| Transformer-Ordinal model exported to ONNX and executed on CPU via ONNX | ||
| Runtime. The architecture choice, performance targets, and operational | ||
| controls are captured in the dedicated | ||
| [`DepthClassifierOnnx` ADR](adr-depth-classifier-onnx.md). | ||
| - **Architecture:** A DistilBERT-class encoder feeds an ordinal regression head | ||
| that predicts `P(depth > τ_k)` for `k` ordered thresholds. The provider | ||
| computes the expected step count (or, optionally, a mid-bin mapping) from the | ||
| ordinal logits and returns this scalar as the raw depth signal for Sigma. | ||
| - **Tokenization & determinism:** Text is tokenized in Rust using a pinned | ||
| vocabulary to guarantee platform parity. The ONNX graph omits | ||
| tokenization-specific nodes, pins opset 17, and includes optional affine | ||
| calibration so that outputs remain deterministic across environments. | ||
| - **Runtime & artefact controls:** The crate exposes an optional `onnx` | ||
| feature that pulls in a pinned `ort = "2.0"` dependency (bundling ONNX | ||
| Runtime 1.18). Provider start-up recomputes SHA-256 digests for | ||
| `depth_transformer_ordinal.onnx`, the INT8 variant, the MLP fallback, and the | ||
| tokenizer artefacts before constructing inference sessions. Any mismatch | ||
| raises an error and the provider fails closed, preventing partially loaded | ||
| state. | ||
| - **Performance roadmap:** Post-training static INT8 quantization and | ||
| intermediate-layer pooling ablations are part of the committed optimization | ||
| plan. These keep CPU latency below 10 ms p95 while shrinking artefact size | ||
| without sacrificing calibration. | ||
| - `DepthClassifierMlpOnnx`: A feature-flagged fallback that consumes the | ||
| engineered feature vector also used by `DepthHeuristic`. It serves | ||
| tokenization-constrained environments, exports as a compact ONNX MLP with | ||
| `log1p`/`expm1` scaling, and is quantised for low footprint. | ||
| - `DepthFromLLM`: Enabled by the `provider-api` feature, this is the |
There was a problem hiding this comment.
Switch US spellings to en‑GB and keep terminology consistent.
Enforce en‑GB-oxendict across these sections: tokenisation/tokeniser, quantisation, and normalisation. Also fix “singularization” → “singularisation”.
Apply these diffs:
-**Tokenization & determinism:** Text is tokenized in Rust using a pinned
+**Tokenisation & determinism:** Text is tokenised in Rust using a pinned
vocabulary to guarantee platform parity. The ONNX graph omits
- tokenization-specific nodes, pins opset 17, and includes optional affine
+ tokenisation-specific nodes, pins opset 17, and includes optional affine
calibration so that outputs remain deterministic across environments.-**Performance roadmap:** Post-training static INT8 quantization and
+**Performance roadmap:** Post-training static INT8 quantisation and
intermediate-layer pooling ablations are part of the committed optimization- tokenizer artefacts before constructing inference sessions. Any mismatch
+ tokeniser artefacts before constructing inference sessions. Any mismatch-- `DepthClassifierMlpOnnx`: A feature-flagged fallback that consumes the
+- `DepthClassifierMlpOnnx`: A feature-flagged fallback that consumes the
engineered feature vector also used by `DepthHeuristic`. It serves
- tokenization-constrained environments, exports as a compact ONNX MLP with
+ tokenisation-constrained environments, exports as a compact ONNX MLP with-### Normalization (sigma)
+### Normalisation (sigma)- singularization (shared text utils) and uses `regex` word boundaries to avoid
+ singularisation (shared text utils) and uses `regex` word boundaries to avoidAs per coding guidelines
Also applies to: 525-550, 555-560, 601-611
🤖 Prompt for AI Agents
In docs/lag-complexity-function-design.md around lines 472-501 (and also apply
the same changes in lines 525-550, 555-560, 601-611), update US spellings to
en-GB using oxendict and make terminology consistent: replace
"tokenization"/"tokenizer" with "tokenisation"/"tokeniser",
"quantization"/"quantized" with "quantisation"/"quantised", "normalization" with
"normalisation", and "singularization" with "singularisation"; ensure hyphenated
phrases follow British spelling (e.g., "tokenisation-constrained"), keep
existing "artefact"/"artefacts" as-is, and sweep the covered line ranges for any
other instances of those terms to make uniform replacements.
| feature that pulls in a pinned `ort = "2.0"` dependency (bundling ONNX | ||
| Runtime 1.18). Provider start-up recomputes SHA-256 digests for | ||
| `depth_transformer_ordinal.onnx`, the INT8 variant, the MLP fallback, and the | ||
| tokenizer artefacts before constructing inference sessions. Any mismatch | ||
| raises an error and the provider fails closed, preventing partially loaded | ||
| state. |
There was a problem hiding this comment.
🧩 Analysis chain
Verify stated dependency versions for accuracy.
Confirm that ort = "2.0" maps to ONNX Runtime 1.18 as documented.
🌐 Web query:
Does the Rust crate `ort` version 2.0 bundle or require ONNX Runtime 1.18, and is that mapping stable/documented?
💡 Result:
Short answer: No — ort 2.0 does not require/bundle ONNX Runtime 1.18. The upstream mapping is documented and changed during the 2.0 prereleases; the official version-mapping page shows ort v2.0.0+ uses ONNX Runtime v1.22. [1][2]
Notes / sources:
- ort "Version mapping" (official docs) — shows ort v2.0.0+ → ONNX Runtime v1.22 and explains mappings may change on minor releases (pin if you need a specific ORT minor). [1]
- docs.rs listings / prerelease docs show some 2.0 RCs were built against older ORT (e.g. rc.4 referenced 1.18), which explains the prerelease wording you may have seen. Final 2.0+ mapping is what the docs page records. [2][3]
References:
[1] ort version-mapping (official docs).
[2] docs.rs ort 2.0.0-rc.4 (mentioned ONNX Runtime 1.18).
[3] docs.rs/ort (general crate docs and history).
Update ONNX Runtime version mapping
Correct docs/lag-complexity-function-design.md:487-492 to reflect that ort = "2.0" bundles ONNX Runtime v1.22 (not v1.18), or add a note that mappings may shift on minor releases.
🤖 Prompt for AI Agents
In docs/lag-complexity-function-design.md around lines 487 to 492, the text
incorrectly states that `ort = "2.0"` bundles ONNX Runtime 1.18; update the
sentence to state it bundles ONNX Runtime v1.22 (or alternatively append a brief
parenthetical noting that the crate-to-runtime version mapping can change across
minor releases), and ensure any downstream references or checksum/compatibility
notes reflect the corrected runtime version.
| /// Maximum token count accepted by the model. Inputs are padded and truncated to this size. | ||
| /// Must be greater than zero so every encoding produces fixed-length tensors. | ||
| pub max_sequence_length: usize, | ||
| /// Token inserted when padding shorter sequences. | ||
| pub pad_token: String, | ||
| /// Identifier of the padding token. | ||
| pub pad_id: u32, | ||
| /// Number of ordinal heads exposed by the model output. | ||
| /// Must be greater than zero so aggregation matches the logits tensor. | ||
| pub ordinal_head_count: usize, | ||
| /// Mapping from ordinal probabilities to the scalar score. | ||
| pub aggregator: OrdinalAggregation, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Expose pad_type_id in config to remove classifier hard‑coding.
Surface the padding type identifier so tokeniser configuration is fully controlled by artefacts/config.
Apply this diff:
pub struct OnnxClassifierConfig {
@@
/// Identifier of the padding token.
pub pad_id: u32,
+ /// Type identifier used for padding (e.g., segment/type id).
+ pub pad_type_id: u32,
@@
}Document defaults in your config loader and update classifier initialisation to use this field.
As per coding guidelines
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Maximum token count accepted by the model. Inputs are padded and truncated to this size. | |
| /// Must be greater than zero so every encoding produces fixed-length tensors. | |
| pub max_sequence_length: usize, | |
| /// Token inserted when padding shorter sequences. | |
| pub pad_token: String, | |
| /// Identifier of the padding token. | |
| pub pad_id: u32, | |
| /// Number of ordinal heads exposed by the model output. | |
| /// Must be greater than zero so aggregation matches the logits tensor. | |
| pub ordinal_head_count: usize, | |
| /// Mapping from ordinal probabilities to the scalar score. | |
| pub aggregator: OrdinalAggregation, | |
| } | |
| pub struct OnnxClassifierConfig { | |
| /// Maximum token count accepted by the model. Inputs are padded and truncated to this size. | |
| /// Must be greater than zero so every encoding produces fixed-length tensors. | |
| pub max_sequence_length: usize, | |
| /// Token inserted when padding shorter sequences. | |
| pub pad_token: String, | |
| /// Identifier of the padding token. | |
| pub pad_id: u32, | |
| /// Type identifier used for padding (e.g., segment/type id). | |
| pub pad_type_id: u32, | |
| /// Number of ordinal heads exposed by the model output. | |
| /// Must be greater than zero so aggregation matches the logits tensor. | |
| pub ordinal_head_count: usize, | |
| /// Mapping from ordinal probabilities to the scalar score. | |
| pub aggregator: OrdinalAggregation, | |
| } |
🤖 Prompt for AI Agents
In src/providers/onnx/config.rs around lines 16 to 28, add a new field pub
pad_type_id: u32 to the config struct (adjacent to pad_token and pad_id) so the
padding token type identifier is exposed; then update the config loader to set a
documented default for pad_type_id (matching existing pad_id default semantics)
and update any classifier initialisation sites to consume cfg.pad_type_id
instead of the hard‑coded value so tokeniser/classifier configuration is driven
entirely from the config artifact. Ensure the struct doc comments mention the
default and purpose of pad_type_id and remove the classifier hard‑coding where
it was previously used.
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68d3a41df410832295cd5f9065fa05d4
Summary by Sourcery
Document the default Transformer-Ordinal depth classifier architecture with an MLP fallback and refresh the complexity design documentation accordingly
New Features:
Enhancements:
Documentation: