Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Improvement] Reorganize Cython to separate C++ bindings and make Cython classes public #1676

Merged
merged 19 commits into from
Oct 3, 2024
Merged

[Improvement] Reorganize Cython to separate C++ bindings and make Cython classes public #1676

merged 19 commits into from
Oct 3, 2024

Conversation

Matt711
Copy link
Contributor

@Matt711 Matt711 commented Sep 10, 2024 •
edited
Loading

Description

Closes #1280

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@github-actions github-actions bot added CMake Python Related to RMM Python API labels Sep 10, 2024
@Matt711 Matt711 changed the title [WIP] Reorganize Cython to separate C++ bindings and make Cython clas… [WIP] Reorganize Cython to separate C++ bindings and make Cython classes public Sep 10, 2024
@Matt711 Matt711 added breaking Breaking change 2 - In Progress Currently a work in progress improvement Improvement / enhancement to an existing function labels Sep 11, 2024
@Matt711
Copy link
Contributor Author

Matt711 commented Sep 11, 2024

TODO: Return deprecation warnings for rmm._lib. Update the docs as needed.

Matt711
Matt711 commented Sep 11, 2024
View reviewed changes
python/rmm/CMakeLists.txt Outdated Show resolved Hide resolved
@Matt711 Matt711 self-assigned this Sep 12, 2024
@Matt711 Matt711 marked this pull request as ready for review September 16, 2024 18:31
@Matt711 Matt711 requested review from a team as code owners September 16, 2024 18:31
@Matt711 Matt711 changed the title [WIP] Reorganize Cython to separate C++ bindings and make Cython classes public [Improvement] Reorganize Cython to separate C++ bindings and make Cython classes public Sep 16, 2024
harrism
harrism reviewed Sep 16, 2024
View reviewed changes
Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

Helpful refactoring! Just wanted to weigh in on naming.

python/rmm/rmm/allocators/cupy.py Outdated Show resolved Hide resolved
@Matt711 Matt711 requested a review from wence- September 17, 2024 14:58
@Matt711 Matt711 removed the 2 - In Progress Currently a work in progress label Sep 19, 2024
wence-
wence- approved these changes Sep 30, 2024
View reviewed changes
Copy link
Contributor

@wence- wence- left a comment

Choose a reason for hiding this comment

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

Thanks Matt

python/rmm/CMakeLists.txt Show resolved Hide resolved
python/rmm/CMakeLists.txt Show resolved Hide resolved
@wence- wence- removed the 5 - DO NOT MERGE Hold off on merging; see PR for details label Sep 30, 2024
@Matt711 Matt711 added 5 - DO NOT MERGE Hold off on merging; see PR for details and removed 3 - Ready for review Ready for review by team labels Oct 1, 2024
@Matt711
Copy link
Contributor Author

Matt711 commented Oct 1, 2024

I added the the DO NOT MERGE label, to do one more quick round of testing in the unified devcontainer tomorrow morning.

@Matt711
Copy link
Contributor Author

Matt711 commented Oct 1, 2024

I retested the changes in the unified dev container. I was only able to successfully test cudf, rmm, and ucxx. cugraph, raft, and cuml returned segfaults when I ran pytest (a problem in the build probably?). Running a clean build takes too long, so I won't run it again. But I think we're safe because the other libraries tests ran successfully.

@Matt711 Matt711 added 3 - Ready for review Ready for review by team and removed 5 - DO NOT MERGE Hold off on merging; see PR for details labels Oct 1, 2024
@Matt711 Matt711 requested review from vyasr and harrism October 1, 2024 19:19
@vyasr
Copy link
Contributor

vyasr commented Oct 2, 2024

I plan to review this tomorrow, please ping me if I don't get around to it.

vyasr
vyasr approved these changes Oct 2, 2024
View reviewed changes
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Couple of small questions, but I think this can move forward largely as is.

python/rmm/docs/guide.md Show resolved Hide resolved
python/rmm/rmm/__init__.py Show resolved Hide resolved
python/rmm/rmm/_lib/logger.pxd Outdated Show resolved Hide resolved
python/rmm/rmm/librmm/_logger.pxd Outdated Show resolved Hide resolved
@Matt711
Copy link
Contributor Author

Matt711 commented Oct 3, 2024

/merge

@rapids-bot rapids-bot bot merged commit 6489bb7 into rapidsai:branch-24.12 Oct 3, 2024
57 checks passed
@Matt711 Matt711 mentioned this pull request Oct 4, 2024
Merged
3 tasks
rapids-bot bot pushed a commit that referenced this pull request Oct 4, 2024
@Matt711
Fix rmm ._lib imports (#1693)
8150032 
This PR fixes a bug in #1676. It makes sure that rmm imports work correctly using both `from rmm._lib...` and `import rmm._lib...` syntax.

I'm adding DO NOT MERGE until I do some more testing.

Authors:
  - Matthew Murray (https://github.com/Matt711)

Approvers:
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: #1693
rapids-bot bot pushed a commit to rapidsai/ucxx that referenced this pull request Oct 7, 2024
@Matt711
Update RMM imports to use pylibrmm (#283)
0d42882 
This PR updates the RMM import to use `pylibrmm` now that `rmm._lib` is deprecated . It should be merged after rapidsai/rmm#1676.

Authors:
  - Matthew Murray (https://github.com/Matt711)

Approvers:
  - Peter Andreas Entschev (https://github.com/pentschev)

URL: #283
rapids-bot bot pushed a commit to rapidsai/cuml that referenced this pull request Oct 8, 2024
@Matt711
Update all rmm imports to use pylibrmm/librmm (#6084)
61f85a6 
This PR updates all the RMM imports to use pylibrmm/librmm now that `rmm._lib` is deprecated . It should be merged after [rmm/1676](rapidsai/rmm#1676).

Authors:
  - Matthew Murray (https://github.com/Matt711)

Approvers:
  - Dante Gama Dessavre (https://github.com/dantegd)

URL: #6084
rapids-bot bot pushed a commit to rapidsai/raft that referenced this pull request Oct 8, 2024
@Matt711
Update all rmm imports to use pylibrmm/librmm (#2451)
b7d0e98 
This PR updates all the RMM imports to use pylibrmm/librmm now that `rmm._lib` is deprecated . It should be merged after [rmm/1676](rapidsai/rmm#1676).

Authors:
  - Matthew Murray (https://github.com/Matt711)

Approvers:
  - Ben Frederickson (https://github.com/benfred)

URL: #2451
@charlesbluca charlesbluca mentioned this pull request Oct 8, 2024
Merged
3 tasks
rapids-bot bot pushed a commit to rapidsai/cugraph that referenced this pull request Oct 9, 2024
@Matt711
Update all rmm imports to use pylibrmm/librmm (#4671)
6f4689a 
This PR updates all the RMM imports to use pylibrmm/librmm now that `rmm._lib` is deprecated . It should be merged after [rmm/1676](rapidsai/rmm#1676).

Authors:
  - Matthew Murray (https://github.com/Matt711)

Approvers:
  - Brad Rees (https://github.com/BradReesWork)
  - Alex Barghi (https://github.com/alexbarghi-nv)

URL: #4671
rapids-bot bot pushed a commit that referenced this pull request Oct 9, 2024
@charlesbluca
Fix typos in .gitignore (#1697)
90a5631 
Small fix to some typos that cropped up in the .gitignore with #1676

Authors:
  - Charles Blackmon-Luca (https://github.com/charlesbluca)
  - Lawrence Mitchell (https://github.com/wence-)

Approvers:
  - Lawrence Mitchell (https://github.com/wence-)
  - Mark Harris (https://github.com/harrism)

URL: #1697
rapids-bot bot pushed a commit to rapidsai/cudf that referenced this pull request Oct 10, 2024
@Matt711
Update all rmm imports to use pylibrmm/librmm (#16913)
31423d0 
This PR updates all the RMM imports to use pylibrmm/librmm now that `rmm._lib` is deprecated . It should be merged after [rmm/1676](rapidsai/rmm#1676).

Authors:
  - Matthew Murray (https://github.com/Matt711)

Approvers:
  - Lawrence Mitchell (https://github.com/wence-)
  - Charles Blackmon-Luca (https://github.com/charlesbluca)

URL: #16913
@Matt711 Matt711 mentioned this pull request Oct 31, 2024
Merged
3 tasks
rapids-bot bot pushed a commit that referenced this pull request Oct 31, 2024
@Matt711
Deprecate rmm._lib (#1713)
8d49fff 
Follows up #1676 to add deprecation warnings to the `rmm._lib` sub package.

Authors:
  - Matthew Murray (https://github.com/Matt711)

Approvers:
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: #1713
@bdice bdice mentioned this pull request Dec 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vyasr vyasr vyasr approved these changes

@wence- wence- wence- approved these changes

@harrism harrism Awaiting requested review from harrism

Assignees

@Matt711 Matt711

Labels
3 - Ready for review Ready for review by team CMake improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Related to RMM Python API
Projects
RMM Project Board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] Reorganize Cython to separate C++ bindings and make Cython classes public
4 participants
@Matt711 @vyasr @wence- @harrism