Fix Normalizer parameter cloning in ColumnTransformer - #8580
sylvesterkaczmarek wants to merge 1 commit into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: NVIDIA/cuml/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughNormalizer now reports its ChangesNormalizer validation
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to 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)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I know it looks like that, but the "Recently Updated" check is actually not mandatory for merge. Please avoid merging |
|
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. |
This comment has been minimized.
This comment has been minimized.
6dbde2f to
31781ea
Compare
|
@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? |
|
/ok to test 31781ea |
|
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? |
Fixes #8577.
Normalizernow exposes its constructor parameters through cuML's_get_param_names, sosklearn.clone()preservesnormandcopy. This preventsColumnTransformerfrom silently rebuildingNormalizer(norm="l1")with the defaultl2norm.Regression coverage checks both direct sklearn cloning and the reported two-branch
ColumnTransformercase against sklearn.Validation:
pre-commit run --files python/cuml/cuml/_thirdparty/sklearn/preprocessing/_data.py python/cuml/tests/test_compose.pypassespython3 -m py_compileon both changed files passesgit diff --checkpassesThe focused pytest collection cannot run on this macOS host because the cuML test configuration requires
cudf; NVIDIA CI provides the RAPIDS/CUDA test environment.