Skip to content

Fix Normalizer parameter cloning in ColumnTransformer - #8580

Open
sylvesterkaczmarek wants to merge 1 commit into
NVIDIA:mainfrom
sylvesterkaczmarek:bug-normalizer-clone-params
Open

sylvesterkaczmarek wants to merge 1 commit into
NVIDIA:mainfrom
sylvesterkaczmarek:bug-normalizer-clone-params

Conversation

@sylvesterkaczmarek

Copy link
Copy Markdown

Fixes #8577.

Normalizer now exposes its constructor parameters through cuML's _get_param_names, so sklearn.clone() preserves norm and copy. This prevents ColumnTransformer from silently rebuilding Normalizer(norm="l1") with the default l2 norm.

Regression coverage checks both direct sklearn cloning and the reported two-branch ColumnTransformer case against sklearn.

Validation:

  • pre-commit run --files python/cuml/cuml/_thirdparty/sklearn/preprocessing/_data.py python/cuml/tests/test_compose.py passes
  • python3 -m py_compile on both changed files passes
  • git diff --check passes

The focused pytest collection cannot run on this macOS host because the cuML test configuration requires cudf; NVIDIA CI provides the RAPIDS/CUDA test environment.

@sylvesterkaczmarek
sylvesterkaczmarek requested a review from a team as a code owner September 9, 2026 08:51
@copy-pr-bot

copy-pr-bot Bot commented Sep 9, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions Bot added the Cython / Python Cython or Python issue label Sep 9, 2026
@coderabbitai

coderabbitai Bot commented Sep 9, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: NVIDIA/cuml/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 38bb7777-a9af-4311-8998-8210c2b3b5b4

📥 Commits

Reviewing files that changed from the base of the PR and between 68c1abb and 6dbde2f.

📒 Files selected for processing (1)
  • python/cuml/cuml/_thirdparty/sklearn/preprocessing/_data.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.


📝 Summary

Summary by CodeRabbit

  • Bug Fixes

    • Improved compatibility with scikit-learn cloning by preserving normalizer parameters.
    • Ensured column transformations retain each normalizer’s selected normalization setting.
  • Tests

    • Added coverage confirming normalizer settings are preserved when cloning normalizers and using them in column transformations.

Walkthrough

Normalizer now reports its norm and copy parameters through parameter introspection. Tests cover cloning and compare the output of multiple L1 normalizers in ColumnTransformer with scikit-learn.

Changes

Normalizer validation

Layer / File(s) Summary
Expose Normalizer parameters
python/cuml/cuml/_thirdparty/sklearn/preprocessing/_data.py
Normalizer._get_param_names adds norm and copy to the names returned by its superclass.
Validate cloning and column transformation
python/cuml/tests/test_compose.py
Tests check that sklearn cloning preserves norm and copy. A parity test compares multiple L1 normalizers in ColumnTransformer with scikit-learn.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: csadorf

Merge Risk: ⚪ Minimal · up to 6dbde

The parameter-preservation change is supported by the estimator contract and regression tests for cloning and column transformation. No concrete merge-blocking risk is established.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: fixing Normalizer parameter cloning in ColumnTransformer.
Description check ✅ Passed The description explains the problem, implementation choice, regression coverage, issue reference, and validation results. It also documents the pytest limitation on the macOS host.
Linked Issues check ✅ Passed Issue #8577 requires Normalizer(norm="l1") to survive cloning inside ColumnTransformer for integer-index and slice selections. The PR adds Normalizer._get_param_names with norm and copy, and…
Out of Scope Changes check ✅ Passed The reviewed changes modify Normalizer parameter discovery and add tests for cloning and ColumnTransformer normalization. These changes directly support issue #8577. The current whole-PR summary s…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

@csadorf csadorf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@csadorf

This comment has been minimized.

@csadorf

csadorf commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

The CI on this PR is currently failing due to unrelated blockers documented in #8576, #8583, and #8584.

@csadorf

This comment has been minimized.

@sylvesterkaczmarek

Copy link
Copy Markdown
Author

@csadorf I refreshed the approved branch onto current main. The new head is 68c1abb; it is now 0 commits behind, the Recently Updated gate is green, and your approval remains intact. Could you please /ok to test 68c1abb so NVIDIA runner CI can validate the refreshed head?

@csadorf

This comment has been minimized.

@csadorf

csadorf commented Sep 29, 2026

Copy link
Copy Markdown
Contributor

@csadorf I refreshed the approved branch onto current main. The new head is 68c1abb; it is now 0 commits behind, the Recently Updated gate is green, and your approval remains intact. Could you please /ok to test 68c1abb so NVIDIA runner CI can validate the refreshed head?

I know it looks like that, but the "Recently Updated" check is actually not mandatory for merge. Please avoid merging main unless there are merge conflicts to spare CI resources. Thank you!

@sylvesterkaczmarek

Copy link
Copy Markdown
Author

Understood, thanks for clarifying. I will avoid merging main into this branch unless there is an actual merge conflict. I will leave the current approved head as-is rather than rewriting it again while the runner results are available.

@csadorf

This comment has been minimized.

@sylvesterkaczmarek
sylvesterkaczmarek force-pushed the bug-normalizer-clone-params branch from 6dbde2f to 31781ea Compare October 1, 2026 08:06
@sylvesterkaczmarek

Copy link
Copy Markdown
Author

@csadorf I accidentally included this approved PR in today's broader NVIDIA branch-refresh pass despite your earlier note not to refresh unless there is a merge conflict. Sorry about that. The rewritten head is 31781ea, is 0 behind, remains approved and mergeable, and currently has no failing checks. I will not rewrite it again. If NVIDIA-runner validation is required for this new head, could you please /ok to test 31781ea?

@csadorf

csadorf commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

/ok to test 31781ea

@sylvesterkaczmarek

Copy link
Copy Markdown
Author

The fresh /ok to test run is red, but I do not see a contributor-side failure from this PR. This branch changes only the Normalizer cloning Python/test path; the failed build jobs are stopping in unrelated C++ infrastructure, primarily CCCL's new cub/cub.cuh umbrella-header warning being promoted to -Werror across many existing C++ targets. The devcontainer leg also reports the expected cache image rapidsai/cuml-devcontainer:cuda13.3-conda as missing.

Given the existing approval, I am leaving the branch unchanged. Could the NVIDIA CI be rerun once the upstream build/container issue is cleared?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Cython / Python Cython or Python issue non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] ColumnTransformer with Normalizer(norm="l1") produces L2-normalized values

3 participants