Support cuDF's built-in spilling #984
Conversation
madsbk
added
improvement
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## branch-22.12 #984 +/- ##
==============================================
Coverage ? 0.00%
==============================================
Files ? 18
Lines ? 2265
Branches ? 0
==============================================
Hits ? 0
Misses ? 2265
Partials ? 0 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
DeviceHostFile: ignore spillable objects
madsbk
force-pushed
the
cudf_spilling
branch
from
c06d30b to
2520b81
Compare
madsbk
changed the title
| Returns: | ||
| - True if cudf's internal spilling is enabled, or | ||
| - False if it is disabled, or | ||
| - None if the current version of cudf doesn't support spilling, or |
There was a problem hiding this comment.
Does documenting "if the current version of cuDF doesn't support spilling" make sense? We're not backwards compatible, is there any case where this would occur in practice?
There was a problem hiding this comment.
Since we don't depend on cudf, I guess people can have any version?
There was a problem hiding this comment.
This is theoretically right, but it isn't supported anyway. I guess it would make more sense for us to handle cuDF as an optional dependency and then we could define the minimum version and drop checks like this, but we can address this on a follow-up PR.
| import versioneer | ||
| from setuptools import setup |
There was a problem hiding this comment.
Is this just linting that added this, or did you intend to make this change for some reason?
There was a problem hiding this comment.
Was linting, don't we run CI linting on setup.py?
There was a problem hiding this comment.
I guess not, it seems like only the dask_cuda directory is checked by CI. I constantly see PRs changing a small linting here and there, and is not exactly clear to me why that happens. How do you run linting, do you rely on pre-commit for that or do you run it manually? I expect pre-commit to only execute on files that actually changed when it runs as part of git commit.
There was a problem hiding this comment.
I used pre-commit run --all-files. Maybe we should just change them to check the whole repos?
|
|
||
| pytest.importorskip( | ||
| "cudf.core.buffer.spill_manager", | ||
| reason="Current version of cudf doesn't support built-in spilling", |
There was a problem hiding this comment.
Same here, will this ever happen in practice?
Co-authored-by: Peter Andreas Entschev <[email protected]>
|
@pentschev, thanks for the review. It is ready for another round :) |
|
@gpucibot merge |
|
Thanks @pentschev! |
|
rerun tests |
1 similar comment
|
rerun tests |
Support of the new built-in spilling in cuDF so that
device_memory_limitandmemory_limitignores cuDF's device buffers.This is only implemented for
DeviceHostFile. Since jit-unspill also targets cuDF and libraries such as cupy isn't supported, I don't think it is important to support cuDF's built-in spilling inProxifyHostFile.For now,
DeviceHostFilesimply ignores cuDF's device buffers and let cuDF handle the spilling. This means thatDeviceHostFilemight estimate the device and host memory usage incorrectly (or more incorrectly than usually).