Repository navigation
Support DuckDB 2 and pandas 3.1 on main, and test the wheels CI built - #51
Conversation
duckdb 2.0.0 was published today and the extra was unbounded, so every CI run since picks it up. 2.0 warns on an Arrow fetch from a DB-API cursor, which the relational engine makes for every DuckDB table and sample it reads, and the engine suite runs warnings as errors, so the required `engine` check fails on main and on every open pull request. The extra is held below 2, and this repository ships against the 1.5 it is tested on. Lifting the bound is its own change, moving those reads off the cursor. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Manush Murali <manushm@nvidia.com>
pandas 3.1.0 was published this morning, and neither the client nor the engine bounded it, so CI picked it up within hours. 3.1 raises taking a `DateOffset` from `Timestamp.min`, which is the anchor time the engine derives for every graph without a time column, so every relational request over a static graph fails with OutOfBoundsDatetime: 24 client tests that run with an engine present. 3.1 also builds a frame around an integer past the float range where 3.0 refused one, and a request's description relies on that refusal to leave out the table carrying it. Both are held below 3.1, as DuckDB is below 2, and this ships against the 3.0 it is tested on. Lifting it is its own change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Manush Murali <manushm@nvidia.com>
The installed-wheel, sbom and build jobs installed the client set with `--find-links dist` and `==$VERSION`. Once 1.0.0 was on PyPI, pip resolved the published release as readily as the wheel built here, and the installed-wheel job began testing it: a pull request adding to the client failed there because the release lacked the addition. Each distribution is now named by its wheel file, and the installed-wheel job checks that every one was installed from dist/ before running the suite. The release workflow keeps `--find-links`: the version it builds is not on PyPI yet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Manush Murali <manushm@nvidia.com>
The hygiene rule on main refuses a structured-data-models- name that is not
one of the five published distributions, and a name assembled from
structured-data-models-{name} reads as one. Spelling out the four names
passes it, and passes the looser rule on run-models-in-process too.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Manush Murali <manushm@nvidia.com>
adi-kumo
left a comment
There was a problem hiding this comment.
Design: Base client caps pandas for one best-effort description case
The cap lands in the base client install, so every NIM-only user is put in resolver conflict with any app that needs pandas 3.1, and lifting it takes a release. The client's reason is only that _wire_frame relies on pandas incidentally refusing an integer past the float range. Could we make that refusal explicit in _wire_frame and keep the cap only on the relational engine, where Timestamp.min - DateOffset(0) really raises?
JGagacheva
left a comment
There was a problem hiding this comment.
Thanks for this. Installing the wheels by file path is the right fix. But the same --find-links + ==$VERSION pattern is still in jobs this PR doesn't touch, so those jobs can still install PyPI's 1.0.0 (which has neither cap) and not the wheels built in the run:
.gitlab-ci.yml: the build dry-run (line 205), the installed-wheel job (line 339) and the sbom job (line 415). The internal pipeline keeps testing the published release, so the new caps never reach it..github/workflows/ci.yml:273: the engine-wheels smoke test still finds sibling wheels throughPIP_FIND_LINKS(also/project/_sibling_wheelsin the enginepyproject.toml:148, andpublish.yml:186). The engine pins api and pql==1.0.0, so the smoke test can import the published siblings and not the ones built in the run, on all 5 platform rows..github/workflows/publish.yml:366: the release SBOM (software bill of materials) step still runspip install --find-links dist ./packages/structured-data-models-client[...]. If a release is re-run after a partial upload, the SBOM describes what's on PyPI rather than what's being released.
I'd want these covered before merging, since the PR's goal isn't met without them. Inline comments below cover the caps and a few smaller points. I reviewed by reading the code only; nothing was run.
DuckDB 2.0 warns that Arrow output from a DB-API result makes an extra copy of the rows, and the engine's suite runs warnings as errors. Every DuckDB read now goes through `con.sql(query).to_arrow_table()`, which DuckDB names as the replacement and which 1.5 already has: the engine's six table and sampler reads, and `read_table` over a caller's DuckDB connection. The `duckdb` extra goes back to `duckdb>=1.5.0`. `read_table` still runs on the caller's own connection, so temp tables and registered frames stay visible. On DuckDB 1.5 a statement without a result set makes `sql()` return nothing, and that reads as an empty frame. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Manush Murali <manushm@nvidia.com>
A graph without a time column is anchored at `Timestamp.min`, and a static query steps by the placeholder `DateOffset(0)`. pandas 3.1 takes even that offset through microseconds, so `Timestamp.min - DateOffset(0)` raises `OutOfBoundsDatetime` and every relational prediction over such a graph failed. pandas 3.0 already raised the same way for `DateOffset(days=0)`. `shift_time` moves a timestamp by an offset and leaves it as it is when the offset moves nothing. Every scalar anchor shift in `rfm.py` and the window check in the base sampler go through it. The samplers' own shifts work on Series, which pandas 3.1 handles, and are unchanged. The engine goes back to an unbounded `pandas`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Manush Murali <manushm@nvidia.com>
A request's description reads each wire table into a frame and leaves out one it cannot read. For an integer past the float range that relied on pandas refusing the frame, which 3.0 did only when every value in the column was a number, and 3.1 does not do at all. The check is now explicit, ahead of pandas, so the same table is left out on every release. The client goes back to `pandas>=2.2.2`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Manush Murali <manushm@nvidia.com>
Naming a sibling by name and version with `--find-links` resolves the published release of that version as readily as the wheel built in the run. Every job that still did so now names the wheel itself: - The engine's smoke test, on all five rows in CI and in the release, and in the internal pipeline's `wheels` job. `PIP_FIND_LINKS` is gone; a `before-test` installs `_sibling_wheels/*.whl` by file before cibuildwheel installs the engine, so its exact pins are already met. - The internal pipeline's build dry-run, `installed-wheel` and `sbom`. - The release SBOM. The engine has a wheel per platform there, so it comes from `dist/` alone with `--no-index`. The pure-Python wheels are named in full rather than found with `ls`, so a missing one fails with pip naming that file, and two cannot match. The build jobs check for exactly one wheel per distribution, which covers the engine's runner-tagged glob. `installed-wheel` compares each distribution's `direct_url.json` with the `dist/` directory exactly, rather than looking for `/dist/` anywhere in the URL, and the internal pipeline's job now makes the same check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Manush Murali <manushm@nvidia.com>
`shift_time` judged an offset empty when it left 2000-01-01 where it was. `DateOffset(months=1, days=-31)` does that on that date and moves 2020-02-01 back two days, so the shift was skipped. An offset is empty when its multiple is zero or every field it names is zero, which holds on every date. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Manush Murali <manushm@nvidia.com>
cibuildwheel 4.3.1 expands `{project}` to `/project` in the Linux
container and to `.` on macOS and Windows, so nothing it substitutes
today reaches the Python source with a backslash in it. An argument holds
any path as it is, including one with spaces, where a string literal
would read a Windows path's backslashes as escapes.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Manush Murali <manushm@nvidia.com>
A zero multiple does not make every pandas offset a no-op: `DateOffset(n=0, year=2022)` still replaces the year, `MonthEnd(0)` still rolls to the month's end, and `normalize=True` still drops the time of day. `shift_time` now leaves a time as it is only for a plain `DateOffset`, without `normalize`, whose fields are all relative and that moves nothing. Every other offset is applied. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Manush Murali <manushm@nvidia.com>
|
Thanks both. I've reworked the PR. The holds are gone: each break is fixed where it happens, and every job installs the wheels it built. The changes are c75eb78..ee5e544, and the inline replies cover the details. @adi-kumo, on the design point (F38): no install caps pandas or DuckDB any more, base or extra.
@JGagacheva, on your three points:
Verified:
|
Signed-off-by: Manush Murali <manushm@nvidia.com>
## Summary Snowflake attributes a session's queries by the application the session names at login. Sessions the client opened named none, so they showed up as the driver's default. Each Snowflake session the client opens now names itself `nvidia/kumo-sdm` when nothing else names it. - **The name is added after the driver resolves the connection.** `_NamedConnection` subclasses the driver's `SnowflakeConnection` and adds `application` in `connect`. The driver first calls `connect` once it has merged three things: - the caller's arguments; - the `connections.toml` profile, read from the caller's `connections_file_path` if one is given; - its own detection of `SF_PARTNER`, Streamlit, Jupyter and Snowflake notebooks. - **Any other name is kept.** That includes an `application` argument, one in the profile, `SF_PARTNER`, or one of the driver's notebook tags. - **Everything else is the driver's own.** The client no longer reads the driver's config or chooses its default connection. The connections file, the default connection and the error when nothing is configured all behave as on `main`. - **Connections the caller owns are unchanged.** A borrowed Snowpark session, or a connection the caller opens and passes to a source, is used as it is. **Stacked on #51.** The base is #51's branch so that CI runs with its DuckDB 2 and pandas 3.1 fixes; this branch is red on `main` until #51 lands. Once #51 merges, I'll point this PR back at `main`. ## Tests `test_snowflake_names_a_session_only_where_nothing_else_does` runs the driver's real resolution (snowflake-connector-python 4.8.0) over a real `connections.toml`, and stops at login. Its cases: | Session opened with | Application sent | |---|---| | credentials | `nvidia/kumo-sdm` | | credentials and `application='theirs'` | `theirs` | | credentials, `SF_PARTNER` set | the partner | | credentials, Streamlit loaded | `streamlit` | | a profile that sets `application` | the profile's | | a profile that doesn't | `nvidia/kumo-sdm` | | only `connections_file_path` | the default profile, and `nvidia/kumo-sdm` | | no arguments | the default profile, and `nvidia/kumo-sdm` | What the cases catch: - Against `main`'s backend, the four cases that should get `nvidia/kumo-sdm` fail. - Against the first version of this PR, the Streamlit and profile cases fail. Those are the two regressions review found. Locally, on pandas 3.1.0 and DuckDB 2.0.0, everything passes: - the client-side suite: 2702 passed; - the engine suite: 2588 passed; - ruff, ruff format, mypy and the hygiene check. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Manush Murali <manushm@nvidia.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the rework. The three points from my earlier review are all addressed. One thing before this merges: #50 (naming Snowflake sessions `nvidia/kumo-sdm`) was merged into this PR's branch instead of `main`, so #51 now also carries f54b093. That brings in `snowflake.py` and its tests, the `APPLICATION` entry in `AGENTS.md`, the Snowflake entry in `CHANGELOG.md`, and a line in `docs/about/architecture.md`. None of it is in this PR's title or description, and a squash merge would put the dependency fixes and a new feature on `main` as one commit. Could you drop f54b093 from this branch and re-land the Snowflake change as its own PR against `main` once this one merges? |
#50 was merged into this branch rather than main, so a squash of this pull request would land it on main as part of the dependency fixes. It goes back in as its own pull request against main. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Manush Murali <manushm@nvidia.com>
main now carries #41, which held duckdb below 2 and pandas below 3.1. This branch fixes what those releases broke, so the merge keeps it uncapped: the three requirements, the lock's matching specifiers and the changelog drop the holds, and the two Fixed sections become one. In ci.yml each conflict was the same wheel-by-file change, and this branch's stricter version is kept alongside #41's in-process engine job. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Manush Murali <manushm@nvidia.com>
|
@JGagacheva Done. df5945e reverts f54b093, so this branch is back to what you reviewed. I reverted rather than dropping the commit, so nothing already pushed is rewritten, and the squash comes out the same. The Snowflake change is now #54, against #41 merged into |
Cached Tabular predictions build a full envelope, including encoding every context row, and then discard the context before sending the request. Skip that encoding for session scoring while retaining schema checks, pinned dtype conversion, and query serialization. One-shot predictions and session creation still encode the context. ## Measured benefits Mean one-row prediction latency fell from **467.4 ms to 52.4 ms**: approximately **415 ms saved per call, 89% lower latency, or 8.9× faster**. This improvement comes from removing repeated encoding of the already-cached context. ## Benchmark configuration - GPU: NVIDIA A10G; client and server on the same machine. - Model: Kumo Tabular Large, BF16, one estimator. - Task: fraud classification using a fitted session with a cached context. - Input: 10,000 context rows, 30 numerical features, one query row. - Timed operation: `FittedModel.predict(...).to_pandas()`; context fit and Request construction are outside the timer. ## Validation - Same server and fitted session, two warmups followed by 20 alternating measurements per version. This is a sequential latency comparison, not a throughput or P99 claim. - Payload contents matched excluding request IDs. Predictions were bitwise identical for query batches of 1, 8 and 1,000 rows. - Four regression cases covering classification/regression with and without a pinned schema fail before the change and pass afterward. Public API and all nine wire-payload snapshots are unchanged. - Ruff, formatting, mypy, repository hygiene and attribution checks passed for this patch. - CI currently needs the compatibility and built-wheel test fixes in #51. Verified this patch applied to #51 at `f54b0938af0fcb12706b3a740304d26ae4837f0c` with pandas 3.1.0 and DuckDB 2.0.0: main suite 2,679 passed, separate engine suite 2,580 passed, and 1,963 client tests passed against the exact newly built wheel installed by filename. Live-service tests were excluded; engine verification reused the unchanged native extension. These are local combined-tree results, not a green CI claim for this PR alone. Signed-off-by: GraysonWu <graysonw@nvidia.com> Co-authored-by: Manush Murali <91221099+Manushpm8@users.noreply.github.com>
## Summary Snowflake attributes a session's queries by the application the session names at login. Sessions the client opened named none, so they showed up as the driver's default. Each Snowflake session the client opens now names itself `nvidia/kumo-sdm` when nothing else names it. This was reviewed as #50, which merged into #51's branch rather than `main`. #51 has reverted it, and it lands here on its own, with one fix from review since. - **The name is added after the driver resolves the connection.** `_NamedConnection` subclasses the driver's `SnowflakeConnection` and adds `application` in `connect`. The driver first calls `connect` once it has merged three things: - the caller's arguments; - the `connections.toml` profile, read from the caller's `connections_file_path` if one is given; - its own detection of `SF_PARTNER`, Streamlit, Jupyter and Snowflake notebooks. - **Any other name is kept.** That includes an `application` argument, one in the profile, `SF_PARTNER`, or one of the driver's notebook tags. - **Fixed since #50:** with no arguments, or only `connections_file_path`, the driver replaces what it was given with the default connection's parameters, which drops the name it had just detected. The session was then named `nvidia/kumo-sdm` under Streamlit or `SF_PARTNER`. `_NamedConnection` now asks the driver's detection again before using its own name (e6e8992). - **Everything else is the driver's own.** The client does not read the driver's config or choose its default connection. The connections file, the default connection and the error when nothing is configured all behave as before. - **Connections the caller owns are unchanged.** A borrowed Snowpark session, or a connection the caller opens and passes to a source, is used as it is. ## Tests `test_snowflake_names_a_session_only_where_nothing_else_does` runs the driver's real resolution (snowflake-connector-python 4.8.0) over a real `connections.toml`, and stops at login. Its cases: | Session opened with | Application sent | |---|---| | credentials | `nvidia/kumo-sdm` | | credentials and `application='theirs'` | `theirs` | | credentials, `SF_PARTNER` set | the partner | | credentials, Streamlit loaded | `streamlit` | | a profile that sets `application` | the profile's | | a profile that doesn't | `nvidia/kumo-sdm` | | only `connections_file_path` | the default profile, and `nvidia/kumo-sdm` | | only `connections_file_path`, `SF_PARTNER` set | the default profile, and the partner | | no arguments | the default profile, and `nvidia/kumo-sdm` | | no arguments, Streamlit loaded | the default profile, and `streamlit` | What the cases catch: - Against `main`'s backend, six fail: the four that should get `nvidia/kumo-sdm`, and the two where the driver drops the name it detected. - Against #50 as reviewed, those last two fail. Locally, the connectors suite passes (309 passed, 9 skipped), with ruff, ruff format, mypy and the hygiene check clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Manush Murali <manushm@nvidia.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Summary
DuckDB 2.0.0 and pandas 3.1.0 broke CI on
mainand on every branch based on it. #41 has since brought #47's holds (duckdb<2,pandas<3.1) tomain. This PR fixes what each release broke instead, and takes the holds back out: no install caps either dependency, anduv.lockdiffers frommainonly in those three specifiers.DuckDB 2.0 warns that Arrow output from a DB-API result makes an extra copy of the rows, and the engine's suite runs warnings as errors. Two places now read through a relation,
con.sql(query).to_arrow_table():read_tableover a caller's DuckDB connection.DuckDB's warning names that call as the replacement, and 1.5 already has it.
pandas 3.1 raises
OutOfBoundsDatetimeonTimestamp.min - DateOffset(0). A graph without a time column is anchored atTimestamp.min, and a static query steps byDateOffset(0), so every relational prediction over such a graph failed.shift_timeleaves a timestamp where it is when the offset moves nothing. Every scalar anchor shift inrfm.py, and the base sampler's window check, now go through it.pandas 3.1 also builds a frame around an integer past the float range, where earlier releases refused one. A request's description relied on that refusal to leave out the table carrying it.
_wire_framenow refuses such a table itself, on every pandas release.Every CI and release job now installs the wheels CI built by file name. With
--find-linksplus a name and version, pip resolves PyPI's release of that version as readily as the wheel built in the run, so a job could test or describe the published 1.0.0 instead.builddry-run,installed-wheelandsbom. This PR cherry-picked these from586882c57, then tightened them:buildjob checks there is exactly one wheel per distribution.installed-wheelcompares each distribution'sdirect_url.jsonwithdist/exactly.PIP_FIND_LINKSis replaced by a cibuildwheelbefore-testthat installs_sibling_wheels/*.whlby file name before the engine.builddry-run,installed-wheelandsbom. Itsinstalled-wheelnow makes the samedirect_url.jsoncheck.dist/with--no-index.Not in this PR: putting a development version on
mainbetween releases. That changes how a release is cut, so it is a separate proposal.Verification
read_tableover DuckDB warns.shift_timemoves byDateOffset(months=1, days=-31), an offset that cancels on some dates../dist/or./_sibling_wheels/.direct_url.jsoncheck refuses a PyPI install, and adist/directory somewhere else._sibling_wheels/for all five Python versions, and none from PyPI.🤖 Generated with Claude Code