Skip to content

refactor: RuntimeContext.container_log_dir for every worker-visible log path - #486

Merged
ishandhanani merged 1 commit into
mainfrom
idhanani/container-log-dir
Sep 21, 2026
Merged

ishandhanani merged 1 commit into
mainfrom
idhanani/container-log-dir

Conversation

@ishandhanani

@ishandhanani ishandhanani commented Sep 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

Paths handed to processes inside the container spelled the log mount three different ways: a literal /logs for nsys output, fingerprints, and the Mooncake process config; the CONTAINER_LOG_DIR constant for the measurement window; and the host log_dir for the worker config dump and the torch profiler directory. The host path only works on clusters where the log directory is visible inside the container at the same path, which #431 showed is not every cluster.

RuntimeContext.container_log_dir is the one accessor. It follows the mount from_config creates at CONTAINER_LOG_DIR, so the default stays /logs and a remapped log mount needs no other change. Config dumps, nsys output, the torch profiler directory, fingerprints, the Mooncake process config path, the benchmark profiles directory, and the measurement window directory all read it.

Landing this on main so #431 can drop its _container_log_path helper on rebase; the stacked fork PRs cannot take maintainer pushes because the fork is organization-owned.

Review map

  • core/runtime.py: the accessor, and the log mount now uses CONTAINER_LOG_DIR so /logs is defined once.
  • cli/mixins/worker_stage.py: eight call sites read the accessor. The torch profiler directory was a host path before.
  • cli/mixins/benchmark_stage.py: profiles directory and measurement window directory read the accessor; the constant import goes.
  • tests/test_slurm.py: accessor default and remapped behavior, the config dump path both launch paths hand the backend, and a remapped mount driving the dump, profiler, and fingerprint paths. Other test fakes gain the attribute.

Full suite: 2959 passed on both 3.13 and 3.10 locally. About 20 more /logs literals remain in services, frontends, TRT-LLM, exporters, and the nsys observability module for a follow-up.

…og path

Paths handed to processes inside the container spelled the log mount
three different ways: a literal /logs for nsys output, fingerprints, and
the Mooncake process config; the CONTAINER_LOG_DIR constant for the
measurement window; and the host log_dir for the worker config dump and
the torch profiler directory. The host path only works on clusters where
the log directory is visible inside the container at the same path.

RuntimeContext.container_log_dir is the one accessor: it follows the
mount from_config creates at CONTAINER_LOG_DIR, so the default stays
/logs and a remapped log mount needs no other change. Config dumps, nsys
output, the torch profiler directory, fingerprints, the Mooncake process
config path, the profiles directory, and the measurement window directory
all read it.

Tests cover the accessor's default and remapped behavior, the config dump
path both launch paths hand the backend, and a remapped mount driving the
dump, profiler, and fingerprint paths.

Signed-off-by: Ishan Dhanani <idhanani@nvidia.com>
@ishandhanani
ishandhanani merged commit cf88363 into main Sep 21, 2026
10 checks passed
@ishandhanani
ishandhanani deleted the idhanani/container-log-dir branch September 21, 2026 23:45
ishandhanani added a commit to SemiAnalysisAI/srt-slurm that referenced this pull request Sep 21, 2026
Brings in NVIDIA#486 (RuntimeContext.container_log_dir), so the branch's own
_container_log_path helper and its Path import are dropped in favor of
the shared accessor; the config dump test is now identical on both sides.

Signed-off-by: Ishan Dhanani <idhanani@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant