Bump scikit-learn minimum to 1.6 - #8091
Conversation
csadorf
added
improvement
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
github-actions
Bot
added
conda
cdd3245 to
61eded8
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
e5f5323 to
777b6f8
Compare
This comment has been minimized.
This comment has been minimized.
777b6f8 to
8358f6f
Compare
This comment has been minimized.
This comment has been minimized.
8358f6f to
7fd18dc
Compare
This comment has been minimized.
This comment has been minimized.
The README compatibility section and pytest_plugin docstrings still referenced scikit-learn 1.4/1.5. Update them to reflect the new minimum version of 1.6. Files updated: - README.md: compatible with scikit-learn version 1.4 → 1.6 - pytest_plugin.py: example conditions updated to >=1.6
7fd18dc to
522b7de
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
This is essentially ready for merge. I'm looking into ripping out the old tag infrastructure as part of this PR. If that turns out to be too tedious, we can split it off into a follow-up. |
We can handle the tags infrastructure in a follow-up: #8228 |
|
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)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR raises cuML's minimum scikit-learn requirement to 1.6 across configs, adds a suppression context for sklearn 1.6 legacy-tag warnings (used for check_is_fitted and tag lookups), and updates pytest filters and test xfail conditions for sklearn 1.6. Changesscikit-learn 1.6 compatibility upgrade
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/cuml/internals/validation.py`:
- Around line 67-76: The wrapper _suppress_sklearn_legacy_tag_warnings currently
only ignores FutureWarning; update it to also ignore DeprecationWarning so
sklearn 1.6/1.7 legacy-tag deprecation messages are captured. In the
contextmanager that iterates _SKLEARN_LEGACY_TAG_WARNING_MESSAGES, change the
warnings.filterwarnings call to target both FutureWarning and DeprecationWarning
(e.g., pass a tuple of exception classes or apply a second filter) so both
warning types are suppressed for the listed messages.
🪄 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: 20400484-dfe4-427b-ba2f-821df8884cec
📒 Files selected for processing (13)
README.mdconda/environments/all_cuda-129_arch-aarch64.yamlconda/environments/all_cuda-129_arch-x86_64.yamlconda/environments/all_cuda-132_arch-aarch64.yamlconda/environments/all_cuda-132_arch-x86_64.yamlconda/recipes/cuml/recipe.yamldependencies.yamlpython/cuml/cuml/accel/pytest_plugin.pypython/cuml/cuml/explainer/common.pypython/cuml/cuml/internals/validation.pypython/cuml/cuml_accel_tests/upstream/pytest.inipython/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yamlpython/cuml/pyproject.toml
| @contextmanager | ||
| def _suppress_sklearn_legacy_tag_warnings(): | ||
| with warnings.catch_warnings(): | ||
| for message in _SKLEARN_LEGACY_TAG_WARNING_MESSAGES: | ||
| warnings.filterwarnings( | ||
| "ignore", | ||
| message=message, | ||
| category=FutureWarning, | ||
| ) | ||
| yield |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In scikit-learn 1.6 and 1.7 transition docs/changelog, are legacy tag warnings (_get_tags/_more_tags) always emitted as FutureWarning, or can they be DeprecationWarning depending on code path?
💡 Result:
In scikit-learn 1.6 and 1.7, warnings related to legacy tag methods (_get_tags, _more_tags, and _safe_tags) are consistently emitted as DeprecationWarning [1][2]. Initially, these warnings were introduced as FutureWarning, but they were explicitly changed to DeprecationWarning in scikit-learn 1.6 to ensure that the notifications are targeted at developers rather than end-users [1][3]. This change was implemented via PR #30573 to prevent end-users from being exposed to library-level deprecation messages that they generally cannot fix [3]. Consequently, there is no code path in scikit-learn 1.6 or 1.7 where these specific legacy tag methods continue to emit a FutureWarning; they are intended to be DeprecationWarning across all relevant paths [1][2]. As of scikit-learn 1.7, the old tag infrastructure was subject to further cleanup [4].
Citations:
- 1: https://sklearn.org/1.7/whats_new/v1.6.html
- 2: https://github.com/scikit-learn/scikit-learn/blob/c7d040e4/doc/whats_new/v1.6.rst
- 3: FIX change FutureWarnings to DeprecationWarnings for the tags scikit-learn/scikit-learn#30573
- 4: MNT Clean-up deprecations for 1.7: old tags scikit-learn/scikit-learn#31134
Suppress sklearn legacy-tag deprecation warnings for both DeprecationWarning and FutureWarning
In python/cuml/cuml/internals/validation.py (lines 67-76), _suppress_sklearn_legacy_tag_warnings() filters only FutureWarning, but sklearn 1.6/1.7 legacy tag warnings are emitted as DeprecationWarning, so this wrapper can miss the messages.
Suggested patch
`@contextmanager`
def _suppress_sklearn_legacy_tag_warnings():
with warnings.catch_warnings():
- for message in _SKLEARN_LEGACY_TAG_WARNING_MESSAGES:
- warnings.filterwarnings(
- "ignore",
- message=message,
- category=FutureWarning,
- )
+ for message in _SKLEARN_LEGACY_TAG_WARNING_MESSAGES:
+ for category in (FutureWarning, DeprecationWarning):
+ warnings.filterwarnings(
+ "ignore",
+ message=message,
+ category=category,
+ )
yield📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @contextmanager | |
| def _suppress_sklearn_legacy_tag_warnings(): | |
| with warnings.catch_warnings(): | |
| for message in _SKLEARN_LEGACY_TAG_WARNING_MESSAGES: | |
| warnings.filterwarnings( | |
| "ignore", | |
| message=message, | |
| category=FutureWarning, | |
| ) | |
| yield | |
| `@contextmanager` | |
| def _suppress_sklearn_legacy_tag_warnings(): | |
| with warnings.catch_warnings(): | |
| for message in _SKLEARN_LEGACY_TAG_WARNING_MESSAGES: | |
| for category in (FutureWarning, DeprecationWarning): | |
| warnings.filterwarnings( | |
| "ignore", | |
| message=message, | |
| category=category, | |
| ) | |
| yield |
🤖 Prompt for 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.
In `@python/cuml/cuml/internals/validation.py` around lines 67 - 76, The wrapper
_suppress_sklearn_legacy_tag_warnings currently only ignores FutureWarning;
update it to also ignore DeprecationWarning so sklearn 1.6/1.7 legacy-tag
deprecation messages are captured. In the contextmanager that iterates
_SKLEARN_LEGACY_TAG_WARNING_MESSAGES, change the warnings.filterwarnings call to
target both FutureWarning and DeprecationWarning (e.g., pass a tuple of
exception classes or apply a second filter) so both warning types are suppressed
for the listed messages.
jcrist
left a comment
jcrist
left a comment
There was a problem hiding this comment.
Approving, but with one comment and one question. Thanks for working on this!
|
|
||
| def check_is_fitted(*args, **kwargs): | ||
| with _suppress_sklearn_legacy_tag_warnings(): | ||
| return _sklearn_check_is_fitted(*args, **kwargs) |
There was a problem hiding this comment.
Can you comment on why we need to suppress the warnings here? I install sklearn 1.6 and can't seem to trigger an warning in check_is_fitted when called on our models.
IIUC this is a stop-gap in this PR that's then removed in #8227, so this code shouldn't be long lived. Mostly just curious why this showed up here and in what conditions.
There was a problem hiding this comment.
Not sure why it doesn't trigger for you, but this code will emit the warning for 26.06 nightlies in combination with sklearn version 1.6:
from cuml.datasets import make_regression
from cuml.linear_model import LinearRegression
X, y = make_regression(n_samples=20, n_features=5, random_state=0)
LinearRegression().fit(X, y).predict(X)
import sklearn; assert sklearn.__version__ == "1.6.0"This calls check_is_fitted() as part of the predict() call, and it can also be triggered by calling check_is_fitted() explicitly.
Either way, this is a very temporary compatibility shim. We can immediately remove it in the follow-up.
There was a problem hiding this comment.
I suspect we just had not explicitly tested with sklearn 1.6.0 previously which is probably why this hadn't come up before.
|
|
||
| * **NumPy**: >=1.23,<3.0a0 | ||
| * **scikit-learn**: >=1.5 | ||
| * **scikit-learn**: >=1.6 |
There was a problem hiding this comment.
Versions are also noted in the cuml-accel docs: https://github.com/rapidsai/cuml/blob/af109cb7e2bc8e62906d533aa098af8ae1474fe3/docs/source/cuml-accel/limitations.rst?plain=1#L50-L52
- If you want to merge this PR as is and add the fixup in Update cuML sklearn tags for scikit-learn 1.6 #8227 that's fine
- I wonder if we could make the doc in
cuml-accel/limitations.rstpoint tosupported_versions.rstto avoid having two places we need to check this? What we have now is also fine, no strong thoughts.
There was a problem hiding this comment.
Let's keep it separate for now and I'll fix this in the follow-up to avoid a CI cycle.
|
/merge |
4 participants
Bumps scikit-learn minimum dependency from 1.5 to 1.6 across all environments, recipes, and pyproject.toml. Also updates the README and pytest_plugin docstrings to reflect the new version.
Closes #8098