Use new rapids-logger library #2566
Conversation
vyasr
added
improvement
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
vyasr
force-pushed
the
feat/rapids_logger_library
branch
from
d4a9a82 to
cc29810
Compare
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
/ok to test |
| { | ||
| auto* filename = std::getenv("RAFT_DEBUG_LOG_FILE"); | ||
| return (filename == nullptr) | ||
| ? static_cast<rapids_logger::sink_ptr>(std::make_shared<rapids_logger::stderr_sink_mt>()) |
There was a problem hiding this comment.
Same question as on cudf about static_cast.
jameslamb
requested review from
jameslamb
and removed request for
raydouglass
There was a problem hiding this comment.
Approving so I don't block merging, since we're now in the state where we need to start merging this sequence of PRs, assuming all the testing-specific stuff will be dropped before merging.
Please see my comment about declaring the dependency on rapids-logger... I think that is missing here.
| @@ -29,6 +29,7 @@ EXCLUDE_ARGS=( | |||
| --exclude "libcusparse.so.*" | |||
| --exclude "libnvJitLink.so.*" | |||
| --exclude "libucp.so.*" | |||
| --exclude "librapids_logger.so" | |||
There was a problem hiding this comment.
This list was in alphabetical order. Could we preserve that, by moving this up above libucp.so.*? I think that's helpful for scanning this at a glance.
| import librmm # noqa: F401 | ||
| import rapids_logger | ||
|
|
||
| rapids_logger.load_library() |
There was a problem hiding this comment.
| import librmm # noqa: F401 | |
| import rapids_logger | |
| rapids_logger.load_library() | |
| import librmm | |
| import rapids_logger | |
| librmm.load_library() | |
| rapids_logger.load_library() |
similar to rapidsai/cudf#17899 (comment)
There was a problem hiding this comment.
I think we want rapids_logger first, right? In this case it may not matter because librmm is header-only, but that may change -- and we otherwise need to load these libraries in dependency order, iirc.
There was a problem hiding this comment.
Yeah you're right, the order here should be flipped (rapids-logger before librmm).
| @@ -31,6 +31,7 @@ authors = [ | |||
| license = { text = "Apache 2.0" } | |||
| requires-python = ">=3.10" | |||
| dependencies = [ | |||
| "librmm==25.4.*,>=0.0.0a0", | |||
There was a problem hiding this comment.
I'd expected to see direct runtime and build-time dependencies on rapids-logger in libraft wheels and the conda recipe, as was done for libcudf in rapidsai/cudf#17899, because RAFT is exposing its own logger.
Don't we need that?
There was a problem hiding this comment.
Yes, we do. That is a good call, I'll add those.
Contributes to rapidsai/build-planning#104.