[ROCm] Stop ROCPROFILER_REGISTER_LIBRARY leaking into engine subprocesses - #1077
[ROCm] Stop ROCPROFILER_REGISTER_LIBRARY leaking into engine subprocesses#1077roberteg16 wants to merge 1 commit into
Conversation
…sses When HIP initializes, librocprofiler-sdk records its own path in ROCPROFILER_REGISTER_LIBRARY, from rocprofiler::registration::set_rocprofiler_register_library() via a call_once that calls set_env(..., overwrite=1). librocprofiler-register reads it in rocp_reg_scan_for_tools() to pick which library to dlopen instead of the default librocprofiler-sdk.so. The variable is inherited by child processes, and a process that starts with it already set produces torch profiler traces with no GPU records at all. Under the V1 engine every trace is taken from an engine-core subprocess, so in practice no ROCm torch-profiler capture contains GPU activity. Measured on gfx1151 with the same script and only the variable differing: 6 GPU events and 2281 us of device time with it unset, 0 and 0 with it pre-set. The variable is set through a C-level setenv() that os.environ never sees, so os.unsetenv() is required to clear it; the accompanying os.environ.pop() only keeps the Python mapping consistent when the variable was also exported by the user. Clearing happens where the HIP/CUDA visible-device vars are already reconciled, which runs before the engine subprocesses are created. This works around the ROCm behaviour rather than fixing it: exporting a process-local value into the environment leaves every descendant to inherit it, with no PID guard and no cleanup, and the resulting failure is silent. Signed-off-by: Robert Esclapez Garcia <robert.garcia@amd.com>
716b348 to
06d96c7
Compare
Who owns the action to report the ROCm issue? |
|
@roberteg16, the description and justification of this change seemed a little too AI-ish to me, so I explored the issue a bit with my own AI. Instead of blocking child processes from inheriting their parent's environment (which is unusual and suspicious), could you try simply exporting |
Ideally this should work without any change on our side. Older versions of rocm worked without this workaround. I should have kept this PR as draft for me, and not mark it ready for review. Sorry about that. |
|
FYI, @roberteg16, ROCm/rocm-systems#9744 proposes changes to the way |
2 participants
Summary
When HIP initializes,
librocprofiler-sdkrecords its own path in theROCPROFILER_REGISTER_LIBRARYenvironment variable. The variable is inherited by child processes, and a process that starts with it already set produces torch profiler traces containing no GPU records at all. Under the V1 engine every trace is captured in an engine-core subprocess, so in practice no ROCm torch-profiler capture contains GPU activity.This clears the variable where the HIP/CUDA visible-device vars are already reconciled, which runs before the engine subprocesses are created. It is what makes
--profiler-config '{"profiler": "torch", ...}'produce usable GPU timelines on ROCm.The variable is set through a C-level
setenv()thatos.environnever sees, soos.unsetenv()is required to clear it; the accompanyingos.environ.pop()only keeps the Python mapping consistent for the case where the user also exported the variable.Where the variable comes from
This is a ROCm-side issue; the change here is a workaround, not a root-cause fix.
librocprofiler-sdk, inrocprofiler::registration::set_rocprofiler_register_library()— acall_oncethat callsset_env("ROCPROFILER_REGISTER_LIBRARY", <resolved path>, /*overwrite=*/1).librocprofiler-registerreferences the string exactly once, in agetenv()insiderocp_reg_scan_for_tools(), where it selects which library todlopeninstead of the defaultlibrocprofiler-sdk.so.Exporting a process-local value (the path of the library this process loaded) into the environment means every descendant inherits it, with no PID guard and no cleanup. The failure is silent: the trace is still produced, just with no GPU rows.
Test plan
ruff check/ruff format --checkon both changed files, plus the full pre-commit hook set (ruff, mypy 3.10, typos, SPDX, forbidden-imports,torch.cudaAPI check) — all pass.Confirmed the variable is injected below Python's view. On gfx1151 with ROCm 10.0 nightly (
torch 2.12.0+rocm10.0.0a20260729):Env-inheritance mechanics verified against
libc.setenv(): after a C-levelsetenv, bothforkandspawnchildren still see the value viagetenv(). With the two-line clear applied, both seeNULL.Causal check on the profiler itself — same script, same machine, only the variable differs:
Full end-to-end vLLM serve capture with
--profiler-config.Relationship to #1076
Draft #1076 currently carries the same two-file change as a side fix. This PR extracts it as a standalone change so it can land independently of the W4A16 kernel work. #1076 is left untouched for now; whichever lands first, the other should drop these two files.