Skip to content

Support coercing host arrays in mlfunc/ReflectedAttr - #8373

Merged
rapids-bot[bot] merged 4 commits into
NVIDIA:mainfrom
jcrist:support-host-arrays-mlfunc-reflectedattr
Jul 14, 2026
Merged

rapids-bot[bot] merged 4 commits into
NVIDIA:mainfrom
jcrist:support-host-arrays-mlfunc-reflectedattr

Conversation

@jcrist

@jcrist jcrist commented Jul 13, 2026

Copy link
Copy Markdown
Member

In #8339 we added mlfunc and ReflectedAttr as new output coercion mechanisms. As part of that, we only implemented coercion paths from device matrices (cupy.ndarray/cupyx.scipy.sparse.spmatrix) since cuml generally does all computations on device and any conversion to host will happen later.

However, while porting cuml.fil I realized there are a few cases (cuml.fil included) where we natively output host memory but may still want to coerce it to device later as part of reflection.

This PR:

  • Adds some code to handle numpy.ndarray/scipy.sparse.spmatrix as source arrays for coercion in mlfunc/ReflectedAttr
  • Adds tests for the same
  • Ports cuml.fil to use mlfunc/ReflectedAttr instead of the legacy equivalents
  • Also addresses a few suggestions for added comments leftover from Unify reflection system & decorators #8339.

Followup to #8339.
Part of #8177.

@jcrist jcrist self-assigned this Jul 13, 2026
@jcrist
jcrist requested a review from a team as a code owner July 13, 2026 19:28
@jcrist jcrist added the improvement Improvement / enhancement to an existing function label Jul 13, 2026
@jcrist
jcrist requested a review from divyegala July 13, 2026 19:28
@jcrist jcrist added the non-breaking Non-breaking change label Jul 13, 2026
@csadorf

csadorf commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Am I correct to interpret this as a must-fix follow-up?

@github-actions github-actions Bot added the Cython / Python Cython or Python issue label Jul 13, 2026
@jcrist

jcrist commented Jul 13, 2026

Copy link
Copy Markdown
Member Author

No, this is required to continue porting to the new system, but is not a bug in anything that's already been ported.

@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 23006302-09cc-4a2b-84e8-b987e46d4dd7

📥 Commits

Reviewing files that changed from the base of the PR and between e751c15 and d24ab14.

📒 Files selected for processing (4)
  • python/cuml/cuml/__init__.py
  • python/cuml/cuml/fil/compat.py
  • python/cuml/cuml/internals/outputs.py
  • python/cuml/tests/test_reflection.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • python/cuml/cuml/init.py
  • python/cuml/cuml/fil/compat.py

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes
    • Improved FIL inference compatibility across CPU/GPU by adjusting how outputs are produced for different memory types.
    • Enhanced convert_arrays handling for dense and sparse data, including NumPy, pandas, SciPy, and CuPy sparse—preserving requested sparse formats.
    • Improved validation and error messaging for unsupported return types.
  • Tests
    • Expanded conversion tests across backends (NumPy/CuPy/SciPy/cupyx), memory orders, sparse formats, and indexed pandas conversions.
  • Documentation
    • Clarified array conversion behavior and updated an internal workaround note.
  • Chores
    • Refreshed copyright header text.

Walkthrough

Changes

FIL output compatibility

Layer / File(s) Summary
Array conversion and reflection handling
python/cuml/cuml/internals/outputs.py, python/cuml/tests/test_reflection.py
convert_arrays now handles NumPy and sparse inputs explicitly, reflection checks cover additional array types, and tests cover input backends, sparse formats, memory orders, and indexes.
nvforest inference output routing
python/cuml/cuml/fil/compat.py
FIL inference uses mlfunc, detects CPU or GPU-backed models, selects host or device preprocessing, and returns NumPy or CuPy arrays based on FIL memory settings.
CuPy setup maintenance
python/cuml/cuml/__init__.py
The copyright declaration and CuPy workaround comment were updated.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: divyegala

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: adding host-array coercion support for mlfunc and ReflectedAttr.
Description check ✅ Passed The description matches the change set by describing host-array coercion, tests, FIL porting, and follow-up comments.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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_reflection.py`:
- Around line 349-375: Rename the test_convert_arrays_sparse_array parameter
format to sparse_format, and update all references in the parametrization,
sparse.random calls, and output format assertion while preserving the existing
test behavior.
🪄 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: b42916a0-f6d9-4113-8a50-9e539a2e3da1

📥 Commits

Reviewing files that changed from the base of the PR and between 89b5804 and e751c15.

📒 Files selected for processing (4)
  • python/cuml/cuml/__init__.py
  • python/cuml/cuml/fil/compat.py
  • python/cuml/cuml/internals/outputs.py
  • python/cuml/tests/test_reflection.py

Comment thread python/cuml/tests/test_reflection.py
@jcrist
jcrist force-pushed the support-host-arrays-mlfunc-reflectedattr branch from e751c15 to d24ab14 Compare July 13, 2026 19:40

@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.

LGTM, just one non-blocking question.

Comment thread python/cuml/cuml/internals/outputs.py
@jcrist

jcrist commented Jul 14, 2026

Copy link
Copy Markdown
Member Author

/merge

@rapids-bot
rapids-bot Bot merged commit f5dba10 into NVIDIA:main Jul 14, 2026
93 of 94 checks passed
@jcrist
jcrist deleted the support-host-arrays-mlfunc-reflectedattr branch July 14, 2026 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants