Skip to content

Commit 06cf57c

Browse files
Benjamin Feuerclaude
Benjamin Feuer
and
claude
committed
fix(daytona): make SDK download primitives UPath-aware for gs:// trial dirs
_sdk_download_file/_sdk_download_dir write to the local filesystem only (Daytona SDK download_files), but jobs with a gs:// trials_dir hand them a remote GCSPath target. The verifier's reward-dir download then crashed with `TypeError: ... not 'GCSPath'`, raising DownloadVerifierDirError and dropping verifier_result for ~59% of trials on the verifier-enabled config — data loss: the in-sandbox verifier ran and produced a reward that never made it out. Download into a TemporaryDirectory then copy bytes into the remote UPath target via fsspec when the target is non-local; local targets keep the original code path unchanged. Fixes verifier-reward, artifact, and agent-log collection on gs:// trial dirs. Mounted and verifier-OFF paths are unaffected (they never reach these methods). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
1 parent 34b8dd6 commit 06cf57c

2 files changed

Lines changed: 157 additions & 4 deletions

File tree

‎src/harbor/environments/daytona.py‎

Lines changed: 69 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
from uuid import uuid4
1313

1414
from tenacity import retry, stop_after_attempt, wait_exponential
15+
from upath import UPath
1516

1617
from harbor.environments.base import (
1718
BaseEnvironment,
@@ -82,6 +83,33 @@
8283
"CreateSandboxFromImageParams", "CreateSandboxFromSnapshotParams"
8384
]
8485

86+
_LOCAL_PROTOCOLS = ("", "file", "local")
87+
88+
89+
def _local_download_target(target: Path | str) -> Path | None:
90+
"""Return a local ``Path`` for *target*, or ``None`` if it is remote.
91+
92+
The Daytona SDK can only write to the local filesystem. Callers may pass a
93+
remote ``UPath`` (e.g. ``gs://...`` when the trial dir lives in object
94+
storage); in that case the caller must stage downloads through a local temp
95+
directory and copy the bytes into the remote target itself. A return value
96+
of ``None`` signals that remote staging is required.
97+
"""
98+
upath = target if isinstance(target, UPath) else UPath(str(target))
99+
if upath.protocol in _LOCAL_PROTOCOLS:
100+
return Path(upath.path)
101+
return None
102+
103+
104+
def _copy_local_tree_to_remote(local_root: Path, remote_root: UPath) -> None:
105+
"""Copy every file under *local_root* into the remote ``UPath`` *remote_root*."""
106+
remote_root.mkdir(parents=True, exist_ok=True)
107+
for local_file in local_root.rglob("*"):
108+
if local_file.is_file():
109+
remote_file = remote_root / local_file.relative_to(local_root).as_posix()
110+
remote_file.parent.mkdir(parents=True, exist_ok=True)
111+
remote_file.write_bytes(local_file.read_bytes())
112+
85113

86114
def _daytona_preflight() -> None:
87115
has_api_key = bool(os.environ.get("DAYTONA_API_KEY"))
@@ -1554,22 +1582,59 @@ async def _sdk_upload_dir(self, source_dir: Path | str, target_dir: str):
15541582
reraise=True,
15551583
)
15561584
async def _sdk_download_file(self, source_path: str, target_path: Path | str):
1557-
"""Download a file from the sandbox filesystem via the Daytona SDK."""
1585+
"""Download a file from the sandbox filesystem via the Daytona SDK.
1586+
1587+
When *target_path* is a remote ``UPath`` (e.g. ``gs://...``), the SDK's
1588+
local-only download is staged through a temp file and the bytes are
1589+
copied into the remote target.
1590+
"""
15581591
if not self._sandbox:
15591592
raise RuntimeError("Sandbox not found. Please build the environment first.")
1560-
await self._sandbox.fs.download_file(source_path, str(target_path))
1593+
1594+
local_target = _local_download_target(target_path)
1595+
if local_target is not None:
1596+
local_target.parent.mkdir(parents=True, exist_ok=True)
1597+
await self._sandbox.fs.download_file(source_path, str(local_target))
1598+
return
1599+
1600+
remote_target = UPath(str(target_path))
1601+
with tempfile.TemporaryDirectory() as staging_dir:
1602+
staged = Path(staging_dir) / Path(source_path).name
1603+
await self._sandbox.fs.download_file(source_path, str(staged))
1604+
remote_target.parent.mkdir(parents=True, exist_ok=True)
1605+
remote_target.write_bytes(staged.read_bytes())
15611606

