Skip to content

Hoisting Logger from communicator - #1081

Merged
rapids-bot[bot] merged 12 commits into
rapidsai:mainfrom
nirandaperera:pull-logger-out-from-communicator
Jul 6, 2026
Merged

Hoisting Logger from communicator#1081
rapids-bot[bot] merged 12 commits into
rapidsai:mainfrom
nirandaperera:pull-logger-out-from-communicator

Conversation

@nirandaperera

@nirandaperera nirandaperera commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

This is a prerequisite to introduce a runtime environment for rapidsmpf. Historically, logger resided in the Communicator, because there were some ideas about distributed log collection. But it is unlikely that we will take that route in the future.

This PR hoists the logger from communicator to a separate header.

Changes

Logger is now a standalone class in rapidsmpf/communicator/logger.hpp:

  • Moved out of Communicator, promoted to namespace scope as rapidsmpf::Logger.
  • Uses std::int32_t for rank instead of Rank, so logger.hpp doesn't depend on communicator.hpp (clean one-way dependency: communicator.hpp includes logger.hpp).
  • New constructor Logger(config::Options) (rank defaults to -1).
  • New Logger::set_rank(std::int32_t) — thread-safe, may be called any number of times. Lets a communicator install the logger first and assign rank after bootstrap.

Communicators take a std::shared_ptr<Logger> externally:

  • Single, MPI, and UCXX constructors now require the caller to provide the logger.
  • Each ctor calls logger->set_rank(...) once the local rank is known.
  • For UCXX, the logger is stored in SharedResources so callbacks observed during bootstrap can log immediately.

config::Options removed where it was only feeding the logger:

  • Single::Single(progress_thread, logger)
  • MPI::MPI(comm, progress_thread, logger)
  • UCXX::UCXX(initialized_rank, progress_thread, logger) (the options_ member is gone)

Factory functions also take an external logger:

  • ucxx::init_using_mpi(mpi_comm, options, progress_thread, logger)
  • bootstrap::create_ucxx_comm(progress_thread, type, options, logger)

No behavior change for users

The Python wrappers still accept Options and construct the cpp_Logger internally, so the Python API is unchanged.

Signed-off-by: niranda perera <niranda.perera@gmail.com>
@nirandaperera
nirandaperera requested review from a team as code owners June 2, 2026 23:46
@nirandaperera nirandaperera added breaking Introduces a breaking change improvement Improves an existing functionality labels Jun 2, 2026
@madsbk

madsbk commented Jun 3, 2026

Copy link
Copy Markdown
Member

For context, the motivation for placing the logger on the communicator itself was to allow each communicator to implement its own logging behavior. For example, a communicator could collect logs on a specific machine.

We have not implemented this yet, and maybe we never will, but moving the logger out of the communicator would make that harder.

@pentschev

Copy link
Copy Markdown
Member

For context, the motivation for placing the logger on the communicator itself was to allow each communicator to implement its own logging behavior. For example, a communicator could collect logs on a specific machine.

We have not implemented this yet, and maybe we never will, but moving the logger out of the communicator would make that harder.

I agree with Niranda's stance here, it's likely we never will implement that. Most cases we care about will fall into one of two categories:

  1. SPMD: primarily used on top of a launcher like mpirun or rrun, both redirect logs to the launcher process, so distributed logging would be redundant. For any other cases that simply run RapidsMPF in "SPMD-mode", they would use something like Ray or Dask to achieve that, so redirecting logs would fall again under category 1.
  2. Third-party launcher with client (e.g., Ray, Dask): usually provide their own logging redirection to the client. To cover those cases, RapidsMPF would need to use the framework's redirection mechanism somehow, which is not guaranteed to support the same Communicator, and thus using Communicator will not necessarily be useful or even work.

IMO, decoupling Logger from Communicator is the right approach, which also allows us logging during initial setup.

Signed-off-by: niranda perera <niranda.perera@gmail.com>
Signed-off-by: niranda perera <niranda.perera@gmail.com>

@madsbk madsbk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Alright, I am onboard, but I really don't like the rank=-1 / set_rank API. Is there any way we can avoid that?

If not, I think we should drop rank entirely and introduce a name instead.

Comment thread cpp/include/rapidsmpf/communicator/logger.hpp Outdated
Comment thread cpp/include/rapidsmpf/communicator/logger.hpp Outdated
@pentschev

Copy link
Copy Markdown
Member

Alright, I am onboard, but I really don't like the rank=-1 / set_rank API. Is there any way we can avoid that?

If not, I think we should drop rank entirely and introduce a name instead.

I agree with Mads. Let's drop rank as the default for the logger and instead make Logger named with a string. Implementations can then choose to use the rank as the name, or use a default (like "Unnamed"/"Unknown") when there are no ranks/names assigned yet

@nirandaperera

Copy link
Copy Markdown
Contributor Author

Alright, I am onboard, but I really don't like the rank=-1 / set_rank API. Is there any way we can avoid that?
If not, I think we should drop rank entirely and introduce a name instead.

I agree with Mads. Let's drop rank as the default for the logger and instead make Logger named with a string. Implementations can then choose to use the rank as the name, or use a default (like "Unnamed"/"Unknown") when there are no ranks/names assigned yet

Sure, I agree with both of you @madsbk @pentschev. Let me push the changes

Signed-off-by: niranda perera <niranda.perera@gmail.com>
@nirandaperera
nirandaperera requested a review from madsbk June 5, 2026 18:56
Comment thread cpp/src/communicator/ucxx.cpp Outdated

public:
UCXX::Logger* logger{nullptr}; ///< UCXX logger
std::shared_ptr<Logger> logger; ///< UCXX logger (may be null before the

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we can avoid the null logger now? Either create a default logger or have the ctor create one from the options?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

…-out-from-communicator

Signed-off-by: niranda perera <niranda.perera@gmail.com>
Signed-off-by: niranda perera <niranda.perera@gmail.com>
@nirandaperera
nirandaperera requested a review from madsbk June 23, 2026 00:16

@madsbk madsbk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@nirandaperera

Copy link
Copy Markdown
Contributor Author

/merge

@rapids-bot
rapids-bot Bot merged commit 443164f into rapidsai:main Jul 6, 2026
66 checks passed
rapids-bot Bot pushed a commit to NVIDIA/cudf that referenced this pull request Jul 7, 2026
Depends on rapidsai/rapidsmpf#1081 

Adds new API changes to cudf polars and cudf streaming

Authors:
  - Niranda Perera (https://github.com/nirandaperera)

Approvers:
  - Mads R. B. Kristensen (https://github.com/madsbk)

URL: #22972
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Introduces a breaking change improvement Improves an existing functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants