Repository navigation
Send a relational inference config only when the caller set one - #16
Conversation
Every KumoRelational request carried the engine's default inference config. SDM-backed deployments refuse any non-empty `inference_config` with INVALID_REQUEST, so every relational request failed there although it asked for nothing beyond the defaults. The adapter now forwards only the options the caller set and omits `inference_config` when none are left. Deployments that read the field apply the same defaults to a request that carries none. The Databricks relational notebooks stop asking for a four-estimator ensemble, and the wire snapshot loses the default config from its one relational case. Signed-off-by: Adi Wadaskar <awadaskar@nvidia.com>
19edea5 to
10e337e
Compare
Manushpm8
left a comment
There was a problem hiding this comment.
Verified the central claim against a live relational NIM rather than reading it, and it holds. One defect inline, plus a stale sentence.
The wire loses the config and the answer does not change. Same request, same data, both branches:
wire inference |
prediction | |
|---|---|---|
main |
run_mode, use_prediction_time, inference_config: {kind, num_estimators: 1, column_shuffle, category_shuffle, hop_shuffle} |
[239.27684, 239.424347, 239.129333] |
| this PR | run_mode, use_prediction_time |
[239.27684, 239.424347, 239.129333] |
Byte identical, which is what the legacy backend safety argument needed.
A caller set option still travels, alone. inference_config={'num_estimators': 4} puts exactly {"num_estimators": 4} on the wire, and the SDM backend refuses it by name (400 num_estimators is fixed by the selected profile) rather than this client swallowing it. prediction_statistic='mean' still goes on output and changes the answer, so the median default is untouched.
Suites 2691 passed against both live NIMs, lint and hygiene green. The median contract is still pinned: output.prediction_statistic == 'median' is asserted, and the changed assertion correctly stops treating the engine's output_type as the contract.
Found by Codex, confirmed here by running it.
One stale sentence outside the diff: test_decomposition_is_faithful.py:255 still says the AdventureWorks notebook asks with an ensemble, which this PR removes from both notebooks. The test is unaffected, only the sentence explaining why it exists.
The request kept the caller's own `inference_config` object, and the adapter reads it at send time to decide which options were asked for. Since only those options are now sent, clearing or editing that dict after `materialize()` dropped an option the payload was gathered with, or described a different one than the payload carries. The request now keeps a copy, which covers an `InferenceConfig` dataclass too. Also drops a docstring's claim that the AdventureWorks notebook asks with an ensemble. Signed-off-by: Adi Wadaskar <awadaskar@nvidia.com>
|
Good catch, also fixed in d0abbea. The docstring of |
Manushpm8
left a comment
There was a problem hiding this comment.
Verified on d0abbea. Editing the config after materialize() no longer changes what is sent, for both the clear and the edit, and the dataclass path holds the same way: mutating a RegressionInferenceConfig after materialize still sends the gathered value, and a bare one sends nothing. Full client suite passes, and against a live relational NIM the no-config, dict-default and dataclass-default forms all return the same prediction.
…ebooks into quickstarts (#29) Running the AdventureWorks Databricks notebook from main on serverless turned up three bugs. This PR fixes them, turns the notebook into a quickstart for first-time users, and fixes and aligns the Lakebase notebook. ## Fixes - **`NameError: ENSEMBLE` in the notebook.** #16 removed `ENSEMBLE`, but #8 merged later and still used it. CI didn't catch it because undefined-name checks (ruff F821) were turned off for the Databricks notebooks. They are now on, with `dbutils`, `spark` and `display` declared as builtins. - **Metric view names with a hyphenated catalog were rejected.** For example `edsp-sandbox.kumo_rfm.adventureworks_sales_metrics` failed with `Invalid metric view name`. The view name you pass in is now parsed loosely and each part is quoted before it reaches SQL. The `source:` entries inside the view's YAML are still parsed strictly. - **No engine wheel for aarch64 in CI.** Databricks serverless can land on aarch64, where `%pip` rejected the x86_64-only wheel. CI now builds manylinux wheels for both architectures on native runners, like the release workflow already did. ## Notebooks The AdventureWorks notebook becomes `examples/databricks/kumo_relational_quickstart.ipynb`, a get-started guide for a first-time user, modelled on the [docs quickstart](https://docs.nvidia.com/sdgm/examples/notebooks). It explains how the model works, creates the serving endpoint if it doesn't exist, downloads the AdventureWorks sample tables from Microsoft's repository (pinned to one commit), and walks through four examples asked at a past date and checked against what actually happened, each next to a simple baseline: customers who will stop ordering (with a chart), customers who will place an order over $1,000, filling in a product's line, and customers who will spend over $1,000 asked through a metric view. It ends with an exercise, a template for your own data and clean-up. CI-only parameters are gone from the notebook. Only examples that clearly beat a simple baseline are kept. Product demand forecasts lost to "next 30 days = last 30 days" (median error 24% against 10%), so they are left out, as are the docs quickstart's multi-period forecast, recommendations and color prediction. Columns not known at the prediction time (product sell and discontinue dates, ship date, status, `ModifiedDate`) are left out of the schema. The Lakebase notebook had the same structure problems and badly low forecasts (25.7 units for a product that sold 93 in the last 30 days, about 120 through Unity Catalog). Lakebase declares a foreign key from order lines to order headers, which the client picks up, so header dates set after each order reached the model. The notebook now removes that link and uses the quickstart's schema, and it is rewritten to match the quickstart: it creates the endpoint, loads the sample data into Lakebase with indexed keys, and has the same examples and baselines. ## Tests Adds tests for the metric view cases from the #8 review that were only partly covered: comment markers inside strings and quoted names, backticked non-ASCII names, `DATEDIFF` over `DATE_TRUNC`, names like `EXTRACT` fields, the sampling matrix on DuckDB and SQLite, and a bound on the scanner's work for large expressions. Expected values were checked with `EXPLAIN` on a Databricks SQL warehouse. ## Testing CI and the engine suite pass. Both notebooks were tested on EDSP the way a new customer would run them: each run created its own serving endpoint (from UC model v24) and its own schema, downloaded and wrote the sample tables, and was then rerun on the same resources, after which the endpoint and schema were deleted. - Quickstart, serverless environment 4: [fresh](https://nvidia-edsp-or1.cloud.databricks.com/?o=4453572645853920#job/894522162581140/run/603073303567701), [rerun](https://nvidia-edsp-or1.cloud.databricks.com/?o=4453572645853920#job/563428267755894/run/1121643156657501). Of the 250 customers rated most likely to stop ordering, 97% did, against 78% for the 250 longest since their last order. 85 of the 92 customers who placed an order over $1,000 were in the model's top 250, against 47 for the 250 with the most large orders last year. Product line: 54 of 60 correct, against 18 for always guessing the most common line. Through the metric view: 83 of the 91 customers who spent over $1,000, against 46. - Lakebase, serverless environments 4 and 1: [fresh](https://nvidia-edsp-or1.cloud.databricks.com/?o=4453572645853920#job/392512646322396/run/701048818349349), [rerun](https://nvidia-edsp-or1.cloud.databricks.com/?o=4453572645853920#job/239043159698875/run/663930375891964), [environment 1](https://nvidia-edsp-or1.cloud.databricks.com/?o=4453572645853920#job/215465219291573/run/1091736486387788). Stop ordering 94% against 78%, large orders 67 of 92 against 47, product line 55 of 60 against 18. - An earlier version of the AdventureWorks notebook passed on DBR 14.3, 15.4, 16.4 and 17.3 (x86_64 and aarch64 each) and serverless environments 1 to 6, 14 runs, with the CI wheels for Python 3.10 to 3.12. - The 52 metric view variants from #8 and the live test module give the same results as the last validation of #8. The #8 review cases that had never run on Databricks, and the unquoted hyphenated view name, also pass through both the warehouse and Spark. --------- Signed-off-by: Adi Wadaskar <awadaskar@nvidia.com> Signed-off-by: Manush Murali <manushm@nvidia.com> Co-authored-by: Manush Murali <manushm@nvidia.com> Co-authored-by: Manush Murali <91221099+Manushpm8@users.noreply.github.com>
Problem
structured-data-nims !189 and !197 make the SDM backends (the Kumo Relational and Kumo Tabular NIMs, and the Databricks MLflow model that reuses them) refuse a non-empty
inference.inference_configwith400 INVALID_REQUEST. The estimator count is now fixed when the endpoint starts, throughSDM_NUM_ESTIMATORS.This client sent the engine's default config on every KumoRelational request (
{kind, num_estimators: 1, column_shuffle: false, ...}), becauseRelationalAdapter._inferenceforwarded every key whether or not the caller had set it. So every relational request failed on those backends, although none asked for anything beyond the defaults.Fix
RelationalAdapter._inferencenow forwards only the inference-config keys the caller set and omitsinference_configwhen none are left.run_mode,use_prediction_timeandoutput.prediction_statisticare unchanged, and sessions use the same path. An option the caller does set, such asnum_estimators: 4, is still sent on its own; a legacy backend honours it and an SDM backend refuses it.This is safe for the legacy kumo-ml backend: it reads
inference_config or {}and applies the rest on top ofInferenceConfig.from_task_type(...), whose defaults are identical to the engine's (one estimator, shuffles off, andquantiletransforms with amedianoutput for regression). A request without the field gets exactly what this client used to send.The two Databricks relational notebooks stop passing
ENSEMBLE = {'num_estimators': 4, 'column_shuffle': True}, which SDM backends refuse; there the ensemble size is set per endpoint throughSDM_NUM_ESTIMATORS.tests/golden/wire_payloads.jsonis regenerated withscripts/snapshot_wire_payloads.py --write, and it only loses the default config from its one relational case.Verification
Updated adapter, session and median tests cover a default request sending no config (through both transports), the engine's full defaults not being sent, and an explicit option being sent alone. They fail on main. The client suite passes (1913 passed), along with ruff, mypy and the golden snapshot check.
End to end, an AdventureWorks
PREDICT SUM(sales_order_details.OrderQty, 0, 30, days) FOR EACH products.ProductIDfor product 712 throughSDMClient.for_databricks_servingagainst the SDM staging endpointkumo-rfm-2-1-stg-v23-mr149-reviewreturns a prediction (120.97) with noinference_configin the request body, while the same request from main gets400 INVALID_REQUEST.