Repository navigation
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?
There was a problem hiding this comment.
.github/workflows/ci.yml:273
P2: engine-wheels smoke test can still pick siblings from PyPI
Small catch: this job still finds siblings through PIP_FIND_LINKS against the engine's ==1.0.0 pins, the same mechanism this PR replaces elsewhere. Its log on this run downloads api/pql/connectors 1.0.0 from PyPI, so engine wheels are smoke-tested without the new holds. Could we install _sibling_wheels/*.whl by file name in a before-test step?
There was a problem hiding this comment.
Done in 0e3a4bf, as you suggested. The engine's [tool.cibuildwheel] has a before-test that installs _sibling_wheels/*.whl by file name before cibuildwheel installs the engine, so its ==1.0.0 pins are already met when it does. PIP_FIND_LINKS is gone from here, from publish.yml and from the pyproject. In e01808f the directory is passed as an argument rather than written into the Python string.
On 0e3a4bf, every engine-wheels row installs api, pql, client and connectors from _sibling_wheels/ for all five interpreters (Processing ./_sibling_wheels/structured_data_models_api-1.0.0-py3-none-any.whl and so on), and none is downloaded from PyPI.
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.
| pip install --dry-run --find-links "$GITHUB_WORKSPACE/dist" \ | ||
| "structured-data-models-client[relational,sqlite,duckdb]==$VERSION" | ||
| pip install --dry-run \ | ||
| "$(ls dist/structured_data_models_client-$VERSION-*.whl)[relational,sqlite,duckdb]" \ |
There was a problem hiding this comment.
The underlying collision is that main builds wheels at the already-published 1.0.0 while changing their dependency metadata. Switching to file paths fixes this job, but any other install of a sibling by name and version (GitLab, cibuildwheel, publish, a local --find-links) can still pick up PyPI's 1.0.0. Bumping main to a dev version after each release (e.g. 1.0.1.dev0) would remove the ambiguity everywhere at once.
There was a problem hiding this comment.
Agreed that it would remove the ambiguity at the source. With this PR, though, no CI or release job installs a first-party distribution by name and version any more:
- the GitLab jobs, the engine smoke test and the release SBOM all take the wheels by file;
- the one remaining
--find-linksis the release SBOM's engine install, which also passes--no-index.
A dev version on main would still help local --find-links installs. It also changes how a release is cut: RELEASING.md bumps _version.py at release time, publish.yml checks the tag against the built version, and it decides the next number. I'd rather propose that separately than fold it into this PR.
| pip install --find-links "$GITHUB_WORKSPACE/dist" \ | ||
| "structured-data-models-client[sqlite,duckdb]==$VERSION" | ||
| pip install \ | ||
| "$(ls dist/structured_data_models_client-$VERSION-*.whl)[sqlite,duckdb]" \ |
There was a problem hiding this comment.
Nit: "$(ls dist/structured_data_models_client-$VERSION-*.whl)[sqlite,duckdb]" doesn't guard against zero or multiple matches. If the wheel is missing, ls fails inside the command substitution without tripping bash -e, and pip gets the bare string [sqlite,duckdb] and fails with an obscure "Invalid requirement". Two matches become one newline-joined argument. An explicit check that exactly one wheel matches would make the failure say which wheel is missing.
There was a problem hiding this comment.
Fixed in 0e3a4bf. The pure-Python wheels are no longer found with ls or a glob. Each is named in full (dist/structured_data_models_client-$VERSION-py3-none-any.whl[...]), so a missing one fails with pip naming that file, and two cannot match.
The engine's wheel in the build dry-run is still a glob, because its tag is the runner's. The step before it now checks there is exactly one wheel per distribution, so that glob matches exactly one. GitLab got the same changes.
| ): | ||
| recorded = distribution(name).read_text('direct_url.json') | ||
| source = json.loads(recorded or '{}').get('url', 'an index') | ||
| assert '/dist/' in source, ( |
There was a problem hiding this comment.
Nit: '/dist/' in source is a substring test, so any direct-URL install whose URL contains /dist/ passes. Parsing the file:// URL and comparing it with Path(os.environ['GITHUB_WORKSPACE'], 'dist') would make the check exact.
There was a problem hiding this comment.
Fixed in 0e3a4bf. The check parses the file:// URL and compares the wheel's directory with Path($GITHUB_WORKSPACE, 'dist').resolve().
I checked it locally both ways. It refuses a PyPI install, and it refuses a dist/ directory somewhere else, which the substring test let through. The internal pipeline's installed-wheel job now runs the same check against $CI_PROJECT_DIR/dist.
| # 3.1 builds a frame around an integer past the float range where earlier | ||
| # releases refused one, and a request's description relies on that | ||
| # refusal to leave out the table carrying it. | ||
| "pandas>=2.2.2,<3.1", |
There was a problem hiding this comment.
This makes the client's behavior depend on pandas refusing to build a frame around an out-of-float-range integer. Any pandas release that accepts it silently changes which tables a description leaves out, and the cap pushes the constraint onto every client user. Could the client check the range explicitly so it works on any pandas?
There was a problem hiding this comment.
Done in 10fd2a2, and the cap is gone. _wire_frame now refuses an integer past the float range itself, before pandas sees the rows, so the same table is left out on every pandas release.
The test is now parametrized with a mixed column as well ('x' beside -(10**400)). pandas 3.0 accepted that column, so the case shows the rule no longer depends on pandas. Without the fix, the test fails on 3.0 and on 3.1.
| # 2.0 warns on an Arrow fetch from a DB-API cursor, which the engine | ||
| # makes for every table and sample it reads. Lifting this moves those | ||
| # reads off the cursor. | ||
| "duckdb>=1.5.0,<2", |
There was a problem hiding this comment.
As I understand it, DuckDB 2.0 only adds a warning, and the engine's test suite treats it as an error; nothing fails at runtime. A user-facing <2 cap stops anyone who needs DuckDB 2.x from installing [duckdb]. Could we move the Arrow fetch off the cursor (as the comment suggests), or filter that specific warning in the test config?
There was a problem hiding this comment.
Moved the reads off the cursor in c75eb78, and dropped the cap. The engine's six DuckDB reads now use cursor.sql(sql).to_arrow_table(), which is what DuckDB 2.0's warning recommends, and 1.5 already has it.
read_table made the same fetch over a caller's DuckDB connection, so users would have seen the warning there too. It now reads through a relation as well, and a new test fails on 2.0 without the change.
The engine suite (warnings as errors) passes on DuckDB 2.0.0 and on 1.5.6, and neither run raises a DuckDB warning.
| 'pandas', | ||
| # 3.1 raises taking a `DateOffset` from `Timestamp.min`, which is the | ||
| # anchor time of every graph without a time column. | ||
| 'pandas<3.1', |
There was a problem hiding this comment.
This cap seems to hide a fixable engine bug: the anchor time for graphs without a time column is Timestamp.min - DateOffset(0) (rfm/base/sampler.py, rfm/backend/*/sampler.py). Fixing the anchor computation (not subtracting a zero offset from Timestamp.min, or clamping) would avoid holding every engine user below pandas 3.1, e.g. on newer Databricks runtimes.
There was a problem hiding this comment.
Fixed in 20c4598, and the cap is gone. The raise comes from moving one scalar timestamp in rfm.py: Timestamp.min - DateOffset(0), where DateOffset(0) is a static query's placeholder step. pandas 3.1 takes even an empty offset through microseconds, which puts Timestamp.min out of range.
shift_time now leaves a timestamp where it is when the offset moves nothing. Every scalar anchor shift in rfm.py, and the base sampler's window check, go through it.
The sampler call sites you listed shift whole columns, which 3.1 handles; I checked each one, so they are unchanged.
New tests:
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>
Summary
DuckDB 2.0.0 and pandas 3.1.0 broke CI on
mainand on every branch based on it. This PR first brought #47's holds (duckdb<2,pandas<3.1) tomain. After review, it now fixes what each release broke instead, and the holds are gone: no install caps either dependency, anduv.lockis unchanged frommain.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.#41's branch still carries #47's holds. When #41 merges
mainback in, takemain's side of the three dependency lines.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