15621607
@retry(
15631608
stop=stop_after_attempt(2),
15641609
wait=wait_exponential(multiplier=1, min=1, max=10),
15651610
reraise=True,
15661611
)
15671612
async def _sdk_download_dir(self, source_dir: str, target_dir: Path | str):
1568-
"""Download a directory from the sandbox filesystem via the Daytona SDK."""
1613+
"""Download a directory from the sandbox filesystem via the Daytona SDK.
1614+
1615+
The SDK writes only to the local filesystem. When *target_dir* is a
1616+
remote ``UPath`` (e.g. ``gs://...``), files are downloaded into a local
1617+
temp directory and then copied into the remote target.
1618+
"""
1619+
if not self._sandbox:
1620+
raise RuntimeError("Sandbox not found. Please build the environment first.")
1621+
1622+
local_target = _local_download_target(target_dir)
1623+
if local_target is not None:
1624+
await self._download_dir_to_local(source_dir, local_target)
1625+
return
1626+
1627+
remote_target = UPath(str(target_dir))
1628+
with tempfile.TemporaryDirectory() as staging_dir:
1629+
staging = Path(staging_dir)
1630+
await self._download_dir_to_local(source_dir, staging)
1631+
_copy_local_tree_to_remote(staging, remote_target)
1632+
1633+
async def _download_dir_to_local(self, source_dir: str, target_dir: Path):
1634+
"""Download every file under *source_dir* in the sandbox into local *target_dir*."""
15691635
if not self._sandbox:
15701636
raise RuntimeError("Sandbox not found. Please build the environment first.")
15711637

1572-
target_dir = Path(target_dir)
15731638
target_dir.mkdir(parents=True, exist_ok=True)
15741639

15751640
search_result = await self._sandbox.fs.search_files(source_dir, "*")

‎tests/unit/environments/test_daytona.py‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,12 @@
33
import json
44
import logging
55
import shlex
6+
from dataclasses import dataclass
67
from pathlib import Path
78
from typing import cast
89

910
import pytest
11+
from upath import UPath
1012

1113
from harbor.environments.daytona import (
1214
DaytonaClientManager,
@@ -602,3 +604,89 @@ async def fake_sleep(delay: float) -> None:
602604
await env._politeness_jitter()
603605
assert len(captured) == 1
604606
assert 0.0 <= captured[0] <= DaytonaEnvironment._LAUNCH_JITTER_MAX_SEC
607+
608+
609+
# ── SDK download to local vs. remote (GCS-style) targets ──────────────
610+
611+
612+
@dataclass
613+
class _FakeFileInfo:
614+
is_dir: bool
615+
616+
617+
class _FakeDaytonaFs:
618+
"""Minimal stand-in for ``AsyncSandbox.fs``.
619+
620+
Holds an in-sandbox tree of ``{path: bytes}`` and mimics the real Daytona
621+
SDK contract: the download primitives write to the *local* filesystem.
622+
"""
623+
624+
def __init__(self, files: dict[str, bytes]):
625+
self._files = files
626+
627+
async def search_files(self, source_dir: str, pattern: str):
628+
prefix = source_dir.rstrip("/") + "/"
629+
matched = [p for p in self._files if p.startswith(prefix)]
630+
return type("SearchResult", (), {"files": matched})()
631+
632+
async def get_file_info(self, path: str):
633+
return _FakeFileInfo(is_dir=path not in self._files)
634+
635+
async def download_file(self, source_path: str, destination: str):
636+
Path(destination).write_bytes(self._files[source_path])
637+
638+
async def download_files(self, files):
639+
for req in files:
640+
Path(req.destination).write_bytes(self._files[req.source])
641+
642+
643+
class _FakeSandbox:
644+
def __init__(self, files: dict[str, bytes]):
645+
self.fs = _FakeDaytonaFs(files)
646+
647+
648+
class TestSdkDownloadTargets:
649+
"""``_sdk_download_*`` must land files at both local and remote targets.
650+
651+
Regression for the UPath/GCS trial-dir migration: a ``gs://`` trial dir
652+
makes ``TrialPaths.verifier_dir`` a remote ``UPath`` so the verifier's
653+
``download_dir`` target was no longer a local path. The Daytona SDK only
654+
writes locally, so remote targets must be staged through a temp dir and
655+
copied into object storage.
656+
"""
657+
658+
@pytest.fixture
659+
def env(self, temp_dir: Path):
660+
e = _make_env(temp_dir, compose=False)
661+
e._sandbox = cast(
662+
object,
663+
_FakeSandbox(
664+
{
665+
"/logs/verifier/reward.json": b'{"reward": 1.0}',
666+
"/logs/verifier/test-stdout.txt": b"ok\n",
667+
}
668+
),
669+
)
670+
return e
671+
672+
async def test_download_dir_to_local_target(self, env, temp_dir: Path):
673+
target = temp_dir / "out_verifier"
674+
await env._sdk_download_dir("/logs/verifier", target)
675+
assert json.loads((target / "reward.json").read_text()) == {"reward": 1.0}
676+
assert (target / "test-stdout.txt").read_text() == "ok\n"
677+
678+
async def test_download_dir_to_remote_target(self, env):
679+
# ``str(GCSPath)`` is what the verifier passes; emulate with memory://,
680+
# a non-local fsspec protocol that needs no network or credentials.
681+
target = "memory://trial-remote/verifier"
682+
await env._sdk_download_dir(target_dir=target, source_dir="/logs/verifier")
683+
reward = UPath("memory://trial-remote/verifier/reward.json")
684+
assert json.loads(reward.read_text()) == {"reward": 1.0}
685+
assert UPath("memory://trial-remote/verifier/test-stdout.txt").read_text() == (
686+
"ok\n"
687+
)
688+
689+
async def test_download_file_to_remote_target(self, env):
690+
target = "memory://trial-remote-file/reward.json"
691+
await env._sdk_download_file("/logs/verifier/reward.json", target)
692+
assert json.loads(UPath(target).read_text()) == {"reward": 1.0}

0 commit comments

Comments
 (0)