Refactor tar packaging logic to work for submodule and extra repo (#347)

* Refactor tar packaging logic for improved performance and simplicity

Signed-off-by: smajumdar <[email protected]>

* Clarify tar repacking logic to avoid issues with concatenating tar files

Signed-off-by: smajumdar <[email protected]>

* Remove redundant test for concatenating tar files on Linux

Signed-off-by: smajumdar <[email protected]>

* spell check fix

Signed-off-by: Hemil Desai <[email protected]>

---------

Signed-off-by: smajumdar <[email protected]>
Signed-off-by: Hemil Desai <[email protected]>
Co-authored-by: Hemil Desai <[email protected]>

Author: titu1994

nemo_run/core/packaging/git.py

@@ -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)
+        # 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")

nemo_run/run/experiment.py

@@ -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
@@ -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

test/core/packaging/test_git.py

@@ -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