Skip to content

Commit cae37ed

Browse files
committed
fix
Signed-off-by: Hemil Desai <hemild@nvidia.com>
1 parent bd1f09e commit cae37ed

5 files changed

Lines changed: 225 additions & 16 deletions

File tree

nemo_run/config.py

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -468,13 +468,33 @@ def get_name(self):
468468
return os.path.basename(self.path)
469469

470470
def to_command(
471-
self, with_entrypoint: bool = False, filename: Optional[str] = None, is_local: bool = False
471+
self,
472+
with_entrypoint: bool = False,
473+
filename: Optional[str] = None,
474+
is_local: bool = False,
475+
substitute_rundir_path: Optional[str] = None,
472476
) -> list[str]:
477+
"""Convert the script to a command.
478+
479+
Args:
480+
with_entrypoint: If True, prepend the entrypoint to the command.
481+
filename: If provided, write the inline script to this file.
482+
is_local: If True, use the local filename in the command.
483+
substitute_rundir_path: If provided, substitute /{RUNDIR_NAME} paths
484+
with this path in the inline script content. Used for non-container
485+
mode where container paths need to be replaced with actual cluster paths.
486+
"""
473487
if self.inline:
474488
if filename:
475489
os.makedirs(os.path.dirname(filename), exist_ok=True)
490+
inline_content = self.inline
491+
# Substitute /{RUNDIR_NAME} paths if specified (non-container mode)
492+
if substitute_rundir_path is not None:
493+
inline_content = inline_content.replace(
494+
f"/{RUNDIR_NAME}", substitute_rundir_path
495+
)
476496
with open(filename, "w") as f:
477-
f.write("#!/usr/bin/bash\n" + self.inline)
497+
f.write("#!/usr/bin/bash\n" + inline_content)
478498

479499
if is_local:
480500
cmd = [filename]

nemo_run/core/execution/slurm.py

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -369,12 +369,12 @@ def merge(
369369
main_executor.run_as_group = True
370370

371371
if main_executor.het_group_indices:
372-
assert (
373-
main_executor.heterogeneous
374-
), "heterogeneous must be True if het_group_indices is provided"
375-
assert (
376-
len(main_executor.het_group_indices) == num_tasks
377-
), "het_group_indices must be the same length as the number of tasks"
372+
assert main_executor.heterogeneous, (
373+
"heterogeneous must be True if het_group_indices is provided"
374+
)
375+
assert len(main_executor.het_group_indices) == num_tasks, (
376+
"het_group_indices must be the same length as the number of tasks"
377+
)
378378
assert all(
379379
x <= y
380380
for x, y in zip(
@@ -858,9 +858,9 @@ def materialize(self) -> str:
858858

859859
sbatch_flags = []
860860
if self.executor.heterogeneous:
861-
assert (
862-
len(self.jobs) == len(self.executor.resource_group)
863-
), f"Number of jobs {len(self.jobs)} must match number of resource group requests {len(self.executor.resource_group)}.\nIf you are just submitting a single job, make sure that heterogeneous=False in the executor."
861+
assert len(self.jobs) == len(self.executor.resource_group), (
862+
f"Number of jobs {len(self.jobs)} must match number of resource group requests {len(self.executor.resource_group)}.\nIf you are just submitting a single job, make sure that heterogeneous=False in the executor."
863+
)
864864
final_group_index = len(self.executor.resource_group) - 1
865865
if self.executor.het_group_indices:
866866
final_group_index = self.executor.het_group_indices.index(
@@ -870,9 +870,9 @@ def materialize(self) -> str:
870870
for i in range(len(self.executor.resource_group)):
871871
resource_req = self.executor.resource_group[i]
872872
if resource_req.het_group_index is not None:
873-
assert (
874-
self.executor.resource_group[i - 1].het_group_index is not None
875-
), "het_group_index must be set for all requests in resource_group"
873+
assert self.executor.resource_group[i - 1].het_group_index is not None, (
874+
"het_group_index must be set for all requests in resource_group"
875+
)
876876
if (
877877
i > 0
878878
and resource_req.het_group_index

nemo_run/run/torchx_backend/packaging.py

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515

1616
import logging
1717
import os
18+
from pathlib import Path
1819
from typing import Iterator, Optional, Type, Union
1920

2021
import fiddle as fdl
@@ -120,9 +121,23 @@ def _get_details_from_script(fn_or_script: Script, serialize_configs: bool):
120121
log.warning(f"Failed saving yaml configs due to: {e}")
121122

122123
args = fn_or_script.args
124+
125+
# For SlurmExecutor without container, substitute /{RUNDIR_NAME} paths
126+
# with actual cluster paths in inline scripts
127+
substitute_rundir_path = None
128+
if (
129+
isinstance(executor, SlurmExecutor)
130+
and executor.container_image is None
131+
and executor.tunnel is not None
132+
):
133+
substitute_rundir_path = os.path.join(
134+
executor.tunnel.job_dir, Path(executor.job_dir).name
135+
)
136+
123137
role_args = fn_or_script.to_command(
124138
filename=os.path.join(executor.job_dir, SCRIPTS_DIR, f"{name}.sh"),
125139
is_local=True if isinstance(executor, LocalExecutor) else False,
140+
substitute_rundir_path=substitute_rundir_path,
126141
)
127142
m = fn_or_script.path if fn_or_script.m else None
128143
no_python = fn_or_script.entrypoint != "python"

test/run/torchx_backend/test_packaging.py

Lines changed: 137 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,16 +14,22 @@
1414
# limitations under the License.
1515

1616
from dataclasses import dataclass
17+
from unittest.mock import MagicMock
1718

1819
import pytest
1920
from torchx import specs
2021

21-
from nemo_run.config import Partial, Script
22+
from nemo_run.config import RUNDIR_NAME, Partial, Script
2223
from nemo_run.core.execution.base import Executor
2324
from nemo_run.core.execution.launcher import FaultTolerance, Torchrun
2425
from nemo_run.core.execution.local import LocalExecutor
26+
from nemo_run.core.execution.slurm import SlurmExecutor
2527
from nemo_run.core.packaging.base import Packager
26-
from nemo_run.run.torchx_backend.packaging import merge_executables, package
28+
from nemo_run.core.tunnel.client import LocalTunnel
29+
from nemo_run.run.torchx_backend.packaging import (
30+
merge_executables,
31+
package,
32+
)
2733

2834

2935
@dataclass(kw_only=True)
@@ -301,3 +307,132 @@ def test_merge_executables():
301307
assert len(merged_app_def.roles) == 2
302308
assert merged_app_def.roles[0].name == "role1"
303309
assert merged_app_def.roles[1].name == "role2"
310+
311+
312+
class TestPackagingNonContainerMode:
313+
"""Tests for non-container mode path substitution in packaging."""
314+
315+
def test_package_script_inline_with_slurm_non_container_mode(self, tmp_path):
316+
"""Test that inline scripts have /nemo_run paths substituted in non-container mode."""
317+
# Create a SlurmExecutor without container
318+
executor = SlurmExecutor(
319+
account="test_account",
320+
partition="gpu",
321+
nodes=1,
322+
ntasks_per_node=8,
323+
container_image=None, # Non-container mode
324+
)
325+
executor.job_dir = str(tmp_path / "test-job")
326+
executor.experiment_id = "exp-123"
327+
328+
# Mock tunnel
329+
tunnel = MagicMock(spec=LocalTunnel)
330+
tunnel.job_dir = "/remote/experiments/exp-123"
331+
executor.tunnel = tunnel
332+
333+
# Create an inline script with /nemo_run paths
334+
fn_or_script = Script(
335+
inline=f"cd /{RUNDIR_NAME}/code && python /{RUNDIR_NAME}/scripts/run.py"
336+
)
337+
338+
app_def = package(
339+
name="test",
340+
fn_or_script=fn_or_script,
341+
executor=executor,
342+
)
343+
344+
assert app_def.name == "test"
345+
assert len(app_def.roles) == 1
346+
347+
# Read the generated script file and verify paths were substituted
348+
script_file = tmp_path / "test-job" / "scripts" / "test.sh"
349+
assert script_file.exists()
350+
351+
content = script_file.read_text()
352+
actual_job_dir = "/remote/experiments/exp-123/test-job"
353+
354+
# Should NOT contain /nemo_run paths
355+
assert f"/{RUNDIR_NAME}" not in content
356+
# Should contain the actual job directory path
357+
assert f"{actual_job_dir}/code" in content
358+
assert f"{actual_job_dir}/scripts/run.py" in content
359+
360+
def test_package_script_inline_with_slurm_container_mode(self, tmp_path):
361+
"""Test that inline scripts preserve /nemo_run paths in container mode."""
362+
# Create a SlurmExecutor with container
363+
executor = SlurmExecutor(
364+
account="test_account",
365+
partition="gpu",
366+
nodes=1,
367+
ntasks_per_node=8,
368+
container_image="nvcr.io/nvidia/pytorch:24.01-py3",
369+
)
370+
executor.job_dir = str(tmp_path / "test-job")
371+
executor.experiment_id = "exp-123"
372+
373+
# Mock tunnel
374+
tunnel = MagicMock(spec=LocalTunnel)
375+
tunnel.job_dir = "/remote/experiments/exp-123"
376+
executor.tunnel = tunnel
377+
378+
# Create an inline script with /nemo_run paths
379+
fn_or_script = Script(
380+
inline=f"cd /{RUNDIR_NAME}/code && python /{RUNDIR_NAME}/scripts/run.py"
381+
)
382+
383+
app_def = package(
384+
name="test",
385+
fn_or_script=fn_or_script,
386+
executor=executor,
387+
)
388+
389+
assert app_def.name == "test"
390+
assert len(app_def.roles) == 1
391+
392+
# Read the generated script file and verify paths were NOT substituted
393+
script_file = tmp_path / "test-job" / "scripts" / "test.sh"
394+
assert script_file.exists()
395+
396+
content = script_file.read_text()
397+
398+
# Should contain /nemo_run paths (not substituted)
399+
assert f"/{RUNDIR_NAME}/code" in content
400+
assert f"/{RUNDIR_NAME}/scripts/run.py" in content
401+
402+
def test_package_script_path_not_affected_by_non_container_mode(self, tmp_path):
403+
"""Test that path-based scripts are not affected by non-container mode substitution."""
404+
# Create a SlurmExecutor without container
405+
executor = SlurmExecutor(
406+
account="test_account",
407+
partition="gpu",
408+
nodes=1,
409+
ntasks_per_node=8,
410+
container_image=None, # Non-container mode
411+
)
412+
executor.job_dir = str(tmp_path / "test-job")
413+
executor.experiment_id = "exp-123"
414+
415+
# Mock tunnel
416+
tunnel = MagicMock(spec=LocalTunnel)
417+
tunnel.job_dir = "/remote/experiments/exp-123"
418+
executor.tunnel = tunnel
419+
420+
# Create a path-based script (not inline)
421+
fn_or_script = Script(
422+
path="test.py",
423+
args=["--config", f"/{RUNDIR_NAME}/configs/config.yaml"],
424+
)
425+
426+
app_def = package(
427+
name="test",
428+
fn_or_script=fn_or_script,
429+
executor=executor,
430+
)
431+
432+
assert app_def.name == "test"
433+
assert len(app_def.roles) == 1
434+
role = app_def.roles[0]
435+
436+
# Path-based scripts don't write files, so args should remain unchanged
437+
# (the substitution only affects inline script file content)
438+
assert role.args == ["test.py", "--config", f"/{RUNDIR_NAME}/configs/config.yaml"]

test/test_config.py

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -391,6 +391,45 @@ def test_inline_script(self):
391391
"\"echo 'test'\"",
392392
]
393393

394+
def test_to_command_substitute_rundir_path(self, tmp_path):
395+
"""Test that substitute_rundir_path substitutes /nemo_run paths in inline scripts."""
396+
from nemo_run.config import RUNDIR_NAME
397+
398+
script = Script(inline=f"cd /{RUNDIR_NAME}/code && python /{RUNDIR_NAME}/scripts/run.py")
399+
filename = str(tmp_path / "test_script.sh")
400+
401+
script.to_command(
402+
filename=filename,
403+
substitute_rundir_path="/remote/experiments/exp-123/test-job",
404+
)
405+
406+
# Read the file and verify paths were substituted
407+
with open(filename) as f:
408+
content = f.read()
409+
410+
assert f"/{RUNDIR_NAME}" not in content
411+
assert "/remote/experiments/exp-123/test-job/code" in content
412+
assert "/remote/experiments/exp-123/test-job/scripts/run.py" in content
413+
414+
def test_to_command_without_substitute_rundir_path(self, tmp_path):
415+
"""Test that paths are NOT substituted when substitute_rundir_path is None."""
416+
from nemo_run.config import RUNDIR_NAME
417+
418+
script = Script(inline=f"cd /{RUNDIR_NAME}/code && python /{RUNDIR_NAME}/scripts/run.py")
419+
filename = str(tmp_path / "test_script.sh")
420+
421+
script.to_command(
422+
filename=filename,
423+
substitute_rundir_path=None, # Default, no substitution
424+
)
425+
426+
# Read the file and verify paths were NOT substituted
427+
with open(filename) as f:
428+
content = f.read()
429+
430+
assert f"/{RUNDIR_NAME}/code" in content
431+
assert f"/{RUNDIR_NAME}/scripts/run.py" in content
432+
394433

395434
class TestGetUnderlyingTypes:
396435
def test_simple_type(self):

0 commit comments

Comments
 (0)