Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Memory Profiling #15866
Memory Profiling #15866
Changes from 10 commits
cf15a02
f1ff8d4
32ca877
aeff5c7
b323b28
386292b
3625db6
2a0c77a
7cae9d1
a4e4fc6
69a54f3
46d9471
ffa6360
6cf2ca3
339fa3e
124b179
2fe290b
3f2ae60
e3e3d73
944901a
3510e27
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: This doesn't behave like a normal context manager (leave the state unchanged after exit) because it doesn't unwind the change to the current memory resource that
rmm.statistics.enable_statistics
enacts. This means that the following code does an "unexpected" thing: side-effectfully changes the MR:It seems like the call to enable statistics should be paired with a matching disable statistics call. But this seems bad if it were done at every level of the call stack.
It feels like the right place for modification of the MR is in the
set_option
call.set_option("memory_profiling", True)
would store the old MR, push the statistics MR, and then a laterset_option("memory_profiling", False")
would pop the statistics MR if it still matches.This, however, would preclude use of the environment variable default (because that is set on import, and we don't want to have to call RMM on import).
Does this suggest that we need a specific
enable_memory_profiling
that can be used as a context manager that sets the option and sets up the MR?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is an issue, RMM already have
rmm.statistics.statistics()
andrmm.statistics.enable_statistics()
.Thus the user code would look something like:
Of cause, we could wrap/alias the rmm functions in cudf native functions. However, I am not sure if the disadvantage of the "unexpected" side-effect outweigh the user-friendliness of just setting
CUDF_MEMORY_PROFILING="1"
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, we could implement lazy environment variable in
options.py
. It might be worth streamlining the option module in any case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of solutions, it depends a little bit on how end-user-friendly we want this to be. If this is mostly for developer use, I am happy with the programmatic set_option + use RMM calls. If we want this as a debugging tool for end users "where is my code allocating lots of vram", then we likely need something a little more ergonomic.
As much as I am allergic to environment variables from a reproducibility point of view, I can see the attraction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure but I guess we could start with the
set_option + use RMM calls
and then later consider a more end-user-friendly solution?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a reasonable compromise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done