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

[FEA] Allow setting *_pool_size with human-readable string #1670

Merged
merged 9 commits into from
Sep 9, 2024
Merged

[FEA] Allow setting *_pool_size with human-readable string #1670

merged 9 commits into from
Sep 9, 2024

Conversation

Matt711
Copy link
Contributor

@Matt711 Matt711 commented Sep 5, 2024

Description

Closes #173

Checklist

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

@Matt711 Matt711 requested a review from a team as a code owner September 5, 2024 23:29
@github-actions github-actions bot added the Python Related to RMM Python API label Sep 5, 2024
harrism
harrism reviewed Sep 6, 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.

Nice convenience addition. Thank you!

A couple of comments/questions before approving.

python/rmm/rmm/_lib/memory_resource.pyx Outdated Show resolved Hide resolved
python/rmm/rmm/_lib/memory_resource.pyx Outdated Show resolved Hide resolved
@harrism harrism added feature request New feature or request non-breaking Non-breaking change labels Sep 6, 2024
@harrism harrism requested review from a team and removed request for a team September 6, 2024 00:36
wence-
wence- requested changes Sep 6, 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.

A suggestion for an alternative approach

python/rmm/rmm/_lib/memory_resource.pyx Outdated Show resolved Hide resolved
python/rmm/rmm/_lib/memory_resource.pyx Outdated Show resolved Hide resolved
python/rmm/rmm/_lib/memory_resource.pyx Outdated Show resolved Hide resolved
python/rmm/rmm/_lib/memory_resource.pyx Outdated Show resolved Hide resolved
python/rmm/rmm/_lib/memory_resource.pyx Outdated Show resolved Hide resolved
python/rmm/rmm/_lib/memory_resource.pyx Outdated Show resolved Hide resolved
@Matt711 Matt711 requested a review from a team as a code owner September 6, 2024 14:38
@github-actions github-actions bot added the CMake label Sep 6, 2024
@Matt711 Matt711 requested review from harrism and wence- September 6, 2024 14:45
wence-
wence- reviewed Sep 6, 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.

Some small cleanups

python/rmm/rmm/_lib/helper.pyx Outdated Show resolved Hide resolved
python/rmm/rmm/_lib/helper.pyx Outdated Show resolved Hide resolved
python/rmm/rmm/_lib/helper.pyx Outdated Show resolved Hide resolved
python/rmm/rmm/_lib/helper.pyx Outdated Show resolved Hide resolved
python/rmm/rmm/_lib/helper.pyx Outdated Show resolved Hide resolved
python/rmm/rmm/_lib/helper.pyx Outdated Show resolved Hide resolved
python/rmm/rmm/_lib/helper.pyx Outdated Show resolved Hide resolved
python/rmm/rmm/_lib/helper.pxd Outdated Show resolved Hide resolved
@Matt711 Matt711 requested a review from wence- September 6, 2024 20:09
wence-
wence- requested changes Sep 9, 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 @Matt711 a few more tiny suggestions to tidy up the documentation. Looking good!

python/rmm/rmm/_lib/helper.pyx Outdated Show resolved Hide resolved
python/rmm/rmm/_lib/helper.pyx Outdated Show resolved Hide resolved
python/rmm/rmm/_lib/helper.pyx Outdated Show resolved Hide resolved
python/rmm/rmm/_lib/memory_resource.pyx Outdated Show resolved Hide resolved
python/rmm/rmm/rmm.py Show resolved Hide resolved
python/rmm/rmm/rmm.py Show resolved Hide resolved
python/rmm/rmm/tests/test_rmm.py Show resolved Hide resolved
wence-
wence- approved these changes Sep 9, 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 @Matt711

harrism
harrism approved these changes Sep 9, 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.

Much improved. Thank you!

@harrism
Copy link
Member

harrism commented Sep 9, 2024

/merge

@rapids-bot rapids-bot bot merged commit 687ed5c into rapidsai:branch-24.10 Sep 9, 2024
57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdice bdice bdice left review comments

@wence- wence- wence- approved these changes

@harrism harrism harrism approved these changes

Assignees
No one assigned
Labels
CMake feature request New feature or request 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] Allow setting pool_size with human-readable string
4 participants
@Matt711 @harrism @wence- @bdice