Skip to content

Add flag to enable cudf.pandas with CLI#6364

Closed
dantegd wants to merge 2 commits intorapidsai:branch-25.04from
dantegd:cuml-accel-cudf-pandas
Closed

Add flag to enable cudf.pandas with CLI#6364
dantegd wants to merge 2 commits intorapidsai:branch-25.04from
dantegd:cuml-accel-cudf-pandas

Conversation

@dantegd
Copy link
Copy Markdown
Member

@dantegd dantegd commented Feb 25, 2025

Adds flag to enable cudf.pandas to experimental CLI UX

@dantegd dantegd added feature request New feature or request non-breaking Non-breaking change cuml-cpu labels Feb 25, 2025
@dantegd dantegd requested review from a team as code owners February 25, 2025 04:52
@github-actions github-actions Bot added Cython / Python Cython or Python issue ci labels Feb 25, 2025
dantegd added a commit to dantegd/cuml that referenced this pull request Feb 25, 2025
# Support invoking run_cuml_singlegpu_pytests.sh outside the script directory
cd "$(dirname "$(realpath "${BASH_SOURCE[0]}")")"/../python/cuml/cuml/tests

python -m pytest -p cudf.pandas -p cuml.experimental.accel --cache-clear "$@" experimental/accel/
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For the cuml.accel tests in run_cuml_singlegpu_accel_pytests.sh we cd into the experimental/accel directory. Can we keep it consistent? Not sure but maybe it effects the paths we see in the logs?

Comment thread ci/test_python_integration.sh
Copy link
Copy Markdown
Member

@betatim betatim left a comment

Choose a reason for hiding this comment

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

I think for now this is good, but we should look at how we do CI and testing more holistically.

@dantegd
Copy link
Copy Markdown
Member Author

dantegd commented Feb 25, 2025

/merge

dantegd added a commit to dantegd/cuml that referenced this pull request Feb 26, 2025
raydouglass pushed a commit that referenced this pull request Feb 28, 2025
PRs being backported: 

- [x] #6234
- [x] #6306
- [x] #6320
- [x] #6319
- [x] #6327
- [x] #6333
- [x] #6142 
- [x] #6223
- [x] #6235
- [x] #6317 
- [x] #6331
- [x] #6326
- [x] #6332
- [x] #6347
- [x] #6348
- [x] #6337
- [x] #6355
- [x] #6354
- [x] #6322
- [x] #6353
- [x] #6359
- [x] #6364
- [x] #6363
- [x] [FIL BATCH_TREE_REORG fix for SM90, 100 and
120](a3e419a)

---------

Co-authored-by: William Hicks <whicks@nvidia.com>
@jcrist
Copy link
Copy Markdown
Member

jcrist commented May 29, 2025

Just like how we can stack accelerators in pytest with py.test -p cuml.accel -p cudf.pandas ..., I think a nicer way of handling this would be to support stacking accelerators with python -m cuml.accel -m cudf.pandas ....

Likewise, the recommended way of loading both accelerators in a notebook is to load both extensions (in either order)

%load_ext cuml.accel
%load_ext cudf.pandas

With some recent changes, this almost works as intended today. Unfortunately there's a bug in the CLI where stacking only works if cudf.pandas comes before cuml.accel.

# Fails
python -m cuml.accel -m cudf.pandas myscript.py

# Succeeds
python -m cudf.pandas -m cuml.accel myscript.py

I'll push a fix up for that, but think we should drop this PR since a custom flag doesn't compose nearly as well. Closing as stale and (IMO) no longer desired. If you disagree feel free to reopen.

@jcrist jcrist closed this May 29, 2025
@jcrist
Copy link
Copy Markdown
Member

jcrist commented May 29, 2025

Gah, just noticed this was merged into the backport but never actually merged into dev. Then after the backport was forward merged into the new branch this was added back into dev but not through this PR. Shenanigans.

Anyway, I'll still push the fix up I mentioned above, and might advocate for deprecating the flag in favor of composing -m flags instead.

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

Labels

ci Cython / Python Cython or Python issue feature request New feature or request non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants