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.

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

Add the ability to register and unregister reinitialization hooks #1072

Merged
merged 14 commits into from
Jul 20, 2022

Conversation

shwina
Copy link
Contributor

@shwina shwina commented Jul 18, 2022

This PR introduces the ability to register one or more "hooks" that will be invoked before each call to rmm.reinitialize().

The motivation is to allow downstream libraries to register hooks that are responsible for deleting any internal objects that that keep references to an RMM memory resource. In particular, this is useful for rapidsai/cudf#11246.

@shwina shwina requested a review from a team as a code owner July 18, 2022 17:45
@github-actions github-actions bot added the Python Related to RMM Python API label Jul 18, 2022
python/rmm/rmm.py Outdated Show resolved Hide resolved
@jakirkham jakirkham added non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels Jul 18, 2022
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Ashwin! 🙏

Generally this seems good. Had a couple comments below

python/rmm/rmm.py Outdated Show resolved Hide resolved
python/rmm/tests/test_rmm.py Outdated Show resolved Hide resolved
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.

Mostly docstring-related quibbles.

python/rmm/rmm.py Outdated Show resolved Hide resolved
python/rmm/rmm.py Outdated Show resolved Hide resolved
python/rmm/rmm.py Outdated Show resolved Hide resolved
python/rmm/rmm.py Outdated Show resolved Hide resolved
python/rmm/rmm.py Outdated Show resolved Hide resolved
python/rmm/rmm.py Outdated Show resolved Hide resolved
python/rmm/rmm.py Outdated Show resolved Hide resolved
shwina and others added 2 commits July 19, 2022 10:28
Co-authored-by: Lawrence Mitchell <wence@gmx.li>
python/rmm/rmm.py Outdated Show resolved Hide resolved
Co-authored-by: Lawrence Mitchell <wence@gmx.li>
Copy link
Member

@pentschev pentschev left a comment

Choose a reason for hiding this comment

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

Thanks @shwina for the work here, and patience for the discussions!

Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

A couple minor nits -- I'm really happy with how this progressed during review from others' inputs.

"""
global _reinitialize_hooks
_reinitialize_hooks = list(
filterfalse(lambda x: x[0] == func, _reinitialize_hooks)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why filterfalse with == when filter with != would do the same thing? Then we don't need to import from itertools.

Suggested change
filterfalse(lambda x: x[0] == func, _reinitialize_hooks)
filter(lambda x: x[0] != func, _reinitialize_hooks)

Copy link
Contributor

@bdice bdice Jul 19, 2022

Choose a reason for hiding this comment

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

Also we could use a comprehension, which tends to be a bit faster as well as more succinct.

_reinitialize_hooks = [x for x in _reinitialize_hooks if x[0] != func]

Copy link
Member

@jakirkham jakirkham Jul 19, 2022

Choose a reason for hiding this comment

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

This (the list comprehension) seems less succinct than what is already here

Copy link
Contributor

@bdice bdice Jul 19, 2022

Choose a reason for hiding this comment

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

The comparison I was making was:

_reinitialize_hooks = list(
    filterfalse(lambda x: x[0] == func, _reinitialize_hooks)
)

(3 lines, 92 chars)
vs.

_reinitialize_hooks = [x for x in _reinitialize_hooks if x[0] != func]

(1 line, 70 chars)

The difference is small. I cared more about removing the from itertools import filterfalse when we just needed to swap filterfalse, == for filter, !=.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah the filter change seems fine (assuming that check behaves as expected). Tried to clarify that above

python/rmm/rmm.py Outdated Show resolved Hide resolved
python/rmm/rmm.py Outdated Show resolved Hide resolved
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 for the changes and doc clarifications, looks good!

@shwina
Copy link
Contributor Author

shwina commented Jul 20, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 8e5b190 into rapidsai:branch-22.08 Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Related to RMM Python API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants