Support Python 3.13 / NumPy 2 / pandas 3, and fix the correctness bugs it exposed - #1372
Open
ran-haim wants to merge 10 commits into
Open
Support Python 3.13 / NumPy 2 / pandas 3, and fix the correctness bugs it exposed#1372ran-haim wants to merge 10 commits into
ran-haim wants to merge 10 commits into
Conversation
Supporting 3.13 and supporting NumPy 2 are the same task: cp313 has no wheels for numpy<2, pandas<2.2, scipy<1.14 or matplotlib<3.9, so the floors have to rise and the code has to work against those majors. Library fixes: - np.float_ was removed in NumPy 2.0, so TableSample.category() raised AttributeError on any float column. Use np.float64 (an exact alias in NumPy 1.x, so this stays correct on both). - read_pandas() failed to detect string columns under pandas 3, which infers a dedicated string dtype instead of object. Undetected string columns are never quoted, so a value containing the separator was ingested into the wrong columns. - read_pandas() also needs quotechar=None: pandas >= 3.0 escapes the quotechar even under QUOTE_NONE, turning the quotes added for COPY's ENCLOSED BY into literal data. pandas 2.x output is unchanged. - to_csv() built its records with "\n" but wrote in text mode, so on Windows the files came out CRLF and COPY (record terminator "\n") mis-parsed them - 528 of 1234 rows survived a round trip. Write with newline="". Packaging: - python_requires >= 3.10 (3.9 is EOL and blocks numpy>=2.1), 3.13 classifier, modern floors, and ipython added to install_requires - it is imported at package-import time but was never declared. - setup.py read README.md with no encoding, failing on non-UTF-8 locales, and hardcoded a version that could drift from __init__.py. It now single-sources the version by parsing it. - Version 1.2.0: dropping 3.9 and raising floors is not a patch, and PyPI already holds a different 1.1.1. - tox/CI matrices cover 3.10-3.13; tensorflow>=2.18 replaces the 2.15.1 pin, which has no cp312/cp313 wheels, removing the !py312 exclusion. Test suite (tests_new/core: 786 passed, 0 failed): - pandas 3 represents string NA as nan, not None, and nan is truthy - "1 if x else 0" and "row is not None" silently stopped detecting NULLs. Use notna()/fillna(). - groupby(by).last(columns) passed a column name into last()'s first positional parameter, numeric_only; pandas 3 validates it. - rolling().sem(ddof=0) compensated for a pandas 2 bug that 3.0 fixed. VerticaPy computes STDDEV/SQRT(COUNT) with a sample STDDEV, so ddof=1 is the matching definition; verified element-wise against a live server. The test had been checking correct SQL against a wrong oracle. - pytest 9 rejects approx() on datetimes without a tolerance, and no longer accepts a trailing comma in parametrize argnames. - Shared normalize_na() helper for None-vs-nan comparisons. Also fixes 25 invalid escape sequences (SyntaxWarning since 3.12, a future SyntaxError). These are LaTeX in docstrings; the backslashes are doubled rather than made raw, so every string value is bytewise unchanged - verified by diffing all 46,727 string constants before and after. CLAUDE.md documents how to run the live-Vertica suite in Docker, which is no longer obvious: Community Edition was removed from Docker Hub and is unsupported from Vertica 26.1, so the CI's opentext/vertica-ce pull cannot succeed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both found by running the plotting suite against a live Vertica server. Axes.boxplot(labels=) was renamed tick_labels in Matplotlib 3.9 and removed in 3.11, so every box plot raised TypeError. That accounted for 7 of the 8 plotting failures. vDataFrame.corr(focus=...) sorted its results with key=abs(tup[1]). A NaN correlation - which any all-NULL column produces - makes abs() return NaN, and NaN compares False against everything, so Timsort emitted an arbitrary order. With the titanic dataset's all-NULL "body" column the rendered plot was genuinely mis-ranked: before: survived, pclass, fare, age, body(NaN), parch, sibsp after: survived, pclass, fare, parch, age, sibsp, body(NaN) parch (r=.087) was being displayed as less correlated than age (r=.042), and the NaN sat in the middle of the ranking. Rank NaN below every real magnitude so it lands last, deterministically. tests_new/plotting: 923 passed, 83 skipped, 0 failed (was 8 failed). CLAUDE.md documents the live-Vertica setup, and records that the performance suite's 36 failures are a missing Graphviz `dot` binary - the Python "graphviz" package is only the binding and does not bundle the executables. docs/superpowers/plans/ carries the running plan. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both found by running tests_new/machine_learning against a live Vertica server, which resolved 231 failures into three causes. pandas removed Series.ravel in 3.0 (deprecated in 2.2), breaking 93 tests. Use np.ravel(), which accepts Series and ndarray alike and so is correct on both pandas 2.2 and 3.0. Only the calls actually made on a Series were changed; confusion_matrix(...).ravel() and .to_numpy().ravel() operate on numpy arrays and are left alone. test_tree_model.py: 93 failed -> 4 failed, 317 passed. remove_model_dir() was a silent no-op on Windows. It shelled out to "test -d", which resolves to Git's /usr/bin/test.exe and interprets a POSIX-looking /tmp/... path against the MSYS root rather than C:\tmp, so the existence check always reported "does not exist" and the removal branch never ran. Even reached, it called "sudo rm -rf", which does not exist on Windows. The helper only prints and never raises, so both legs failed silently. Cleanup therefore never happened and EXPORT_MODELS refused to overwrite the leftover directory. The arithmetic confirms it: 13 model classes x 4 categories, minus 2 pmml skips, is 50 exports; the first category per class creates the directory and passes, so 37 fail. import and load run after the directory exists and fail 50 each. 37 + 50 + 50 = 137, the exact observed count. Replaced with os.path.isdir + shutil.rmtree, keeping the original sudo rm -rf as the POSIX fallback for the case where the directory is owned by the server's dbadmin user. Where no filesystem is shared with the server, isdir returns False and the helper no-ops exactly as before. test_model_management.py: 137 failed -> 163 passed, 0 failed. The 4 remaining prc_auc/RandomForestClassifier failures are NOT from this port: they produce byte-identical values under pandas 2.2.3 and 3.0.5, and were previously masked by the ravel failure aborting the test first. Their tolerance is left untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
conftest.py assigned the same precision-recall trapezoid to both "auc"
and "prc_auc", and roc_auc_score appeared nowhere in the file:
classification_metrics_map["auc"] = skl_metrics.auc(recall, precision)
classification_metrics_map["prc_auc"] = skl_metrics.auc(recall, precision)
The VerticaPy side of the "auc" assertion reads Vertica's ROC AUC, so
the test has been comparing two different quantities. On titanic the two
sit roughly 3% apart, which is precisely the rel=0.03 the auc tolerance
was fitted to, so the wrong reference was being masked by a tolerance
widened to accommodate it.
Effect across tests_new/machine_learning/vertica: 14 auc assertions now
compare ROC AUC against ROC AUC and pass.
One test newly fails, and it is a true positive rather than a
regression:
test_score[vpy_metric_name0-auc-XGBClassifier]
assert 0.8517505750063886 == 0.9104310418263907 +/- 0.0455216
Vertica's XGB ROC AUC is about 6% below scikit-learn's. That gap was
previously hidden because sklearn's PR AUC (~0.85) happened to fall
within tolerance of Vertica's ROC AUC. Its tolerance is deliberately NOT
widened: the tolerance table is machine-generated from whatever passed
at the time (log_loss sits at 9e10), and VerticaPy's own documentation
states the policy as 1% for regression and 10% for classification.
Re-deriving these bounds is a maintainer decision, and loosening one to
force green is what let this defect survive since PR vertica#1157.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
XGBInitializer silently dropped xgboost-spelled kwargs (only max_depth and learning_rate survived by name collision with the Vertica spelling) and py() never forwarded most of what it stored, so the python XGBoost reference model trained with mismatched hyperparameters (e.g. eta=0.3 instead of 0.1). Accept both spellings per parameter, with the Vertica spelling taking precedence, reject unrecognized kwargs instead of silently ignoring them, and forward learning_rate/gamma/reg_lambda/ subsample/colsample_* from py(). Also drop the incorrect reg_alpha= weight_reg mapping in model_score() — Vertica's weight_reg is an L2 penalty (reg_lambda), not L1. See task-1-report.md for full before/after pass counts: the fix is correct per spec but test_score[...-auc-XGBClassifier] still narrowly misses its 5e-2 tolerance (gap improved 6.45%->5.30%), and test_predict[XGBClassifier] newly regresses as a direct, expected consequence of py() now forwarding its stored defaults. No tolerances were changed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The test harness hardcoded tree_method="exact" for the python-side XGBoost
reference model. That is a different split-finding algorithm from the one
Vertica runs, and it ignores max_bin entirely -- so the nbins/max_bin value
the harness forwards was dead code.
Vertica's split_proposal_method='global' with nbins is the XGBoost paper's
approximate-greedy split finding with a global proposal, which is xgboost's
tree_method='approx' with max_bin as the bin count. Measured on titanic
(max_ntree=10, max_depth=5, nbins=32, learning_rate=0.1, weight_reg=0):
exact, max_bin=32/150/1000 -> roc_auc 0.89691200 (identical)
approx, max_bin=32 -> roc_auc 0.88798237
against Vertica's 0.851751, taking the gap from -5.04% to -4.08% and inside
the existing 5e-2 tolerance. No tolerance was changed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HD4ecxVqvgxNP9No91e2Xp
Six assertions compare a Vertica-trained model against a scikit-learn-trained one and fail because the two engines are different algorithms, not because any code is wrong. They are not flaky: both sides are deterministic and reproduce bit-for-bit across independent retrains. No tolerance is widened. The sklearn reference's own seed-to-seed spread is 1.89% for prc_auc against a 0.40% tolerance and 16.08% for predict-mean against 7.00%, so no tolerance value is reachable. Widening is also what hid the auc/ROC reference defect from 2024-02 until now. Marked via request.applymarker at runtime rather than in the parametrize lists, because those lists are shared across model classes -- marking there would also xfail the DecisionTreeClassifier and XGBClassifier cases, which pass. strict=True is deliberate: both sides are deterministic, so an unexpected pass is a real signal. Also fixes test_prc_curve comparing a 30-point vpy curve against a full-resolution sklearn one; nbins is now explicit at 10000, matching what prc_auc_score uses. That is a correctness fix independent of the verdict. Verified with pinpoint node IDs: 4 target RF assertions -> 4 xfailed test_predict RF + XGBClassifier -> 2 xfailed test_predict XGBRegressor + DTC -> 2 passed 8 neighbouring classification tests -> 8 passed, 0 xfailed Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HD4ecxVqvgxNP9No91e2Xp
…ariant The existing metric tests compare a Vertica-trained model against a scikit-learn-trained one, which conflates two questions: does VerticaPy compute the metric correctly, and do the two engines build the same model. Only the first is VerticaPy's to get right, and only the first has a tight answer -- the sklearn reference's own seed-to-seed spread exceeds the tolerances it is measured against. test_metric_matches_sklearn_on_same_probabilities feeds both implementations the same probability vector, isolating the metric code. Measured worst relative difference across five deliberately different Vertica models is 2.7e-05, so rel=1e-3 leaves a 37x margin. That tolerance is kept out of rel_abs_tol_map on purpose: it is derived from a measurement rather than fitted to whatever passed. test_predict_matches_argmax_of_predict_proba asserts an exact invariant of the prediction path that nothing else covered. Verified with pinpoint node IDs: 9 passed across RandomForestClassifier, DecisionTreeClassifier and XGBClassifier. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HD4ecxVqvgxNP9No91e2Xp
… quote
read_pandas() silently stripped literal double quotes from string values:
'say "hi"' round-tripped as 'say hi'. It enclosed each string field in '"',
doubled any embedded quote, then post-processed the file with a blanket
.replace('""', ""). That replace served the all-NULL sentinel, but it also
deleted the doubling that escaped a real quote.
Removing the replace on its own is worse, not better: Vertica does not read a
doubled "" as an escaped quote, so '"say ""hi"""' is a parse error and the
whole row is dropped. Silent character loss becomes silent row loss.
The enclosure itself had to move. '"' is now ordinary data, enclosed by a
control character instead, and the post-processing block is gone. The
all-NULL sentinel writes an empty field directly, which is what NULL ''
matches.
Vertica turns out to require an enclosure character appearing inside an
enclosed field to be written as ESCAPE_AS + enclosure -- a raw or a doubled
one gets the row rejected. Measured against 25.3 with an explicit COPY; the
delimited-data docs state only the first-and-last rule and do not cover this
case. to_csv cannot emit that sequence, because under QUOTE_NONE it only ever
writes the escape character ahead of the escape character, the delimiter or
the record terminator, and it doubles any escape character we pre-insert.
So _pick_enclosure() chooses the first of twelve candidate control characters
that occurs in neither the data nor the column names, and nothing needs
escaping at all.
The enclosure exists for one reason: to stop the flex table that guesses the
column types from retyping a string column, where '007' would be read as
INTEGER. Supplying dtype, or inserting into an existing relation, skips that
guess -- and the data then loads correctly with no enclosure whatsoever
(verified for quotes, commas, surrounding whitespace, '007' and escaped
control characters). read_pandas now falls back to that when the enclosure is
exhausted, and only raises otherwise, naming dtype as the way through.
read_csv and pcsv accept quotechar=None to express it.
Also fixes an independent corruption on the insert path, which declared
ESCAPE AS '\' -- a single backslash -- while to_csv writes escapes as \027.
Its escapes were never decoded and a literal backslash was eaten: 'C:\dir'
loaded as 'C:dir'.
One deliberate behaviour change: an empty string used to round-trip as NULL
via the blanket replace, and now stays an empty string. Nothing pinned the
old behaviour. Note the no-enclosure fallback cannot preserve it -- there an
empty string is indistinguishable from an empty field and loads as NULL.
A string column of '007' still types as INTEGER. That is pre-existing,
confirmed identical on HEAD, and inherent to the flex-table type guess rather
than to the encoding.
tests_new/core: 794 passed, 2 skipped -- the 786 baseline plus the 8 tests
added here, which cover the encoding at byte level offline, the round trip on
both the read_csv and COPY paths asserting row count as well as values, the
enclosure picker, and both escape hatches.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011Ri4NJDUgT2kwPtGfU5Ttt
The previous commit picked the CSV enclosure character at run time, scanning the string columns for the first of twelve control characters the data did not contain. That defended against a literal chr(22) in text -- an input that essentially never occurs -- by making every ingest pay a scan, and it hid the collision rather than reporting it. Back to a single fixed ENCLOSED_BY. The collision is now detected when it actually happens, which costs nothing on the path that already knows: COPY returns the number of rows it accepted, so insert=True gets the count for free, and the read_csv path adds one COUNT(*). Fewer rows than the DataFrame holds raises, naming dtype as the way through. read_csv creates the relation before the check can run, so it is dropped again on rejection -- handing back a table quietly short of rows would be the same silent data loss this whole change is about. Both checks are gated on the fields actually being enclosed. Supplying dtype skips the flex-table type guess, which is the only reason to enclose at all, so nothing can collide and the check would be a wasted round trip; worse, a rejection there would be a type mismatch that this message would blame on the enclosure. enclose = not dtype is now the whole rule. tests_new/core: 793 passed, 2 skipped -- the 786 baseline plus the 7 tests here, which cover the encoding at byte level offline, the round trip on both paths, the rejection raising and dropping the table, and dtype carrying a value that holds the enclosure character. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Ri4NJDUgT2kwPtGfU5Ttt
|
|
ran-haim
force-pushed
the
py313-modern-deps
branch
from
August 31, 2026 12:23
416eda7 to
11cf2e8
Compare
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
2 participants
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.
Brings VerticaPy up to Python 3.13 / NumPy 2 / pandas 3 / Matplotlib 3.11, and fixes the correctness bugs that running the live-Vertica suite against those versions exposed.
10 commits, 38 files, +961 / -355.
Python 3.13 and modern dependency floors
cp313 has no wheels for
numpy<2,pandas<2.2,scipy<1.14ormatplotlib<3.9, so the floors rise and the code has to work against those majors.np.float_removed in NumPy 2.0 —TableSample.category()raisedAttributeErroron any float column.Series.ravelremoved in pandas 3.0 — 93 test failures; replaced withnp.ravel(), correct on both 2.2 and 3.0.Axes.boxplot(labels=)removed in Matplotlib 3.11 — every box plot raisedTypeError.nan(truthy), so1 if x else 0androw is not Nonesilently stopped detecting NULLs.python_requires >= 3.10, 3.13 classifier,ipythondeclared (it was imported at package-import time but never listed), version single-sourced from__init__.py, tox/CI matrices cover 3.10–3.13.SyntaxWarningsince 3.12, a futureSyntaxError). All 46,727 string constants diffed before/after — bytewise unchanged.Data-corruption fixes in
read_pandassay "hi"round-tripped assay hi. A blanket.replace('""', "")deleted the doubling that escaped a real quote. The CSV is now enclosed with a control character instead, so"is ordinary data.ESCAPE AS '\'whileto_csvwrites escapes as\027, so escapes were never decoded and a literal backslash was eaten:C:\dirloaded asC:dir.to_csv()wrote\nrecords in text mode, so on Windows the files came out CRLF and COPY mis-parsed them: 528 of 1234 rows survived a round trip.dtypeas the way through, rather than silently dropping rows.One deliberate behaviour change:
''no longer loads as NULLAn empty string used to round-trip as NULL, because the blanket
.replace('""', "")erased it and the NULL representation alike. Dropping that replace — the same removal that stopssay "hi"losing its quotes — lets the two stay distinct.The enclosure is what carries the difference.
COPY … NULL ''matches a field that is empty and unenclosed; an empty string is written as a zero-length enclosed value, which does not match:"text"\x16text\x16'text'""\x16\x16''None/NaNNULLsay "hi"\x16say "hi"\x16say "hi"A
None/NaNnever acquires the enclosure characters in the first place: the value is built asENCLOSED_BY + col.str.slice() + ENCLOSED_BY, and string concatenation with NA propagates NA, so it stays NA andto_csvwrites it as an empty field. All-NULL columns are set to""without enclosure for the same effect, and non-string columns are untouched.This distinction holds only where the fields are enclosed. Passing
dtypesetsenclose = False, and an empty string is then indistinguishable from an empty field and still loads as NULL — with no enclosure there is no byte left to separate "empty" from "absent". Nothing pinned the old behaviour in either direction.Test-harness defects (wrong oracles, not flaky tests)
conftest.pyassigned the same precision-recall trapezoid to bothaucandprc_auc;roc_auc_scoreappeared nowhere in the file, while the VerticaPy side of theaucassertion reads Vertica's ROC AUC. The ~3% gap was masked by a tolerance fitted to it. Present since PR Unit test report tolerance at metric level #1157.XGBInitializersilently dropped xgboost-spelled kwargs, so the reference model trained witheta=0.3instead of0.1. Unrecognized kwargs are now rejected rather than ignored.tree_method="exact", a different split-finding algorithm from Vertica's, which also ignoredmax_binentirely. Mappedsplit_proposal_method='global'→tree_method='approx'; gap −5.04% → −4.08%.remove_model_dir()was a silent no-op on Windows (test -dresolved to Git's MSYStest.exe;sudo rm -rfdoesn't exist), so cleanup never ran andEXPORT_MODELSrefused to overwrite — 137 failures, matching the arithmetic exactly.rolling().sem(ddof=0)compensated for a pandas 2 bug that 3.0 fixed; the test had been checking correct SQL against a wrong oracle.No tolerance was widened anywhere. Six assertions that compare a Vertica-trained model against a scikit-learn-trained one are marked
xfail(strict=True)— the sklearn reference's own seed-to-seed spread (1.89% against a 0.40% tolerance) means no tolerance value is reachable. Loosening one is what hid the auc defect for two years.New tests isolate what is actually VerticaPy's to get right:
test_metric_matches_sklearn_on_same_probabilitiesfeeds both implementations the same probability vector (worst relative difference 2.7e-05 against a 1e-3 tolerance), andtest_predict_matches_argmax_of_predict_probapins an invariant nothing else covered.Other
vDataFrame.corr(focus=...)sorted byabs(...), and a NaN correlation (any all-NULL column) compares False against everything, so Timsort emitted an arbitrary order — the titanic plot was genuinely mis-ranked, withparch(r=.087) shown as less correlated thanage(r=.042).CLAUDE.mddocuments how to run the live-Vertica suite in Docker, which is no longer obvious: Community Edition was pulled from Docker Hub and is unsupported from Vertica 26.1, so CI'sopentext/vertica-cepull cannot succeed.Test results (live Vertica 25.3, Python 3.13, NumPy 2.5, pandas 3.0)
tests_new/coretests_new/plottingtests_new/performancetest_tree_model.pytest_model_management.pymachine_learning(2,084 tests) was run in full — 1,559 passed, 231 failed, 294 skipped (2h28m). That run is the source of the 231 failures diagnosed above, which resolved into three causes: 137 fromremove_model_dirbeing a silent no-op on Windows, 93 fromSeries.ravel, and a handful of pre-existing metric disagreements.After the fixes, the two affected files were re-run:
test_model_management.py137 failed → 163 passed, 12 skipped, 0 failed, andtest_tree_model.py93 failed → 317 passed, 4 failed, 26 skipped. Those last 4, plus 2 intest_base_model_methods.py, are the assertions subsequently markedxfail(strict=True).A whole-suite re-run with every fix in place has not been captured, so the numbers above are the per-file re-runs rather than a single final tally.
🤖 Generated with Claude Code
https://claude.ai/code/session_018dGoxha5DN2QzfZV8HbLkb