Fall back to folly XLOG in CTRAN_LOG on AMD (#3558) - #3558
Open
sunguobao wants to merge 1 commit into
Open
Conversation
Contributor
|
@sunguobao has exported this pull request. If you are a Meta employee, you can view the originating Diff in D115468117. |
Summary: The standalone RCCL/conda build (`comms/rcclx/develop/projects/rccl`) globs all of `comms/ctran` into `librccl` and deliberately has no spdlog -- the same premise as D115067885. The recent defolly work re-broke that build: - D115082769 added `comms/ctran/utils/CtranLogger.h`, which includes `comms/utils/logger/SpdlogLogger.h` unconditionally. - Subsequent diffs swapped `folly/logging/xlog.h` for that header across ctran, including in `comms/ctran/utils/AsyncError.h` -- a header reached from `src/include/comm.h` via `comms/ctran/CtranComm.h`. `SpdlogLogger.h` hard-requires `SPDLOG_FMT_EXTERNAL` / `SPDLOG_ACTIVE_LEVEL` and the spdlog headers, so every affected TU fails: ``` SpdlogLogger.h:15: error: "SpdlogLogger requires SPDLOG_FMT_EXTERNAL from the build target" SpdlogLogger.h:19: error: "SpdlogLogger requires SPDLOG_ACTIVE_LEVEL from the build target" SpdlogLogger.h:22: fatal error: 'spdlog/sinks/dist_sink.h' file not found ``` Because `AsyncError.h` is reachable from `comm.h`, this is not confined to ctran sources -- it hits RCCL core (`src/transport/shm.cc` and effectively every TU that includes `comm.h`). Trunk is currently red for this build. Guard, do not exclude. `CtranAlgo.cc`, `AsyncError.h` and the bootstrap sources are core ctran; dropping them from the CMake source list leaves undefined symbols at link. Guard instead at the `CtranLogger.h` facade, on the condition D115067885 established -- `defined(__HIP_PLATFORM_AMD__)`. Guarding the facade rather than each consumer is what makes this hold up: the `AsyncError.h` include landed after the first call sites and would otherwise have needed its own guard, and `CTRAN_LOG_FIRST_N_*` arrived after that. Fall back to folly XLOG rather than compiling the macros out. That is what these call sites used before the spdlog migration, and it keeps ROCm logging alive instead of silently dropping ~119 call sites. `LogInit.cc` already registers the folly categories unconditionally -- `kCtranCategory`, `kCtranHeaderCategory`, and a ROCm-only `getHipCtranCategory()` under `#if defined(USE_ROCM)` -- each with `logPrefix = "CTRAN"`, `logFilePath` from `NCCL_DEBUG_FILE`, `logLevel` from `NCCL_DEBUG`, and the per-thread device-id context. So level gating, the CTRAN prefix, debug-file output and thread context are preserved on ROCm. Scope note, worth a reviewer's attention. `__HIP_PLATFORM_AMD__` is true for *every* AMD build, not just the spdlog-less CMake one. Buck's AMD config does have spdlog, so this changes its behavior too: `CTRAN_LOG(ERR, ...)` there no longer feeds `getLastCommsError()` (the folly path has no equivalent hook), and `CTRAN_LOG_FIRST_N` gating moves from spdlog levels to folly. `LogUT.cc` is the evidence -- its two spdlog-semantics cases, `TestCtranLoggerPreservesLastError` and `TestCtranLogFirstNPreservesEnabledBudget`, assert exactly those behaviors and cannot hold on the folly path, so they are guarded off on AMD. That is a real loss of AMD coverage with nothing replacing it. Keying the facade on `SPDLOG_FMT_EXTERNAL` / `SPDLOG_ACTIVE_LEVEL` instead would confine the change to the CMake build and leave both tests intact; the platform macro was chosen for consistency with D115067885. Happy to switch if reviewers prefer the narrower condition. Also adds `//folly/logging:logging` to the `exported_deps` of the target owning `CtranLogger.h`. The include sits behind `__HIP_PLATFORM_AMD__`, but autodeps is not preprocessor-aware and the Buck ROCm build needs the dep to compile. No behavior change on NVIDIA: the spdlog branch, including `CTRAN_LOG_FIRST_N_IMPL` and its `RateLimit.h` dependency, is untouched. Thanks to the reviewer suggestion in P2456385988, which replaced an earlier version of this diff that made `CTRAN_LOG` a no-op on AMD. Reviewed By: srinathb-meta Differential Revision: D115468117
sunguobao
force-pushed
the
export-D115468117
branch
from
August 11, 2026 03:13
14faf1a to
fee7370
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary:
The standalone RCCL/conda build (
comms/rcclx/develop/projects/rccl) globs allof
comms/ctranintolibrccland deliberately has no spdlog -- the samepremise as D115067885. The recent defolly work re-broke that build:
comms/ctran/utils/CtranLogger.h, which includescomms/utils/logger/SpdlogLogger.hunconditionally.folly/logging/xlog.hfor that header across ctran,including in
comms/ctran/utils/AsyncError.h-- a header reached fromsrc/include/comm.hviacomms/ctran/CtranComm.h.SpdlogLogger.hhard-requiresSPDLOG_FMT_EXTERNAL/SPDLOG_ACTIVE_LEVELandthe spdlog headers, so every affected TU fails:
Because
AsyncError.his reachable fromcomm.h, this is not confined to ctransources -- it hits RCCL core (
src/transport/shm.ccand effectively every TUthat includes
comm.h). Trunk is currently red for this build.Guard, do not exclude.
CtranAlgo.cc,AsyncError.hand the bootstrap sourcesare core ctran; dropping them from the CMake source list leaves undefined
symbols at link. Guard instead at the
CtranLogger.hfacade, on the conditionD115067885 established --
defined(__HIP_PLATFORM_AMD__). Guarding the facaderather than each consumer is what makes this hold up: the
AsyncError.hincludelanded after the first call sites and would otherwise have needed its own guard,
and
CTRAN_LOG_FIRST_N_*arrived after that.Fall back to folly XLOG rather than compiling the macros out. That is what these
call sites used before the spdlog migration, and it keeps ROCm logging alive
instead of silently dropping ~119 call sites.
LogInit.ccalready registers thefolly categories unconditionally --
kCtranCategory,kCtranHeaderCategory,and a ROCm-only
getHipCtranCategory()under#if defined(USE_ROCM)-- eachwith
logPrefix = "CTRAN",logFilePathfromNCCL_DEBUG_FILE,logLevelfrom
NCCL_DEBUG, and the per-thread device-id context. So level gating, theCTRAN prefix, debug-file output and thread context are preserved on ROCm.
Scope note, worth a reviewer's attention.
__HIP_PLATFORM_AMD__is true forevery AMD build, not just the spdlog-less CMake one. Buck's AMD config does
have spdlog, so this changes its behavior too:
CTRAN_LOG(ERR, ...)there nolonger feeds
getLastCommsError()(the folly path has no equivalent hook), andCTRAN_LOG_FIRST_Ngating moves from spdlog levels to folly.LogUT.ccis theevidence -- its two spdlog-semantics cases,
TestCtranLoggerPreservesLastErrorandTestCtranLogFirstNPreservesEnabledBudget, assert exactly those behaviors andcannot hold on the folly path, so they are guarded off on AMD. That is a real
loss of AMD coverage with nothing replacing it. Keying the facade on
SPDLOG_FMT_EXTERNAL/SPDLOG_ACTIVE_LEVELinstead would confine the changeto the CMake build and leave both tests intact; the platform macro was chosen
for consistency with D115067885. Happy to switch if reviewers prefer the
narrower condition.
Also adds
//folly/logging:loggingto theexported_depsof the target owningCtranLogger.h. The include sits behind__HIP_PLATFORM_AMD__, but autodeps isnot preprocessor-aware and the Buck ROCm build needs the dep to compile.
No behavior change on NVIDIA: the spdlog branch, including
CTRAN_LOG_FIRST_N_IMPLand itsRateLimit.hdependency, is untouched.Thanks to the reviewer suggestion in P2456385988, which replaced an earlier
version of this diff that made
CTRAN_LOGa no-op on AMD.Reviewed By: srinathb-meta
Differential Revision: D115468117