feat: fit() accepts an optional leading model name - #23
Merged
Conversation
Mirror predict(model, ...) on the training side: fit('churn', f1, ..., fN,
label) registers the trained student as 'churn' and returns its id, so the
train and serve calls read in parallel with predict('churn', f1, ..., fN).
A leading argument is only a name when it is TEXT, and features are numeric,
so every existing fit(f1, ..., fN, label [, options]) call is unchanged. The
name may still be given as {"register":"churn"}; supplying it both ways is a
mistake, not a precedence to resolve, and raises PREDICT_ERR_OPTIONS. The
leading name, like the options object, must be constant within a group.
Adds adversarial tests (name-given-twice, name-with-no-features) and a parity
test that a leading-name model predicts identically to an options-register one.
Docs and CHANGELOG updated.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address review: the trailing-options detection used a raw argc >= 3, so a JSON-object trailing argument was consumed as options with a leading name but could land as a class label without one. Detect the leading name first and gate options on argc - has_name >= 3, so both forms treat the trailing object the same way. Adds a consistency test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address review: the fit reference claimed task is "inferred from the label", but fit defaults to classify and only regresses when task=regress is given explicitly (a real-valued label without it is trained as classification). State the real default. Also document that the leading name and the options object must be constant within an aggregate group. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthrough
ChangesNamed fit registration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SQLClient
participant fit_step
participant ModelRegistry
participant predict
SQLClient->>fit_step: call fit(name, features, label, options)
fit_step->>fit_step: validate name, options, arity, and aggregate consistency
fit_step->>ModelRegistry: register trained model under name
ModelRegistry-->>fit_step: return model ID
SQLClient->>predict: predict(model ID, scalar features)
predict-->>SQLClient: return prediction
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: 5
🤖 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-tabular.c`:
- Around line 858-866: Update the fit() documentation near the trailing options
description to state that a trailing JSON-object string is interpreted as
options only when argc - has_name is at least 3; otherwise, as in fit(tenure,
'{}'), it remains the classification label. Preserve the existing behavior and
clarify that object-valued labels are valid for the lower-arity form.
- Around line 965-972: Update the name handling in the visible model-name setup
block to obtain the TEXT byte length with sqlite3_value_bytes() and reject any
embedded NUL using memchr() after sqlite3_value_text(); set a
PREDICT_ERR_OPTIONS error formatted as "PREDICT_ERR_OPTIONS: ..." and return
before C-string consumers such as the registration/comparison paths. Add a
regression test that binds TEXT containing an embedded NUL and verifies the
operation returns that error rather than truncating the model name.
In `@README.md`:
- Line 162: Update the fit(...) API description in the README to explicitly
state that the leading optional argument is a TEXT model name, while register is
a key in the trailing JSON options object; replace the ambiguous “leading id (or
register)” wording and keep the native tabular student/blob behavior clear for
first-time readers.
In `@tests/test_fit.py`:
- Around line 38-48: Add a negative aggregate test near
test_fit_leading_name_rhymes_with_predict using a CASE expression over tenure as
the leading TEXT argument so different rows produce different model names.
Execute the query through the aggregate prediction path and assert the result is
PREDICT_ERR_OPTIONS, covering the group-consistency contract rather than a
constant-name happy path.
In `@website/src/content/docs/reference/functions.md`:
- Around line 70-72: Update the descriptions in
website/src/content/docs/reference/functions.md lines 70-72 and
website/src/content/docs/reference/options.md lines 51-53 so both consistently
identify label as the last training-data argument before optional options, while
keeping each document standalone for first-time readers.
🪄 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: cc714660-b43a-495c-b31b-ecaf7f3be81c
📒 Files selected for processing (7)
CHANGELOG.mdREADME.mdpredict-tabular.ctests/test_fit.pywebsite/src/content/docs/guides/operations.mdwebsite/src/content/docs/reference/functions.mdwebsite/src/content/docs/reference/options.md
- Reject a model name with an embedded NUL byte (PREDICT_ERR_OPTIONS): it would
otherwise be truncated by the mprintf/strcmp path and register a different id
than the caller passed. Adds a char(97,0,98) regression test.
- Add an adversarial test that a leading name varying within an aggregate group
(a CASE over a column) fails loud, exercising the group-constancy contract.
- Correct the header comment: a trailing '{...}' is options only once a feature
and a label precede it (argc - has_name >= 3); at the lowest arity it is the
label.
- Docs: state both registration forms (leading id argument or the register
option) explicitly, and describe the label as the last argument before the
optional options.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Makes the training side rhyme with the serving side.
fit('churn', f1, ..., fN, label)registers the trained student aschurnand returns its id, so it readsin parallel with
predict('churn', f1, ..., fN). A leading TEXT argument isunambiguously the name (features are numeric), so every existing
fit(f1, ..., fN, label [, options])call is unchanged. The name may still begiven as
'{"register":"churn"}'; supplying it both ways raisesPREDICT_ERR_OPTIONS. Like the options object, the leading name must be constantwithin an aggregate group.
Why
fitandpredictdid not read as a pair: the model name was buried in theoptions JSON for
fitbut a leading positional forpredict. This is the cheap,non-breaking ergonomic half. The robustness half (binding features by name so a
reordered
predictcannot silently score the wrong columns) is tracked in #21for v0.3.
Also in here
name (
argc - has_name >= 3), so a trailing JSON-object argument is treated thesame way with or without a name.
fit'staskis "inferredfrom the label", but it defaults to
classifyand only regresses on an explicittask:regress(a real-valued label with no task was silently classified). Thedocs now state the real default. Surfaced by review while touching these files.
Verification
make test: 215 passed, 35 skipped. New adversarial tests: leading-nameregister + serve, name-with-options, name-given-twice, name-with-no-features,
JSON-object-label consistency, and a parity test that a leading-name model
predicts identically to an options-register one.
main: no findings.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
fitnow accepts an optional leading model name and can register the trained model under that name.registeroptions remain supported.Bug Fixes
Documentation