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

[BUG] deprecation warning on rmm::mr::device_memory_resource::supports_streams #1814

Closed
abellina opened this issue Feb 28, 2024 · 4 comments · Fixed by #1847
Closed

[BUG] deprecation warning on rmm::mr::device_memory_resource::supports_streams #1814

abellina opened this issue Feb 28, 2024 · 4 comments · Fixed by #1847
Assignees
Labels
bug Something isn't working

Comments

@abellina
Copy link
Collaborator

This is something I saw in my local build of spark-rapids-jni and think we should adjust SparkResourceAdaptorJni to use the new async_memory_resource concept:

INFO]      [exec] [15/36] Building CXX object CMakeFiles/spark_rapids_jni.dir/src/SparkResourceAdaptorJni.cpp.o
[INFO]      [exec] spark-rapids-jni/src/main/cpp/src/SparkResourceAdaptorJni.cpp: In member function 'virtual bool {anonymous}::spark_resource_adaptor::supports_streams() const':
[INFO]      [exec] spark-rapids-jni/src/main/cpp/src/SparkResourceAdaptorJni.cpp:404:86: warning: 'virtual bool rmm::mr::device_memory_resource::supports_streams() const' is deprecated: Functionality removed in favor of cuda::mr::async_memory_resource. [-Wdeprecated-declarations]
[INFO]      [exec]   404 |   bool supports_streams() const noexcept override { return resource->supports_streams(); }
[INFO]      [exec]       |                                                            ~~~~~~~~~~~~~~~~~~~~~~~~~~^~
[INFO]      [exec] In file included from spark-rapids-jni/src/main/cpp/src/SparkResourceAdaptorJni.cpp:17:
[INFO]      [exec] spark-rapids-jni/target/libcudf-install/include/rmm/mr/device/device_memory_resource.hpp:306:3: note: declared here
[INFO]      [exec]   306 |   supports_streams() const noexcept
[INFO]      [exec]       |   ^~~~~~~~~~~~~~~~
@mythrocks
Copy link
Collaborator

mythrocks commented Mar 7, 2024

Seeking clarity on this issue. There seem to be a couple of tasks implied here:

  1. Addressing the deprecation warning: libcudf has addressed this by removing the override. (See https://github.com/rapidsai/cudf/pull/14857/files.) This needs addressing before supports_streams() is removed in 24.04.
  2. Reimplement with async_memory_resource.

Is the intention for both to be addressed here? (I'm assuming yes, for now.)

mythrocks added a commit to mythrocks/spark-rapids-jni that referenced this issue Mar 7, 2024
Fixes NVIDIA#1814.

This change removes the override of `spark_resource_adaptor::supports_stream()`,
in light of rapidsai/rmm#1389, which deprecated the
method.

Similar to rapidsai/cudf#14857, this change removes
the deprecated override.

Signed-off-by: MithunR <mythrocks@gmail.com>
@mythrocks
Copy link
Collaborator

#1847 should resolve the deprecation warning.

  1. Reimplement with async_memory_resource.

Actually, I'm not fully clear on this point. asynch_memory_resource is final, so we're not asking for spark_memory_resource to extend it.

Is the suggestion for spark_memory_resource to take an async_memory_resource explicitly? It doesn't currently use much beyond what's in the device_memory_resource interface, so I'm guessing not.

If Rmm.getCurrentDeviceResource() were to return an async_memory_resource, wouldn't spark_memory_resource work unchanged?

@abellina
Copy link
Collaborator Author

abellina commented Mar 8, 2024

Sorry for the confusion, the deprecation is what I meant. I was confused (still am a bit) on the async_memory_resource concepts, and misunderstood when I filed this whether there was a type hierarchy, which turns out to be false.

@mythrocks
Copy link
Collaborator

Thank you for clarifying, @abellina.

mythrocks added a commit that referenced this issue Mar 10, 2024
Fixes #1814.

This change removes the override of `spark_resource_adaptor::supports_stream()`,
in light of rapidsai/rmm#1389, which deprecated the
method.

Similar to rapidsai/cudf#14857, this change removes
the deprecated override.

Signed-off-by: MithunR <mythrocks@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants