Large (int64 indices) sparse input support - #8196
Conversation
Rather than relying on the cupy error, we now error informing users that their version of cupy is too old.
No need to branch here, these columns will always need a copy so `copy=False` is useless.
Testing these is infeasible in CI, but I've locally verified things work. We're mostly plumbing inputs -> cupy -> cublas/cusolver/cublas, so I don't personally feel lack of testing here is a merge blocker.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds an int64-sparse detector and CuPy >=14.1.0 gate to validation, opts estimators into large-sparse acceptance, adds version-gated tests, and simplifies float16 upcasting in cudf handling. ChangesLarge Sparse Matrix Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/cuml/tests/test_validation.py`:
- Around line 1133-1135: The pytest.raises call currently uses match="Sparse
matrices with int64 indices require cupy >= 14.1.0" which is treated as a regex;
escape the literal dots so the version string is matched literally (e.g.,
replace "." with "\." in the version portion) in the pytest.raises match
argument within the test that contains this raise check to ensure the assertion
matches the exact message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b4e7cb6b-7aeb-414f-bb25-2b9464a559e3
📒 Files selected for processing (5)
python/cuml/cuml/internals/validation.pypython/cuml/cuml/linear_model/linear_regression.pyxpython/cuml/cuml/linear_model/ridge.pyxpython/cuml/cuml/random_projection/random_projection.pypython/cuml/tests/test_validation.py
csadorf
left a comment
There was a problem hiding this comment.
Great! Just two minor things.
|
/merge |
This:
cuml.internals.validationto provide a better error message ifcupy < 14.1.0when users pass in large sparse matrices on host, noting the cupy version that's required to support this.accept_large_sparse=TrueforRidge,LinearRegression, andcuml.random_projection. I've tested that these work locally, but running these requires more memory than we can feasibly rely on in CI. The approach used in sklearn (constructing a tiny sparse matrix, but forcing int64 indices) isn't feasible here, sincecupyrightfully tries to optimistically coerce indices toint32values, which would defeat the purpose of testing. Sinceint32indices run much faster, I think this validation check is valid and not something worth working around. FWIW, these estimators are mostly plumbing the inputs -> routines in cupy/cublas/cusparse/cusolver, so I don't think missing tests in CI here should be a release blocker.Fixes #8150.
Fixes #8151.
Fixes #8152.
Fixes #8157.