nixlbench: limit Neuron core allocation in SG mode for multi-processing#1913
nixlbench: limit Neuron core allocation in SG mode for multi-processing#1913fengjica wants to merge 1 commit into
Conversation
fengjica
requested review from
a team,
aranadive,
brminich and
ovidiusm
as code owners
|
👋 Hi fengjica! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change updates ChangesNeuron core allocation changes
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
fengjica
force-pushed
the
dev/fix_nrt_core_alloc
branch
from
c42c358 to
74a44d8
Compare
|
/build |
|
👀 Investigating |
|
🤖 CI Triage Agent — I have clear evidence now. The build stages succeeded; the failing stages (183 and 200 - "Allocate DL EP Environment") failed at Slurm resource allocation. Let me confirm the timing to distinguish hang vs. genuine wait. Looking at the log for stage 200:
That's exactly ~1 hour (3600s) between the salloc invocation and the timeout error. The This is not a hang in nixl code, and not a code defect at all. The build of NIXL + UCX + nixlbench all completed successfully (stages 159, 162 succeeded). Both allocation attempts (v1.22.x → job 1507450, and master → job 1507420) sat in the Slurm queue for the full immediate-timeout window on partition The earlier stage failures (124/125 Summary: The Root cause: Infrastructure/capacity, not a code defect. Both allocation attempts ran Implicated commit: unknown — not code-related. The build for commit [REDACTED:Hex High Entropy String] compiled cleanly; failure is in cluster resource allocation. File: The Jenkins pipeline SLURM allocation step ( Suggested fix:
Related: none found (no code issue to correlate; failure is Slurm allocation
|
|
🤖 CI Triage Agent — TL;DR: The "Clang Format Check" failed because Full analysisSummary: GitHub Actions Root cause: The newly added Implicated commit: [REDACTED:Hex High Entropy String] (PR #1913, branch File: benchmark/nixlbench/src/utils/neuron.cpp (the Suggested fix: Reformat the file locally and commit the result: The expected result matches what CI proposed: std::cerr << "nixlbench: NEURON_RT_VISIBLE_CORES is set to \"" << visible_cores
<< "\" but SG mode uses only the first core (vnc=0)" << std::endl;Then Related: none
|
fengjica
force-pushed
the
dev/fix_nrt_core_alloc
branch
from
48faf8b to
b618855
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmark/nixlbench/src/utils/neuron.cpp`:
- Around line 160-163: Run clang-format-19 in place on the file containing the
NEURON_RT_VISIBLE_CORES warning, ensuring the stream expression in the neuron
utility is reformatted to the repository’s expected layout. Commit the resulting
formatting-only change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 3f2b97d6-75de-4d70-9c42-56bc9af322b4
📒 Files selected for processing (1)
benchmark/nixlbench/src/utils/neuron.cpp
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmark/nixlbench/src/utils/neuron.cpp`:
- Around line 155-158: Check the return value of setenv in the SG-mode branch
before printing the success message or proceeding to nrt_init. On failure,
report the error and abort initialization; only log that NEURON_RT_NUM_CORES was
set when setenv succeeds.
- Around line 152-165: Update the SG core-handling condition in neuron.cpp to
use IS_PAIRWISE_AND_SG() instead of checking xferBenchConfig::mode alone. Keep
the existing environment-variable setup and warning behavior unchanged, but
ensure it runs only for pairwise SG configurations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 98fba73b-3d01-4abe-9c5e-d964c60760c8
📒 Files selected for processing (1)
benchmark/nixlbench/src/utils/neuron.cpp
| if (XFERBENCH_MODE_SG == xferBenchConfig::mode) { | ||
| const char *num_cores = getenv("NEURON_RT_NUM_CORES"); | ||
| const char *visible_cores = getenv("NEURON_RT_VISIBLE_CORES"); | ||
| if (!num_cores && !visible_cores) { | ||
| setenv("NEURON_RT_NUM_CORES", "1", 0); | ||
| std::cerr << "nixlbench: SG mode — set NEURON_RT_NUM_CORES=1" | ||
| << std::endl; | ||
| } else if (visible_cores) { | ||
| std::cerr << "nixlbench: NEURON_RT_VISIBLE_CORES is set to \"" | ||
| << visible_cores | ||
| << "\" but SG mode uses only the first core (vnc=0)" | ||
| << std::endl; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Gate SG core handling on pairwise SG
Use IS_PAIRWISE_AND_SG() here instead of checking xferBenchConfig::mode alone; otherwise non-pairwise SG runs still get forced to NEURON_RT_NUM_CORES=1 and vnc=0.
🧰 Tools
🪛 GitHub Actions: Clang Format Check / 0_clang-format.txt
[error] 154-160: clang-format check failed for modified C/C++ file. clang-format-diff-19 -p1 -style=file -v detected formatting differences (std::cerr output stream formatting/line wrapping).
🪛 GitHub Actions: Clang Format Check / clang-format
[error] 154-157: clang-format-diff-19 reported formatting differences (C/C++ formatting check failed). Run clang-format to apply the suggested changes.
[error] 159-164: clang-format-diff-19 reported formatting differences (C/C++ formatting check failed). The affected std::cerr statement formatting needs to match clang-format output.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmark/nixlbench/src/utils/neuron.cpp` around lines 152 - 165, Update the
SG core-handling condition in neuron.cpp to use IS_PAIRWISE_AND_SG() instead of
checking xferBenchConfig::mode alone. Keep the existing environment-variable
setup and warning behavior unchanged, but ensure it runs only for pairwise SG
configurations.
b618855 to
da85dd0
Compare
|
🤖 CI Triage Agent — TL;DR: The Clang Format Check failed because Full analysisSummary: Root cause: The PR (#1913) modified std::cerr << "nixlbench: SG mode — set NEURON_RT_NUM_CORES=1"
<< std::endl;to a single line ending in Implicated commit: [REDACTED:Hex High Entropy String] (PR #1913, branch File: Suggested fix: Run the repository's formatter locally on the changed file and commit the result: Concretely, collapse the wrapped Related: none found.
|
fengjica
force-pushed
the
dev/fix_nrt_core_alloc
branch
from
da85dd0 to
e7a3114
Compare
On Neuron instances, nrt_init() claims all visible Neuron cores by default. When testing multiple nixlbench processes in SG pairwise mode (one per device), each process tries to claim all cores, causing "Logical Neuron Core(s) not available" failures for all but the first process. Set NEURON_RT_NUM_CORES=1 before nrt_init() in SG mode so each process only reserves one core. MG mode is unaffected (single process per host, claims all cores).
fengjica
force-pushed
the
dev/fix_nrt_core_alloc
branch
from
e7a3114 to
efd9672
Compare
4 participants
On Neuron instances, nrt_init() claims all visible Neuron cores by default. When testing multiple nixlbench processes in SG pairwise mode (one process per device), each process tries to claim all cores, causing "Logical Neuron Core(s) not available" failures for all but the first process.
Set NEURON_RT_NUM_CORES=1 before nrt_init() in SG mode so each process only reserves one core.
MG mode is unaffected (single process per host, claims all cores).
What?
Fix nixlbench multiprocessing with neruon runtime.
Why?
Without the fix, nixlbench multiprocessing (SG mode) would fail.
How?
Set NEURON_RT_NUM_CORES = 1, before nrt_init().
Tested with trn2.48xlarge instance.
Summary by CodeRabbit