Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 9 additions & 16 deletions nemo_run/core/packaging/git.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,22 +88,15 @@ def _concatenate_tar_files(
quoted_files = [shlex.quote(f) for f in files_to_concatenate]
quoted_output_file = shlex.quote(output_file)

if os.uname().sysname == "Linux":
# Start from the first archive then append the rest, to avoid self-append issues
first_file, *rest_files = quoted_files
ctx.run(f"cp {first_file} {quoted_output_file}")
if rest_files:
ctx.run(f"tar Af {quoted_output_file} {' '.join(rest_files)}")
# Remove all input fragments
ctx.run(f"rm {' '.join(quoted_files)}")
else:
# Extract all fragments and repack once (faster than iterative extract/append)
temp_dir = f"temp_extract_{uuid.uuid4()}"
ctx.run(f"mkdir -p {temp_dir}")
for file in quoted_files:
ctx.run(f"tar xf {file} -C {temp_dir}")
ctx.run(f"tar cf {quoted_output_file} -C {temp_dir} .")
ctx.run(f"rm -r {temp_dir} {' '.join(quoted_files)}")
# Extract all fragments and repack once (faster than iterative extract/append)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a comment to not use tar Af so future devs don't accidentally revert it? The bug is non-obvious.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes will do

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

# Note: Avoid using tar Af based solution as it does not properly concatenate
# tar files for additional filepaths and submodules.
temp_dir = f"temp_extract_{uuid.uuid4()}"
ctx.run(f"mkdir -p {temp_dir}")
for file in quoted_files:
ctx.run(f"tar xf {file} -C {temp_dir}")
ctx.run(f"tar cf {quoted_output_file} -C {temp_dir} .")
ctx.run(f"rm -r {temp_dir} {' '.join(quoted_files)}")

def package(self, path: Path, job_dir: str, name: str) -> str:
output_file = os.path.join(job_dir, f"{name}.tar.gz")
Expand Down
4 changes: 2 additions & 2 deletions nemo_run/run/experiment.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@
from nemo_run.core.execution.lepton import LeptonExecutor
from nemo_run.core.execution.local import LocalExecutor
from nemo_run.core.execution.skypilot import SkypilotExecutor
from nemo_run.core.execution.slurm import SlurmExecutor
from nemo_run.core.execution.skypilot_jobs import SkypilotJobsExecutor
from nemo_run.core.execution.slurm import SlurmExecutor
from nemo_run.core.frontend.console.api import CONSOLE, configure_logging, deconfigure_logging
from nemo_run.core.serialization.zlib_json import ZlibJSONSerializer
from nemo_run.core.tunnel.client import SSHTunnel, Tunnel
Expand Down Expand Up @@ -639,7 +639,7 @@ def run(
If sequential=True, all tasks will be run one after the other.
The order is based on the order in which they were added.

Parallel mode only works if all exectuors in the experiment support it.
Parallel mode only works if all executors in the experiment support it.
Currently, all executors support parallel mode.

In sequential mode, if all executor supports dependencies, then all tasks will be scheduled at once
Expand Down
41 changes: 0 additions & 41 deletions test/core/packaging/test_git.py
Original file line number Diff line number Diff line change
Expand Up @@ -463,47 +463,6 @@ def test_concatenate_tar_files_non_linux_integration(tmp_path, monkeypatch):
assert names == ["./fileA.txt", "./fileB.txt"]


def test_concatenate_tar_files_linux_emits_expected_commands(monkeypatch, tmp_path):
# Simulate Linux branch; use a dummy Context that records commands instead of executing
monkeypatch.setattr(os, "uname", lambda: SimpleNamespace(sysname="Linux"))

class DummyContext:
def __init__(self):
self.commands: list[str] = []

def run(self, cmd: str, **_kwargs):
self.commands.append(cmd)

# Support ctx.cd(...) context manager API
def cd(self, _path: Path):
class _CD:
def __enter__(self_nonlocal):
return self

def __exit__(self_nonlocal, exc_type, exc, tb):
return False

return _CD()

# Fake inputs (do not need to exist since we don't execute)
tar1 = str(tmp_path / "one.tar")
tar2 = str(tmp_path / "two.tar")
tar3 = str(tmp_path / "three.tar")
out_tar = str(tmp_path / "out.tar")

ctx = DummyContext()
packager = GitArchivePackager()
packager._concatenate_tar_files(ctx, out_tar, [tar1, tar2, tar3])

# Expected sequence: cp first -> tar Af rest -> rm all inputs
assert len(ctx.commands) == 3
assert ctx.commands[0] == f"cp {shlex.quote(tar1)} {shlex.quote(out_tar)}"
assert (
ctx.commands[1] == f"tar Af {shlex.quote(out_tar)} {shlex.quote(tar2)} {shlex.quote(tar3)}"
)
assert ctx.commands[2] == f"rm {shlex.quote(tar1)} {shlex.quote(tar2)} {shlex.quote(tar3)}"


@patch("nemo_run.core.packaging.git.Context", MockContext)
def test_include_pattern_length_mismatch_raises(packager, temp_repo):
# Mismatch between include_pattern and include_pattern_relative_path should raise
Expand Down
Loading