Add Nori tabular regression plugin - #115
Conversation
879c68a to
956cd9c
Compare
4360cbd to
2756726
Compare
2756726 to
c6578d2
Compare
0ed09f1 to
58684c5
Compare
Add a scheduled and HTTP Processing Engine plugin that predicts a numeric field in an InfluxDB 3 measurement from other columns with Synthefy's Nori in-context tabular regression model, via the Baseten gateway. It trains on the rows where the target field is present and predicts (imputes) the rows where it is null, writing the predictions back with write_sync. The Nori API key is read only from a request header or the NORI_API_KEY env var, never from trigger arguments or the request body. The model slug is configurable (synthefy/nori, synthefy/nori-30m). Ships with README, manifest.toml, requirements.txt, and a plugin_library.json entry.
58684c5 to
868438e
Compare
|
@Aliaksei-Kharlap thanks for the review, and for the detailed comments. One larger change first: we have refocused this plugin to be regression-only and renamed it ( How each comment is addressed:
CI is green. This is ready for another review, thank you. |
|
Hi, I'll check out the plugin soon, thanks! |
| p = f"tag{i}" | ||
| tag_clause += f" AND {_ident(k)} = ${p}" | ||
| params[p] = v | ||
| if cfg["start_time"] and cfg["end_time"]: |
There was a problem hiding this comment.
The docstring states that end_time is optional and start_time can be used independently, but here start_time is ignored unless end_time is specified
| if request_headers: | ||
| for k, v in request_headers.items(): | ||
| if k.lower() == "x-nori-api-key": # case-insensitive per server | ||
| return v.strip() |
There was a problem hiding this comment.
I think it's worth adding a check for an empty string here
There was a problem hiding this comment.
Had a few agents do reviews and manual validations, and they found the following issues that might need to be addressed.
I wouldn't consider any of these "required", so if you don't agree, that's fine, we can ignore.
1. Blocker: a caller can redirect the operator's API key
process_request merges every scalar body key over the trigger arguments, and gateway_url is one of those keys. A caller who holds only a database token therefore obtains the operator's NORI_API_KEY, the X_train and y_train data, and a request that starts inside the InfluxDB host and goes to any URL. The same merge also exposes target_database, model, and min_history.
Fix: define an explicit list of body-overridable keys and drop every other key. Keep gateway_url and target_database out of that list. Add a regression test.
2. Fix before merge
Each item below is small.
| Location | Finding |
|---|---|
:116 |
A tags token with no colon is discarded in silence, so the query reads every series |
:392 |
A gateway fault reports invalid JSON in request body, which blames the caller |
:241 |
Predictions are not validated: a list of nulls reports success, and a NaN fails at the storage layer |
:351 |
The same defect, seen from the caller side |
:86 |
An empty X-Nori-Api-Key value suppresses the environment fallback |
:155 |
The error text contradicts the documented format |
:23 |
http_args_config declares 5 parameters. _build_config accepts 13. |
plugin_library.json:318 |
REQUIRED_PLUGIN_METADATA.md:55 forbids a hand edit of this file |
3. The documents make three claims that the code does not meet
| Location | Claim |
|---|---|
:280 and README.md:307 |
"the plugin fails loud if a run resolves to more than one series". The guard tests the test set for duplicate timestamps only, so two series that hold different timestamps train as one. |
README.md:76 |
The PR description reports "0 errors". scripts/validate_readme.py also gives 7 warnings. |
| PR description | config_file_path is listed under "Configuration highlights". The code has no such support, and passing the value changes nothing. 28 of 33 plugins accept it. Delete the claim, or add the feature. |
4. Recommended, but not a merge gate
| Location | Finding |
|---|---|
:205 |
The query has no LIMIT. A 30-day window at 1 Hz is about 2.6 M rows in one POST. |
:237 |
The gateway call has no retry, but the local write retries three times. The README documents a cold-start 500. |
:179 |
start_time alone, and end_time alone, both fall back to the 30-day window in silence |
:163 |
A non-numeric feature column passes validation, and the user then sees a min_history message |
No test file exists. _interval_to_timedelta, _split_list, _build_config, _where_clause, and _get_api_key are pure and need no mock server. Four of the comments above live in them.
5. Other Findings
| Location | Finding |
|---|---|
:370 |
Each scheduled run sends a byte-identical payload to a metered endpoint. At every:15m with window=30d, that is about 2,880 sends per row. The fix needs a flag, so design it rather than rush it. |
:103 |
A field name that contains a space is unreachable from a scheduled trigger |
:397 |
The response returns str(e), which can hold the gateway URL and the database name. The pattern is repository-wide. |
:337 |
The write retry catches every exception, so a permanent fault costs about 5 seconds |
| # scalar body values override trigger args | ||
| merged = {**(args or {}), **{k: str(v) for k, v in body.items() if not isinstance(v, (list, dict))}} |
There was a problem hiding this comment.
Blocker: a caller can redirect the operator's API key to any host.
This line merges every scalar body key over the trigger arguments. _build_config:126 then reads gateway_url from the merged map, and _call_nori:232 posts to that URL with Authorization: Api-Key <NORI_API_KEY>.
A caller who holds only a database token therefore obtains three things:
- the operator's
NORI_API_KEY, a credential that the operator never shared; - the
X_trainandy_traindata; - a request that starts inside the InfluxDB host and goes to any URL.
The same merge also exposes target_database, model, min_history, window, output_measurement, and dry_run. A caller can write to another database, select a more expensive model slug, or set min_history to 0.
Do not patch one key. Define an explicit list of body-overridable keys and drop every other key. Keep gateway_url and target_database out of that list. Add a regression test, because a security fix with no test returns.
| "tags": tags, | ||
| "window": args.get("window", "30d"), | ||
| "model": args.get("model", DEFAULT_MODEL_SLUG), | ||
| "gateway_url": args.get("gateway_url", DEFAULT_GATEWAY_URL), |
There was a problem hiding this comment.
Blocker (see line 380). gateway_url comes from the arguments, and process_request:380 merges the request body over those arguments. A caller therefore chooses the host that receives the Authorization: Api-Key header at line 235.
Make the URL a module constant, as synthefy_forecasting.py:78 does with SYNTHEFY_API_BASE_URL. Apply the same rule to target_database at line 128.
| resp = requests.post( | ||
| cfg["gateway_url"], | ||
| json=payload, | ||
| headers={"Content-Type": "application/json", "Authorization": f"Api-Key {api_key}"}, | ||
| timeout=120, | ||
| ) |
There was a problem hiding this comment.
Blocker (see line 380). This call sends the operator's NORI_API_KEY to cfg["gateway_url"], and a caller controls that value. The X_train and y_train data go with it. Require HTTPS.
Second defect: no retry. This call fails after one attempt, but _write_predictions:330 retries three times with backoff. The README documents a cold-start 500 and a 60-70 second latency at lines 297-303. The retry effort therefore sits on the reliable side of the system. Retry a 5xx response, a 429 response, and a timeout. Make the 120-second timeout configurable.
| if args.get("tags"): | ||
| for pair in args["tags"].split(): # space-separated key:val pairs (a value may contain '.') | ||
| if ":" in pair: | ||
| k, v = pair.split(":", 1) | ||
| tags[k.strip()] = v.strip() |
There was a problem hiding this comment.
A token that holds no colon is discarded without a message. tags=site=A therefore gives an empty filter, and the query reads every series in the measurement.
The failure is silent, and it widens the query. Raise a ValueError for each invalid token.
| # surface the real outcome at the top level (success / skipped / dry_run), not always | ||
| # "success", so a caller can tell a real prediction from a no-op. | ||
| return {"status": result.get("status", "success"), "task_id": task_id, "result": result} | ||
| except json.JSONDecodeError: |
There was a problem hiding this comment.
requests.exceptions.JSONDecodeError is a subclass of json.JSONDecodeError (verified, requests 2.32.5). A failure of resp.json() at line 239 therefore lands in this handler.
A gateway that returns HTTP 200 with a non-JSON body gives the caller invalid JSON in request body, for a request whose body was valid JSON. The operator then debugs the wrong system.
Parse request_body in its own try/except block at line 378.
| raise ValueError("`measurement` and `field` are required") | ||
| _validate_columns(influxdb3_local, cfg) | ||
| out_times, preds = _regress(influxdb3_local, cfg, api_key, task_id) | ||
| if not preds: |
There was a problem hiding this comment.
A list of nulls is truthy, so this test passes, and the run reports success with written: 0. Validate the predictions at line 241 instead.
| def process_scheduled_call(influxdb3_local, call_time, args=None): | ||
| task_id = str(uuid.uuid4()) | ||
| try: | ||
| cfg = _build_config(args) | ||
| api_key = _get_api_key() # scheduled: no request headers -> env var only | ||
| _run(influxdb3_local, cfg, api_key, task_id) | ||
| except Exception as e: # noqa: BLE001 - never let a scheduled run crash the engine | ||
| influxdb3_local.error(f"[{task_id}] {type(e).__name__}: {e}") |
There was a problem hiding this comment.
Follow-up, not a merge gate. Each scheduled run reads the same window, and the predictions go to <measurement>_regressed, so the source rows stay null and nothing converges. The next run therefore sends a byte-identical payload to a metered endpoint. At every:15m with window=30d, the plugin sends each row about 2,880 times.
Add a skip_existing flag with a default of true: read output_measurement first and remove the timestamps that already hold a prediction.
Keep the flag rather than remove the behaviour, because later training data can improve an earlier prediction.
| (the CLI splits argument pairs on commas, so a comma inside a value breaks parsing); commas are | ||
| fine inside the HTTP JSON body. | ||
| """ | ||
| return [x for x in re.split(r"[,\s]+", str(text).strip()) if x] |
There was a problem hiding this comment.
Follow-up. Every supported string format splits on whitespace, so a field name that contains a space is unreachable from a scheduled trigger. feature_fields=air temp gives ["air", "temp"], and then feature field(s) not found.
The JSON list form at line 386 works over HTTP. Document the limit for the scheduled trigger.
| return {"status": "failed", "message": "invalid JSON in request body"} | ||
| except Exception as e: # noqa: BLE001 | ||
| influxdb3_local.error(f"[{task_id}] {type(e).__name__}: {e}") | ||
| return {"status": "failed", "message": str(e)} |
There was a problem hiding this comment.
Follow-up. The response returns str(e) to the caller. That text can hold the full gateway URL, the target database name, and the storage error detail.
Return a stable public message and write the detail to the log. This pattern is repository-wide, so a repository-wide fix is better than a fix here alone.
| else: | ||
| influxdb3_local.write_sync(batch, no_sync=True) | ||
| return len(builders) | ||
| except Exception as e: # noqa: BLE001 |
There was a problem hiding this comment.
Follow-up. The retry catches every exception. A permanent fault, such as a schema conflict, costs about 5 seconds and then fails anyway. Narrow the retry to a transport error and a timeout.
Summary
nori_regressionis a new Processing Engine plugin that predicts a numeric field in an InfluxDB 3 measurement from other columns on the same rows with Nori, Synthefy's in-context-learning tabular regression model, served through the Baseten gateway. It trains on the rows where the target field is present and predicts (imputes) the rows where it is null, writing the predicted values back as a new measurement. It runs as a scheduled trigger (on an interval) or an HTTP trigger (on-demand).What Nori is (for reviewers not familiar with it)
Nori is a tabular regression foundation model. You send it labeled feature rows (
X_train,y_train) and query rows (X_test) in a single request, and it predicts a value for each query row in one forward pass, with no training or fine-tuning. It has no notion of time or row ordering: it only sees numeric feature rows.How the plugin works
fieldplus thefeature_fieldscolumns, filtered bytags.X_train/y_train/X_testto Nori (task: "regression").<measurement>_regressed.Typical uses: backfill a field that dropped out, impute a metric from correlated ones (for example, predict
pressurefromtempandhumidity), or derive an expensive-to-measure field from cheaper ones recorded alongside it.Trigger types
process_scheduled_call): run on an interval; re-imputes any rows still missing the target each tick.process_request): on-demand at/api/v3/engine/<path>; supports an explicitstart_time/end_timewindow.Configuration highlights
measurement,field: the source table and the target field to predict (required).feature_fields: the feature columns (X),./,-separated (required).window, orstart_time+end_time: which rows to read.tags: filter to a single series (required if the measurement holds several series).model: the Nori gateway slug (synthefy/noridefault, orsynthefy/nori-30m).output_measurement,target_database,dry_run,min_history,config_file_path.Full parameter tables are in the README.
Authentication
The Nori gateway key is a secret and is never read from trigger arguments or the request body (both are logged). It is resolved from, in order: an incoming request header (
X-Nori-Api-Key, orAuthorization: Api-Keyon the HTTP trigger), then theNORI_API_KEYenvironment variable on the InfluxDB host. Keys are obtained from the Synthefy console (https://console.synthefy.com/); a single key works for all Nori model variants.One InfluxDB-specific note for the HTTP trigger: InfluxDB's own HTTP layer parses the
Authorizationheader and only accepts the Bearer/Token/Basic schemes, soAuthorization: Api-Keyis rejected before the plugin runs. Use the customX-Nori-Api-Keyheader instead. This is documented in the README.Testing
Verified end-to-end against
influxdb:3-corev3.10.1 with a production key:sensorstable wherepressure = 1000 + 0.5*temp - 0.3*humidity + noise(sd 0.8), the plugin imputed the 24 rows missingpressurefromtempandhumidity. Predictions matched the true relationship to within the training noise (absolute errors 0.02 to 0.71).tagsfilter fails loud (a would-collide guard) rather than silently dropping values; a wrong or missing key returns a cleanfailedrather than crashing.Conformance to repo conventions
scripts/validate_readme.py --plugins nori_regression: 0 errors.scripts/validate_influxdb3_syntax.py --readme influxdata/nori_regression/README.md: 0 errors, all CLI and API usage validated.organization/plugin_name/layout with the JSON metadata docstring, README,manifest.toml, andrequirements.txt, plus aplugin_library.jsonentry (author Synthefy).write_sync/write_sync_to_db(InfluxDB 3.8.2+, the same pattern as themqtt_subscriberandsynthefy_forecastingplugins) so write errors surface during trigger execution instead of being swallowed by the buffered writer.NORI_API_KEYenvironment variable, never from trigger arguments or the request body.Files added
influxdata/nori_regression/nori_regression.py: the plugin (metadata docstring plus implementation).influxdata/nori_regression/README.md: usage, configuration, and troubleshooting.influxdata/nori_regression/manifest.toml: packaging metadata.influxdata/nori_regression/requirements.txt: Python dependencies.influxdata/library/plugin_library.json: registry entry (the plugin lives in theinfluxdata/namespace, authored by Synthefy).Limitations