feat: fit()/predict() + backtest() aggregate for an agent-guessable API - #20
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe SQL surface is reorganized around aggregate ChangesSQL API redesign
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (3)
website/src/content/docs/guides/backtesting.md (1)
6-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLine 8 promises
GROUP BYcomposes but no example shows the shape it requires.A first-time reader cannot guess that grouped backtesting needs the two-step join over the document column (
(SELECT grp, backtest(...) AS d FROM readings GROUP BY grp) g, backtest_rows(g.d) r), nor thatseries_keyfrombacktest_rowsis always empty and the group key comes from their ownGROUP BY. Add the grouped example.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@website/src/content/docs/guides/backtesting.md` around lines 6 - 21, Add a grouped backtesting SQL example near the existing backtest_rows examples, using the two-step pattern that selects the GROUP BY key and backtest document, then expands the document via backtest_rows. Explicitly show that the resulting group key comes from the outer grouped query and that backtest_rows.series_key is empty, rather than relying on series_key to identify groups.predict-forecast.c (1)
3489-3495: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the
#18issue references from these comments.Lines 3495 and 2681 both cite
#18. The public docs site is the contract, and a bare issue number in the amalgamation means nothing to a reader outside the tracker. State the rule, not the ticket.As per coding guidelines: "Flag any comment that references internal design documents (RFC section numbers): the public docs site is the contract."
Proposed fix
/* forecast/detect_anomalies/backtest are aggregates over the caller's rows: * one calling convention, GROUP BY splits series, each group returns a JSON - * document. backtest (`#18`) replaces the former backtest(query, ...) TVF. */ + * document. backtest replaces the former backtest(query, ...) TVF. */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@predict-forecast.c` around lines 3489 - 3495, Update the comments describing the backtest aggregate behavior near the backtest_rows module registration and the corresponding comment near the other `#18` reference to remove the internal “#18” issue citations. Preserve the documented behavior description while stating the applicable rule directly, without referencing tracker-specific identifiers.Source: Path instructions
tests/test_fit.py (1)
77-98: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd an adversarial malformed-BLOB test.
Mutate a valid
fit()blob's serialized dimensions and assert thatpredict(blob, ...)returnsPREDICT_ERR_SCHEMAwithout crashing. This directly covers the new untrusted deserialization entrypoint.As per path instructions, tests for new features must try malformed blobs and error paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_fit.py` around lines 77 - 98, Add an adversarial test alongside the existing prediction error tests that fits a valid model, mutates its serialized dimension metadata in the stored BLOB, and calls predict with that malformed blob. Assert it raises sqlite3.OperationalError containing PREDICT_ERR_SCHEMA and does not crash, covering the untrusted deserialization path.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@predict-distill.c`:
- Around line 892-904: Update the feature-name allocation loop in the tree
construction path so cleanup tracks partially copied names when sqlite3_mprintf
fails. Set tree.nfeat to the allocated feature count before copying, or
otherwise ensure predict0_tree_free() can release every successfully copied
tree.feat_names entry before jumping to done; preserve the existing SQLITE_NOMEM
return and cleanup flow.
In `@predict-forecast.c`:
- Around line 2686-2688: Update AGG_BACKTEST_OPTION_KEYS to include the existing
candidates option parsed by forecast_opt_cb, so backtest accepts the documented
candidates array and passes it through the auto-selection path without reporting
an unknown option. Keep the existing option keys unchanged.
In `@predict-student.c`:
- Around line 998-1012: The direct student-blob dispatch around the
is_mlp/is_forest checks must validate serialized dimensions against supported
caps and use overflow-checked allocation sizes before calling mlp_deserialize,
forest_deserialize, or tree_deserialize, returning PREDICT_ERR_SCHEMA for
malformed input without crashing. In tests/test_fit.py lines 77-98, mutate
dimensions in a valid blob and assert PREDICT_ERR_SCHEMA; no direct change is
required to other sites.
In `@predict-tabular.c`:
- Around line 1249-1260: Update the proba JSON construction in the prediction
output path to use the shared JSON encoder for pred, ensuring control characters
and other JSON-sensitive content are fully escaped; replace confidence
formatting in the hc branch with %.17g to preserve round-trippable
floating-point values.
- Around line 917-975: The fit_step aggregate currently parses options only
during initial configuration, allowing later rows to supply conflicting kind,
task, or register values silently. Update fit_step to detect and parse trailing
options on every row, compare them with the configured c->kind,
c->classify/task, and c->reg_id values, and set an error when any option
differs; preserve successful aggregation for identical options regardless of row
order.
- Around line 839-846: Remove internal RFC or issue references from the comments
at predict-tabular.c:839-846, predict-tabular.c:1272-1275,
predict-internal.h:199-204, and predict-distill.c:779-783. Keep the comments’
behavioral explanations and registration rationale, but rewrite “RFC `#19`” and
“#19 follow-up” references as standalone descriptions based on the public
contract.
- Around line 1279-1281: Update the sqlite3_create_function registration for
predict in predict-tabular.c to remove SQLITE_DETERMINISTIC, while preserving
SQLITE_UTF8 and the existing predict_scalar callback and registration arguments.
- Around line 862-879: Update fit_opt_cb so each sqlite3_mprintf option copy for
kind, student_kind, task, or register is checked for allocation failure; return
a PREDICT_ERR_* failure through errmsg instead of returning success with a NULL
option. Preserve duplicate-key replacement while ensuring allocation failures
cannot silently fall back to defaults or disable registration.
In `@skills/sqlite-predict/SKILL.md`:
- Around line 89-93: Update the predict documentation’s proba JSON example to
use valid JSON with explicit placeholder values for both prediction and
confidence, preserving the surrounding guidance and behavior description.
In `@tests/test_backtest.py`:
- Around line 135-139: Update the backtest query and assertion around
backtest_rows to select the per-group fold metrics rather than only g.grp, then
assert those metrics are populated for both groups. Ensure the test fails when
btr_filter synthesizes blank rows for empty $.rows while preserving validation
that groups a and b are present.
- Around line 142-148: Strengthen test_backtest_rejects_unknown_model by
asserting the raised sqlite3.OperationalError contains
PREDICT_ERR_MODEL_NOT_FOUND rather than accepting any operational error. Add
adversarial tests covering malformed or non-object backtest_rows input, a
non-array $.rows value, unknown option keys, and the arity-2 backtest('SELECT
...', 6) misuse; assert each failure uses its corresponding PREDICT_ERR_* code
and preserve the existing valid-path coverage.
In `@tests/test_fit.py`:
- Around line 94-98: Update test_fit_needs_features_and_a_label to assert the
specific PREDICT_ERR_* error code produced by SELECT fit(tenure) FROM h, rather
than accepting any sqlite3.OperationalError. Match the error-code assertion
style used by nearby tests and avoid asserting the prose message.
In `@tests/test_hardening.py`:
- Around line 21-25: Update tests/test_hardening.py lines 21-25 to retain only
the PREDICT_ERR_SCHEMA assertion and remove the “too many feature columns” prose
assertion. Update tests/test_distill.py lines 450-452 to replace match="no
train_query" with an assertion that the returned error contains the stable
PREDICT_ERR_SCHEMA code.
In `@website/src/content/docs/reference/functions.md`:
- Around line 68-75: The documented fit task behavior must match the
implementation: in website/src/content/docs/reference/functions.md lines 68-75,
replace the claim that task is inferred with the actual classify default. In
website/src/content/docs/reference/options.md lines 49-58, remove the
unsupported integer/text/REAL task-inference rules so both standalone
documentation sections describe only the supported public contract.
---
Nitpick comments:
In `@predict-forecast.c`:
- Around line 3489-3495: Update the comments describing the backtest aggregate
behavior near the backtest_rows module registration and the corresponding
comment near the other `#18` reference to remove the internal “#18” issue
citations. Preserve the documented behavior description while stating the
applicable rule directly, without referencing tracker-specific identifiers.
In `@tests/test_fit.py`:
- Around line 77-98: Add an adversarial test alongside the existing prediction
error tests that fits a valid model, mutates its serialized dimension metadata
in the stored BLOB, and calls predict with that malformed blob. Assert it raises
sqlite3.OperationalError containing PREDICT_ERR_SCHEMA and does not crash,
covering the untrusted deserialization path.
In `@website/src/content/docs/guides/backtesting.md`:
- Around line 6-21: Add a grouped backtesting SQL example near the existing
backtest_rows examples, using the two-step pattern that selects the GROUP BY key
and backtest document, then expands the document via backtest_rows. Explicitly
show that the resulting group key comes from the outer grouped query and that
backtest_rows.series_key is empty, rather than relying on series_key to identify
groups.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f16a3de-6aec-48a0-8eaa-3c04d88ba337
📒 Files selected for processing (29)
README.mdpredict-distill.cpredict-forecast.cpredict-internal.hpredict-student.cpredict-tabular.cskills/distill-lifecycle/SKILL.mdskills/interpret-backtest/SKILL.mdskills/prediction-receipts/scripts/receipt.pyskills/sqlite-predict/SKILL.mdtests/soak.ctests/soak_onnx.ctests/test_backtest.pytests/test_backtest_errors.pytests/test_distill.pytests/test_errors.pytests/test_fit.pytests/test_forecast_student.pytests/test_hardening.pytests/test_onnx.pytests/test_predict.pytests/test_skills.pywebsite/src/content/docs/guides/backtesting.mdwebsite/src/content/docs/guides/distillation.mdwebsite/src/content/docs/guides/operations.mdwebsite/src/content/docs/index.mdxwebsite/src/content/docs/reference/functions.mdwebsite/src/content/docs/reference/models.mdwebsite/src/content/docs/reference/options.md
💤 Files with no reviewable changes (2)
- tests/test_backtest_errors.py
- tests/test_errors.py
|
Addressed the review in
Local: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 11-16: Update the CHANGELOG entry describing fit() so it clearly
distinguishes the {"register":"..."} input option from the returned value:
supplying the option registers the trained model and makes fit() return its
registered model id, while omitting it makes fit() return a serialized blob.
Ensure the wording is standalone and understandable to first-time readers.
In `@predict-student.c`:
- Around line 360-365: Validate every float deserialized by predict-student.c
with isfinite(), including lr, init, tree thresholds/values/confidence, and MLP
weights, and route any non-finite value through the existing PREDICT_ERR_SCHEMA
validation paths. Cover all affected readers around the model-loading logic,
then add a tests/test_fit.py case that flips one serialized blob float to NaN
and asserts schema failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3de99267-f964-4eb0-a570-230089df137b
📒 Files selected for processing (12)
CHANGELOG.mdpredict-distill.cpredict-forecast.cpredict-internal.hpredict-student.cpredict-tabular.cskills/sqlite-predict/SKILL.mdtests/test_backtest.pytests/test_fit.pywebsite/src/content/docs/guides/distillation.mdwebsite/src/content/docs/reference/functions.mdwebsite/src/content/docs/reference/options.md
🚧 Files skipped from review as they are similar to previous changes (4)
- website/src/content/docs/guides/distillation.md
- website/src/content/docs/reference/options.md
- skills/sqlite-predict/SKILL.md
- website/src/content/docs/reference/functions.md
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_fit.py (1)
66-75: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the
probaresponse contract, not just required fields.Checking that
predictionandconfidenceexist would still pass if the implementation regressed to include the full class distribution. Assert that the distribution field is absent, or that the returned JSON has exactly the documented shape.As per PR objectives,
probareturnspredictionandconfidence, not the full class distribution.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_fit.py` around lines 66 - 75, Update test_predict_proba_returns_confidence to assert the exact documented response shape for proba predictions: require prediction and confidence while also asserting the full class distribution field is absent, or equivalently that the JSON keys are exactly those two fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_fit.py`:
- Around line 127-135: Replace the arbitrary byte scan in the malformed-float
test with the documented serialized float-field offset, leaving framing and
integrity fields untouched. Update the test to corrupt that field directly and
cover both NaN and infinity where practical, so the assertion specifically
verifies rd_f32() rejects non-finite values rather than an unrelated blob-format
failure.
---
Outside diff comments:
In `@tests/test_fit.py`:
- Around line 66-75: Update test_predict_proba_returns_confidence to assert the
exact documented response shape for proba predictions: require prediction and
confidence while also asserting the full class distribution field is absent, or
equivalently that the JSON keys are exactly those two fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2394648d-9535-494e-8236-fd01d3807a06
📒 Files selected for processing (3)
CHANGELOG.mdpredict-student.ctests/test_fit.py
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
|
@coderabbitai review The last review ( |
|
✅ Action performedReview finished.
|
|
@coderabbitai review Pushed |
|
✅ Action performedReview finished.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@predict-train.c`:
- Around line 262-281: Change bld_leaf from void to an integer status-returning
function, returning -1 when its cnt allocation fails and 0 after successfully
building the leaf. Update all three bld_leaf call sites in
bld_build—depth/purity limit, degenerate split, and no-improving-split—to check
the result and immediately return -1 on failure, preserving the existing OOM
error contract and eliminating the malformed fallback leaf.
In `@predict-train.h`:
- Around line 32-42: Rename the cross-translation-unit symbols bld_build,
train_gbt, train_mlp, intern_label, and register_student to use the predict0_
prefix. Update their declarations in the header, definitions in predict-train.c,
and all call sites in predict-distill.c, preserving signatures and behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3542814c-a58c-4e20-a703-f1ac347bc767
📒 Files selected for processing (7)
Makefilepredict-distill.cpredict-internal.hpredict-tabular.cpredict-train.cpredict-train.hscripts/amalgamate.py
🚧 Files skipped from review as they are similar to previous changes (1)
- predict-tabular.c
There was a problem hiding this comment.
🧹 Nitpick comments (1)
predict-train.c (1)
574-576: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDo the GBT score accumulation in
f64.
fo->lr * predict0_reg_tree_value(...)is an all-f32product that is only widened after rounding, which is what CodeQL flags at line 576. Over 200 rounds the accumulated rounding is avoidable.♻️ Proposed fix
for (int i = 0; i < n; i++) F[(size_t)i * nscore + s] += - fo->lr * predict0_reg_tree_value(b.nodes, b.n, &X[(size_t)i * nfeat]); + (f64)fo->lr * + (f64)predict0_reg_tree_value(b.nodes, b.n, &X[(size_t)i * nfeat]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@predict-train.c` around lines 574 - 576, Update the score accumulation loop using `predict0_reg_tree_value` so the learning-rate and tree-value multiplication is performed in `f64` before adding to `F`, rather than multiplying as `f32` and widening afterward. Preserve the existing indexing and accumulation behavior while explicitly widening the operands before multiplication.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@predict-train.c`:
- Around line 574-576: Update the score accumulation loop using
`predict0_reg_tree_value` so the learning-rate and tree-value multiplication is
performed in `f64` before adding to `F`, rather than multiplying as `f32` and
widening afterward. Preserve the existing indexing and accumulation behavior
while explicitly widening the operands before multiplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b5caeb8-1f7c-4cae-84e7-47d9a2ffe637
📒 Files selected for processing (7)
Makefilepredict-distill.cpredict-internal.hpredict-tabular.cpredict-train.cpredict-train.hscripts/amalgamate.py
🚧 Files skipped from review as they are similar to previous changes (1)
- predict-tabular.c
|
@coderabbitai review Pushed |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== commit summary ==='
git show --stat --oneline --decorate --no-renames 5f43962
printf '%s\n' '=== latest commit patch ==='
git show --format=fuller --find-renames --find-copies 5f43962 -- \
predict-train.c predict-train.h predict-distill.c predict-tabular.c \
Makefile scripts/amalgamate.py
printf '%s\n' '=== source outlines ==='
ast-grep outline predict-train.c --items all --type function --view expanded
ast-grep outline predict-train.h --items all --view expanded
ast-grep outline predict-distill.c --items all --type function --view expanded
ast-grep outline predict-tabular.c --items all --type function --view expanded
printf '%s\n' '=== trainer symbols: declarations, definitions, and calls ==='
rg -n -C 3 \
'\bpredict0_(bld_build|train_gbt|train_mlp|intern_label|register_student)\s*\(|(?<!predict0_)\b(bld_build|train_gbt|train_mlp|intern_label|register_student)\s*\(' \
-g '*.[ch]' .
printf '%s\n' '=== leaf builder, GBT update, and error mapping contexts ==='
rg -n -C 18 \
'\bbld_leaf\s*\(|\btrain_gbt\s*\(|\bSQLITE_NOMEM\b|\bPREDICT_ERR_RESOURCE\b|\(f64\)fo->lr|fo->lr' \
predict-train.c predict-train.h predict-distill.c predict-tabular.cLength of output: 21840 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== all renamed symbol references ==='
rg -n -C 2 \
'\b(predict0_bld_build|predict0_train_gbt|predict0_train_mlp|predict0_intern_label|predict0_register_student|bld_build|train_gbt|train_mlp|intern_label|register_student)\s*\(' \
-g '*.[ch]' .
printf '%s\n' '=== GBT implementation and allocation/count arithmetic ==='
sed -n '470,615p' predict-train.c
printf '%s\n' '=== tree builder implementation ==='
sed -n '240,475p' predict-train.c
printf '%s\n' '=== shared training entry and direct-tree error translation ==='
sed -n '675,850p' predict-train.c
printf '%s\n' '=== distillation tree and GBT error translation ==='
sed -n '450,670p' predict-distill.c
printf '%s\n' '=== fit aggregate error translation ==='
sed -n '900,1140p' predict-tabular.c
printf '%s\n' '=== training type definitions and relevant bounds/constants ==='
rg -n -C 4 \
'\b(typedef struct.*(Forest|TreeNode)|struct (Forest|TreeNode)|TREE_|GBT_|MAX_|nclass|nscore|n_rounds|n_trees)\b' \
predict-student.h predict-student.c predict-train.h predict-train.cLength of output: 69860 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== regression-tree value declaration and prediction use ==='
rg -n -C 8 '\bpredict0_reg_tree_value\s*\(' \
predict-student.c predict-student.h predict-train.c
printf '%s\n' '=== scalar model-load error path ==='
sed -n '1140,1305p' predict-tabular.c
rg -n -C 8 '\bpredict0_student_load\s*\(|\bpredict0_forest_deserialize\s*\(' \
predict-student.c predict-tabular.c
printf '%s\n' '=== class-count validation and serialization boundaries ==='
sed -n '100,220p' predict-student.c
sed -n '305,425p' predict-student.c
printf '%s\n' '=== fit tests and label-cardinality cases ==='
fd -t f '^test_.*\.py$' tests -x rg -n -C 4 \
'fit\s*\(|distinct label|nclass|2048|classif' {}Length of output: 28427
Reject ✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@predict-train.c`:
- Line 721: Reject class vocabularies exceeding 2,048 immediately after
vocabulary construction completes and before training or serialization. In
predict-train.c at the fit/label-interning flow around predict0_intern_label,
add the shared nclass validation; in predict-distill.c across the vocabulary
setup ending with teacher labels and soft-label nclass = nproba, apply the same
centralized check after all paths converge. Return an existing PREDICT_ERR_*
code with errors formatted as “CODE: detail,” preserving no silent fallback
paths, and add regression coverage for both entrypoints.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e79f5d46-94e4-41c1-8ed3-ee2bc6fff8b5
📒 Files selected for processing (4)
predict-distill.cpredict-student.cpredict-train.cpredict-train.h
🚧 Files skipped from review as they are similar to previous changes (1)
- predict-student.c
|
@coderabbitai review Pushed |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
predict-student.c (1)
396-397: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftCheck cumulative tree-pool sizes with overflow-safe arithmetic.
The per-header limits do not prevent
tree_off[j] + mandpool_cap * 2from overflowingint. After overflow,tree_off[j + 1]can become invalid and the subsequentf->nodes[...]writes may address outside the allocation. Track cumulative sizes with checkedsize_tarithmetic and reject totals that cannot fit the allocation or remaining blob before indexing.As per path instructions, all deserialization must be bounds-checked and must fail loudly without silent fallback paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@predict-student.c` around lines 396 - 397, Update the deserialization logic around the tree-count validation to track cumulative tree-pool sizes with checked size_t arithmetic. Validate each tree_off[j] plus its tree count and pool_cap multiplied by two against allocation limits and the remaining blob before computing offsets or indexing f->nodes. Reject any overflow or out-of-bounds total through the existing loud deserialization failure path, with no silent fallback.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@predict-distill.c`:
- Around line 225-232: Move the STUDENT_MAX_CLASS enforcement into the JSON
array parsing/counting path used before the allocations in predict-distill.c,
such as predict0_json_str_array(), so oversized proba/classes payloads are
rejected while parsing. Ensure the existing too-many-classes SQLITE_ERROR and
errmsg behavior remains consistent, and prevent proba_names or soft_labels from
growing beyond the cap.
In `@tests/test_fit.py`:
- Around line 150-162: Extend test_fit_rejects_too_many_classes with a companion
boundary test covering exactly 2,048 distinct labels, and assert that fit()
succeeds without a schema error. Keep the existing 2,049-class rejection test
unchanged so both sides of the STUDENT_MAX_CLASS limit are verified.
---
Outside diff comments:
In `@predict-student.c`:
- Around line 396-397: Update the deserialization logic around the tree-count
validation to track cumulative tree-pool sizes with checked size_t arithmetic.
Validate each tree_off[j] plus its tree count and pool_cap multiplied by two
against allocation limits and the remaining blob before computing offsets or
indexing f->nodes. Reject any overflow or out-of-bounds total through the
existing loud deserialization failure path, with no silent fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b699d829-b5c5-41fe-9305-648cf1374e21
📒 Files selected for processing (6)
predict-distill.cpredict-student.cpredict-student.hpredict-train.ctests/test_distill.pytests/test_fit.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_distill.py
Reshape the tabular and evaluation surface into the scikit-learn shape agents
guess, and extract and harden the native-student training core that backs it.
Surface:
- fit(f1, ..., fN, label [, options]) is an aggregate over the caller's rows and
returns a registered model id or a serialized blob; predict(model, f1, ..., fN
[, options]) is a scalar serving one prediction per row (deserialized once,
cached on the model argument). The label is the last positional argument; the
optional trailing '{...}' is always the options object.
- backtest(ts, value, horizon [, options]) is an aggregate (matching forecast /
detect_anomalies) that runs rolling-origin evaluation and returns one JSON
document; backtest_rows() expands it into typed rows. The old TVF is renamed
predict_batch.
- The tabular training core (tree / gradient-boosted / MLP fitters) moves to
predict-train.{c,h}, shared by fit() and the distill recipes.
Correctness and safety hardening (a thorough review pass, fixed at the root and
swept across every sibling, not only where first flagged):
- Memory safety: a compile-time assert that predict()'s feature buffer holds any
student the loader admits; re-fetch after sqlite3_set_auxdata to avoid a
use-after-free; propagate OOM through the row-cursor mprintf paths instead of
silent NULL columns; 64-bit sizing for model-scaled allocations; bounded,
overflow-safe onnx io_spec arrays (a 32-bit-wasm overflow); a fit() training-set
cap.
- Determinism: total-order split comparators so the tree, and its content_hash,
are identical across platforms/libc (no reads of uninitialized task-union
fields).
- Fail loud, never silently: reject non-finite features/labels/teacher cells and
NULL labels; surface malformed-JSON parse errors, prepare failures, and
mid-read step errors; preserve distinct error codes; cap the classifier
vocabulary at PREDICT0_MAX_CLASS during interning and parsing; require a
positive json-array cap rather than a silent-unbounded footgun.
- The receipt tool's operation taxonomy is restructured (aggregate vs row-shaped,
with the *_rows TVFs first-class and wrapper-aware inference).
Tests: adversarial coverage for the new surface and every hardening path
(malformed/oversized/boundary/non-finite inputs asserting concrete PREDICT_ERR_*
codes); the corrupt-blob tests now supply a matching content hash so they reach
the bounds-checked deserializer; the C soak drivers assert row counts and the
specific error code per branch, and propagate statement failures to the exit
code.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Document the new surface for a first-time reader: the functions reference (fit/predict/backtest, predict_batch's optional train_query and required apply_query, the JSON-object-label convention), the models and options pages, the sqlite-predict / distill-lifecycle / interpret-backtest skills, README, and the CHANGELOG. Scope the read-only guarantee to the serving functions (registration writes to _predict_models). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
cf115ba to
0786b26
Compare
2 participants
What
Splits the tabular prediction surface into
fit()(an aggregate that trains over your rows) andpredict()(a scalar that serves a model per row), the scikit-learn shape, and makesbacktest()an aggregate over the caller's rows likeforecast/detect_anomalies. One rule now governs the whole API:forecast,detect_anomalies,backtest,fit.predict.forecast_rows,anomaly_rows,backtest_rows.The old query-string TVF forms are removed (clean break; pre-1.0, no users). The old
predictTVF is retained aspredict_batchfor the batched, in-context (knn5), and ONNX serving path, that is a distinct capability, not a compat shim.Closes #18. Closes #19.
Why
A clean-room guessability test (fresh agents given only the function signatures, scored by executing their SQL against the live extension) scored the old
predict(train_query, apply_query)TVF at 0/6. Every agent, skilled or not, failed on atargetoption that was required but never inferred, and on the apply query's silent first-column-is-row-id convention.The redesign flips it:
fit/predictare also ORM-native (verified with SQLAlchemyfunc.fit/func.predictover mapped columns, composing insideWHERE), which the query-string TVF could never be.Changes
fit()aggregate +predict()scalar (predict-tabular.c);backtest()aggregate +backtest_rows()(predict-forecast.c); a sharedpredict0_train_studenttraining entry that bothfit()anddistill_predictcall; the oldpredictTVF renamed topredict_batch.README,reference/{functions,options,models},guides/{operations,backtesting,distillation},index; skillssqlite-predict,interpret-backtest,distill-lifecycle.Notes
predict('{"proba":true}')returns{"prediction","confidence"}(argmax + softmax) rather than the full per-class distribution. A follow-up.TODO(#19 follow-up)markers): splitting the training core into a standalonepredict-train.{c,h}. The boundaryfit()needs already exists viapredict0_train_student; the file split is coupled and cosmetic, so it is deliberately deferred rather than risk the green branch.distill_predict/distill_forecast,forecast, anddetect_anomaliesare unchanged.Checklist
test_fit.py(fit + scalarpredict) with corrupt-blob, NaN/Inf-float, wrong-arity, and unknown-model cases; hostile-document cases forbacktest_rows; the C soak drivers (soak.c/soak_onnx.c) re-pointed to the new surface.make testpasses (196 passed, 35 skipped).make test-asanis clean (soak ok; also verified under-fsanitize=function, which was the CI failure this PR fixes).git commit -s, DCO). Not added. DCO is not an enforced check on this repo; maintainer to sign off at merge if desired.predictis now a scalar;backtestis now an aggregate). The receipt/replay tests and the receipts skill were re-pointed to the new operations (predict_batch) deliberately.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
fit(...), with optional model registration.predict_batch(...)and row-wise scalar serving viapredict(model, f1, ..., fN).backtest(ts, value, horizon[, options]), producing JSON per group expanded viabacktest_rows(doc).Bug Fixes
Documentation
probaoutput.Tests
predict_batch(...)/predict(...)andbacktest_rows(...), including additional error-path assertions.